Re: [PATCH] Improved diagnostics for casts and enums
On 28 Apr, Volker Reichelt wrote:
> On 27 Apr, Martin Sebor wrote:
>> On 04/27/2017 01:29 AM, Volker Reichelt wrote:
>>> Hi,
>>>
>>> the following two patches aim at improving GCC's diagnostics to help
>>> the user to get rid of old-style casts. While old-style pointer casts
>>> are really bad and need to be weeded out quickly, old-style casts between
>>> arithmetic types are IMHO much more tolerable. The patches allow to
>>> easily distinguish between those situations.
>>
>> FWIW, it can be most helpful to include this sort of detail (and
>> similar) in diagnostics. In the case of the C-style cast, besides
>> mentioning the type of the result, it might be even more helpful
>> to mention the type of the operand because unlike that of the
>> result, its type is not apparent from the cast itself.
>
> In this particular case the operand is evaluated after the warning
> message. So I couldn't include it in the message without fiddling
> around with the logic. I was also afraid that the warning message would
> become too long.
>
>>> The first patch for cp_parser_cast_expression in parser.c just adds
>>> the target type of the cast to the diagnostic (like in
>>> maybe_warn_about_useless_cast in typeck.c).
>>>
>>> The second patch for type_to_string in error.c tackles the problem
>>> that the name of a type doesn't tell you if you have a class or just
>>> a simple enum. Similar to adding "{aka 'int'}" to types that
>>> are essentially integers, this patch adds "{enum}" to all
>>> enumeration types (and adjusts two testcases accordingly).
>>
>> In the C front end %qT prints 'enum E' for an argument of
>> an enumerated type. Is there some significance to having
>> the C++ front end print 'E { enum }' or can C++ be made
>> consistent?
>>
>> Martin
>
> Ah, good point! I was so focused on the '{aka ...}' thing that I didn't
> see the obvious: I should have used %q#T instead of just %qT to get
> the enum prefix with the type. Unfortunately, I already committed the
> patch after Nathan's OK before seeing your message.
>
> I'll prepare another patch this weekend to revert my error.c changes and
> use %q#T for the warnings about old-style and useless casts mentioned
> above.
>
> Another thought just crossed my mind: Other users might find an
> enum/class prefix helpful for different warnings. Right now they
> would have to modify the compile to use %q#T instead of %qT.
> Would it make sense to add a compiler option that sets the verbose
> flag for cp_printer unconditionally? Or am I just unaware of existing
> functionality?
>
> Regards,
> Volker
So here's the patch that reverts the special enum handling in
type_to_string and uses %q#T instead of %qT for two casting-related
diagnostics.
Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?
As one can see from the testsuite changes, there are several
casting- and conversion-related messages like "invalid conversion from",
"cannot convert", "invalid cast" that still use the simple %qT
form. I'll give it a try to use %q#T there as well and prepare a
separate patch if this really improves the diagnostics.
Regards,
Volker
2017-04-30 Volker Reichelt
* parser.c (cp_parser_cast_expression): Use %q#T instead of %qT
in old-style cast diagnostic.
* typeck.c (maybe_warn_about_useless_cast): Use %q#T instead of %qT
in useless cast diagnostic.
* error.c (type_to_string): Remove enum special handling.
Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247394)
+++ gcc/cp/parser.c (working copy)
@@ -8764,7 +8764,7 @@
&& !VOID_TYPE_P (type)
&& current_lang_name != lang_name_c)
warning (OPT_Wold_style_cast,
-"use of old-style cast to %qT", type);
+"use of old-style cast to %q#T", type);
/* Only type conversions to integral or enumeration types
can be used in constant-expressions. */
Index: gcc/cp/typeck.c
===
--- gcc/cp/typeck.c (revision 247394)
+++ gcc/cp/typeck.c (working copy)
@@ -6631,7 +6631,7 @@
? xvalue_p (expr) : lvalue_p (expr))
&& same_type_p (TREE_TYPE (expr), TREE_TYPE (type)))
|| same_type_p (TREE_TYPE (expr), type))
- warning (OPT_Wuseless_cast, "useless cast to type %qT", type);
+ warning (OPT_Wuseless_cast, "useless cast to type %q#T", type);
}
}
Index: gcc/cp/error.c
===
--- gcc/cp/error.c (revision 247394)
+++ gcc/cp/error.c (working copy)
@@ -3134,10 +3134,6 @@
if (len == aka_len && memcmp (p, p+aka_start, len) == 0)
p[len] = '\0';
}
-
- if (typ && TYPE_P (typ) && TREE_CODE (typ) == ENUMERAL_TYPE)
-pp_string (cxx_pp, M_(" {enum}"));
-
return pp_ggc_formatted_text (c
Make ipa-inline-analysis to compute expected runtime without specialization
Hi,
this patch fixes the underlying issue of PR 79224. Both inliner and ipa-cp is
trying
to maximize performance benefit at a given code size expense. The problem is
that
perfomrance benefit is computed by comparing estimated time of offline function
to
inline/specialized time. This is however not realistic, becaue offline
function time
is computed with all parameters being unknown. For example if body has
if (param)
do_something_difficult
the offline copy will not do the difficult work and the benefits of
specialization
for param==0 is only elimination of param test and possibly cheaper prologue/
less of register pressure.
This patch makes ipa-analysis to be able to compute epxected runtime of
unspecialized
function and turns inliner heuristics to use it as a base for its metrics.
This is
quite important change so re-tunning of inliner parameters will need to follow.
I would be thus interested in any humanly analyzable testcases which regress
because
of this ;)
Bootstrapped/regtested x86_64-linux, comitted.
Honza
PR ipa/79224
* ipa-inline-analysis.c (dump_predicate): Add optional parameter NL.
(account_size_time): Use two predicates - exec_pred and
nonconst_pred_ptr.
(evaluate_conditions_for_known_args): Compute both clause and
nonspec_clause.
(evaluate_properties_for_edge): Evaulate both clause and nonspec_clause.
(inline_summary_t::duplicate): Update.
(estimate_function_body_sizes): Caluculate exec and nonconst predicates
separately.
(compute_inline_parameters): Likewise.
(estimate_edge_size_and_time): Update caluclation of time.
(estimate_node_size_and_time): Compute both time and nonspecialized
time.
(estimate_ipcp_clone_size_and_time): Update.
(inline_merge_summary): Update.
(do_estimate_edge_time): Update.
(do_estimate_edge_size): Update.
(do_estimate_edge_hints): Update.
(inline_read_section, inline_write_summary): Stream both new predicates.
* ipa-inline.c (compute_uninlined_call_time): Take uninlined_call_time
as argument.
(compute_inlined_call_time): Cleanup.
(big_speedup_p): Update.
(edge_badness): Update.
* ipa-inline.h (INLINE_TIME_SCALE): Remove.
(size_time_entry): Replace predicate by exec_predicate and
nonconst_predicate.
(edge_growth_cache_entry): Cache both time nad nonspecialized time.
(estimate_edge_time): Return also nonspec_time.
(reset_edge_growth_cache): Update.
Index: ipa-inline-analysis.c
===
--- ipa-inline-analysis.c (revision 247380)
+++ ipa-inline-analysis.c (working copy)
@@ -585,10 +585,12 @@ dump_clause (FILE *f, conditions conds,
}
-/* Dump predicate PREDICATE. */
+/* Dump PREDICATE to F. CONDS a vector of conditions used when evauating
+ predicats. When NL is true new line is output at the end of dump. */
static void
-dump_predicate (FILE *f, conditions conds, struct predicate *pred)
+dump_predicate (FILE *f, conditions conds, struct predicate *pred,
+ bool nl = true)
{
int i;
if (true_predicate_p (pred))
@@ -600,7 +602,8 @@ dump_predicate (FILE *f, conditions cond
fprintf (f, " && ");
dump_clause (f, conds, pred->clause[i]);
}
- fprintf (f, "\n");
+ if (nl)
+fprintf (f, "\n");
}
@@ -660,17 +663,27 @@ dump_inline_hints (FILE *f, inline_hints
}
-/* Record SIZE and TIME under condition PRED into the inline summary. */
+/* Record SIZE and TIME to SUMMARY.
+ The accounted code will be executed when EXEC_PRED is true.
+ When NONCONST_PRED is false the code will evaulate to constant and
+ will get optimized out in specialized clones of the function. */
static void
account_size_time (struct inline_summary *summary, int size, sreal time,
- struct predicate *pred)
+ struct predicate *exec_pred,
+ struct predicate *nonconst_pred_ptr)
{
size_time_entry *e;
bool found = false;
int i;
+ struct predicate nonconst_pred;
- if (false_predicate_p (pred))
+ if (false_predicate_p (exec_pred))
+return;
+
+ nonconst_pred = and_predicates (summary->conds, nonconst_pred_ptr,
exec_pred);
+
+ if (false_predicate_p (&nonconst_pred))
return;
/* We need to create initial empty unconitional clause, but otherwie
@@ -681,7 +694,8 @@ account_size_time (struct inline_summary
gcc_assert (time >= 0);
for (i = 0; vec_safe_iterate (summary->entry, i, &e); i++)
-if (predicates_equal_p (&e->predicate, pred))
+if (predicates_equal_p (&e->exec_predicate, exec_pred)
+ && predicates_equal_p (&e->nonconst_predicate, &nonconst_pred))
{
found = true;
break;
@@ -691,7 +705,7 @@ account_size_time (struct inline_summary
i = 0;
found = true;
e = &(*summar
Re: Who broke options.h?
On Tue, 25 Apr 2017, Jakub Jelinek wrote: > Committed to trunk, 7.x will need to wait until 7.1 is released, > the rc1 is already in the works and this isn't anything new, I > see the same thing already in GCC 4.0. Thanks, Jakub! Are you planning to apply this to GCC 7 after the release of 7.1? And would you mind backporting this to active release branches? (If not, okay if I do?) Gerald
Re: Add st[pr]ncpy to stmt_kills_ref_p
On 04/29/2017 01:17 AM, Marc Glisse wrote:
Hello,
this patch seems rather trivial, once one knows that those functions
always write exactly n characters (they fill with 0 as needed at the end).
Nice! Now it just needs to be made to work the other way around
too, so the dead str{,n}cpy calls is eliminated:
void sink (void*);
void f (const char *s)
{
char a[256];
__builtin_strncpy (a, s, sizeof a);
__builtin_memset (a, 0, sizeof a);
sink (a);
}
I've opened bug 80576 for this small improvement.
Martin
Re: Add st[pr]ncpy to stmt_kills_ref_p
On Sun, 30 Apr 2017, Martin Sebor wrote:
On 04/29/2017 01:17 AM, Marc Glisse wrote:
Hello,
this patch seems rather trivial, once one knows that those functions
always write exactly n characters (they fill with 0 as needed at the end).
Nice! Now it just needs to be made to work the other way around
too, so the dead str{,n}cpy calls is eliminated:
I think I've seen related patches on the list, Prathamesh Kulkarni seems
to be working on it.
--
Marc Glisse
Re: Who broke options.h?
On Sun, Apr 30, 2017 at 07:49:45PM +0200, Gerald Pfeifer wrote: > On Tue, 25 Apr 2017, Jakub Jelinek wrote: > > Committed to trunk, 7.x will need to wait until 7.1 is released, > > the rc1 is already in the works and this isn't anything new, I > > see the same thing already in GCC 4.0. > > Thanks, Jakub! Are you planning to apply this to GCC 7 after the > release of 7.1? Yes. > And would you mind backporting this to active release branches? > (If not, okay if I do?) I'll handle it together with other 6.x and 5.x backports of my patches. Jakub
Re: [PATCH] adding missing LTO to some warning options (PR 78606)
On 01/10/2017 11:16 PM, Martin Sebor wrote:
+ __builtin_sprintf (d, "%32s", "x"); /* { dg-warning "directive writing 32 bytes into a
region of size 12" "-Wformat-length" { xfail *-*-* } } */
This xpasses for me on an older system:
...
XPASS: gcc.dg/pr78768.c -Wformat-overflow (test for warnings, line 11)
...
The mechanism is as follows:
- the system doesn't have linker plugin support, and sets
HAVE_LTO_PLUGIN to 0
- consequently, opts.c:finish_options() sets x_flag_fat_lto_objects to 1
in cc1
- cgraphunit.c:symbol_table::compile() does not return
here:
...
/* Do nothing else if any IPA pass found errors or if we are just
streaming LTO. */
if (seen_error ()
|| (!in_lto_p && flag_lto && !flag_fat_lto_objects))
{
timevar_pop (TV_CGRAPHOPT);
return;
}
...
and ends up calling expand_all_functions, which calls
pass_sprintf_length
- the warning is generated by cc1
Maybe the test needs:
...
/* { dg-require-linker-plugin "" } */
...
?
Thanks,
- Tom
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
Hi Martin,
Thanks much for doing this. A few comments below, in light of my
experience doing the equivalent checks in the gdb patch linked below,
using standard C++11.
On 04/29/2017 09:09 PM, Martin Sebor wrote:
> Calling memset, memcpy, or similar to write to an object of
> a non-trivial type (such as one that defines a ctor or dtor,
> or has such a member) can break the invariants otherwise
> maintained by the class and cause undefined behavior.
>
> The motivating example that prompted this work was a review of
> a change that added to a plain old struct a new member with a ctor
> and dtor (in this instance the member was of type std::vector).
>
> To help catch problems of this sort some projects (such as GDB)
> have apparently even devised their own clever solutions to detect
> them: https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html.
>
> The attached patch adds a new warning, -Wnon-trivial-memaccess,
> that has GCC detect these mistakes. The patch also fixes up
> a handful of instances of the problem in GCC. These instances
> correspond to the two patterns below:
>
> struct A
> {
> void *p;
> void foo (int n) { p = malloc (n); }
> ~A () { free (p); }
> };
>
> void init (A *a)
> {
> memset (a, 0, sizeof *a);
> }
>
> and
>
> struct B
> {
> int i;
> ~A ();
> };
(typo: "~B ();")
>
> void copy (B *p, const B *q)
> {
> memcpy (p, q, sizeof *p);
> ...
>}
>
IMO the check should be relaxed from "type is trivial" to "type is
trivially copyable" (which is what the gdb detection at
https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
uses for memcpy/memmove). Checking that the destination is trivial is
going to generate false positives -- specifically, [basic-types]/3
specifies that it's fine to memcpy trivially _copyable_ types, not
trivial types. A type can be both non-trivial and trivially copyable
at the same time. For example, this compiles, but triggers
your new warning:
#include
#include
#include
struct NonTrivialButTriviallyCopyable
{
NonTrivialButTriviallyCopyable () : i (0) {}
int i;
};
static_assert (!std::is_trivial::value, "");
static_assert
(std::is_trivially_copyable::value, "");
void copy (NonTrivialButTriviallyCopyable *dst, NonTrivialButTriviallyCopyable
*src)
{
memcpy (dst, src, sizeof (*src));
}
$ /opt/gcc/bin/g++ -std=gnu++11 trivial-warn.cc -o trivial-warn -g3 -O0 -Wall
-Wextra -c
trivial-warn.cc: In function ‘void copy(NonTrivialButTriviallyCopyable*,
NonTrivialButTriviallyCopyable*)’:
trivial-warn.cc:16:34: warning: calling ‘void* memcpy(void*, const void*,
size_t)’ with a pointer to a non-trivial type ‘struct
NonTrivialButTriviallyCopyable’ [-Wnon-trivial-memaccess]
memcpy (dst, src, sizeof (*src));
^
$
Implementations of vector-like classes can very well (and are
encouraged) to make use of std::is_trivially_copyable to know whether
they can copy a range of elements to new storage
using memcpy/memmove/mempcpy.
Running your patch against GDB trips on such a case:
src/gdb/btrace.h: In function ‘btrace_insn_s*
VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const
btrace_insn_s*, const char*, unsigned int)’:
src/gdb/common/vec.h:948:62: error: calling ‘void* memmove(void*, const void*,
size_t)’ with a pointer to a non-trivial type ‘btrace_insn_s {aka struct
btrace_insn}’ [-Werror=non-trivial-memaccess]
memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));\
^
There is nothing wrong with the code being warned here.
While "struct btrace_insn" is trivial (has a user-provided default
ctor), it is still trivially copyable.
Now, this gdb code is using the old VEC (originated from
gcc's C days, it's not the current C++fied VEC implementation),
but the point is that any other random vector-like container out there
is free to optimize copy of a range of non-trivial but trivially
copyable types using memcpy/memmove.
Note that libstdc++ does not actually do that optimization, but
that's just a missed optimization, see PR libstdc++/68350 [1]
"std::uninitialized_copy overly restrictive for
trivially_copyable types". (libstdc++'s std::vector defers
copy to std::unitialized_copy.)
[1] - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350
> These aren't undefined and the patch could be tweaked to allow
> them.
I think they're undefined because the types are not trivially
copyable (non-trivial destructor with side effects).
> I decided not to invest effort into it because, although
> not strictly erroneous, I think they represent poor practice.
I think that for my suggestion, all you really need is use
call trivially_copyable_p instead of trivial_type_p, for
memcpy/memmove/mempcpy at least.
For memset, I'd suggest to go the other direction actually, and
instead of relaxing the trivial check, to tighten it, by warning about
memset'ting objects of non-sta
Re: [PATCH] RISC-V: Don't built 64-bit multilibs on 32-bit targets
On Sat, 29 Apr 2017, Richard Biener wrote: > On April 29, 2017 3:59:27 AM GMT+02:00, Palmer Dabbelt > wrote: > >We've been telling people that "riscv32-*" and "riscv64-*" are exactly > >the same toolchain aside from defaults for "-march" and "-mabi", but it > >appears we were lying. As far as I can tell, binutils doesn't support > >64-bit targets when it has been configured for a 32-bit target. This > >seems to be an upstream limitation that we can't fix in the RISC-V > >port. > >This means that building the toolchain with "--with-arch=rv32i > >--enable-multilib" will fail when building the 64-bit multilibs. > > > >This patch adds two new multilib target fragments that don't add the > >64-bit multilibs. This fixes the build, but has the disadvantage of > >making our 32-bit and 64-bit toolchains very different. > > PowerPC for 32bit can use powerpc64 as target and enable 32bit code-gen > by default at configure time. Maybe this is an option for riscv as > well. Specifically, in such cases the GCC configure option is --enable-target=all to enable 64-bit multilibs for a default-32-bit target, and the binutils/GDB configure option is --enable-64-bit-bfd. But you can also make a binutils/GDB target include 64-bit support unconditionally without requiring --enable-64-bit-bfd. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
On Sat, 29 Apr 2017, Martin Sebor wrote: > +The safe way to either initialize or "reset" objects of non-trivial Should use TeX quotes in Texinfo files, ``reset''. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] RISC-V: Don't built 64-bit multilibs on 32-bit targets
On Sun, 30 Apr 2017 16:38:35 PDT (-0700), jos...@codesourcery.com wrote: > On Sat, 29 Apr 2017, Richard Biener wrote: >> On April 29, 2017 3:59:27 AM GMT+02:00, Palmer Dabbelt >> wrote: >> >We've been telling people that "riscv32-*" and "riscv64-*" are exactly >> >the same toolchain aside from defaults for "-march" and "-mabi", but it >> >appears we were lying. As far as I can tell, binutils doesn't support >> >64-bit targets when it has been configured for a 32-bit target. This >> >seems to be an upstream limitation that we can't fix in the RISC-V >> >port. >> >This means that building the toolchain with "--with-arch=rv32i >> >--enable-multilib" will fail when building the 64-bit multilibs. >> > >> >This patch adds two new multilib target fragments that don't add the >> >64-bit multilibs. This fixes the build, but has the disadvantage of >> >making our 32-bit and 64-bit toolchains very different. >> >> PowerPC for 32bit can use powerpc64 as target and enable 32bit code-gen >> by default at configure time. Maybe this is an option for riscv as >> well. I think we already support this as well: you can build RISC-V with "--target=riscv64-*" and "--with-isa=rv32g" which would build the 64-bit toolchain that defaults to a 32-bit target. We can (and pretty much always do, which is why this bug went unnoticed) generate 32-bit objects using our 64-bit toolchain. The bug here is that a LD that's built with "--target=riscv32*" can't handle 64-bit ELFs > Specifically, in such cases the GCC configure option is > --enable-target=all to enable 64-bit multilibs for a default-32-bit > target, and the binutils/GDB configure option is --enable-64-bit-bfd. But > you can also make a binutils/GDB target include 64-bit support > unconditionally without requiring --enable-64-bit-bfd. Do you happen to know how to do that (enable 64-bit BFDs in a 32-bit binutils build)? I'd prefer to have the riscv32 and riscv64 ports the same if possible, but poked around a bit and couldn't find a way to do it. If you just know some other port does this then I can probably figure it out from there. I'd hoped that submitting this patch would lead to someone telling me a better way to do it :).
