Since revision r207465, all names of function clones in g++ (as opposed to the C front-end) are "<built-in>", in dumps and even in warnings, for example: mjambor@virgil:~/gcc/bisect/test/clonenames$ ~/gcc/bisect/inst/bin/g++ -O3 -S -Wall zz.C -fno-inline zz.C: In function ‘<built-in>’: zz.C:14:13: warning: iteration 3u invokes undefined behavior [-Waggressive-loop-optimizations] z[i] = i; ^ zz.C:13:3: note: containing loop for (int i = 0; i < s; i++) ^ zz.C:14:8: warning: array subscript is above array bounds [-Warray-bounds] z[i] = i; ^ mjambor@virgil:~/gcc/bisect/test/clonenames$ cat zz.C extern int sum; void do_sum (char *p) { for (int i = 0; i < 2; i++) sum += p[i]; } void foo (int s) { char z[3]; for (int i = 0; i < s; i++) z[i] = i; do_sum (z); } int bar (int i) { foo (4); return 0; }
This is because in dump_decl in gcc/cp/error.c there is: case FUNCTION_DECL: if (! DECL_LANG_SPECIFIC (t)) pp_string (pp, M_("<built-in>")); and the revision causing this clears DECL_LANG_SPECIFIC.
(In reply to Martin Jambor from comment #0) > mjambor@virgil:~/gcc/bisect/test/clonenames$ ~/gcc/bisect/inst/bin/g++ -O3 > -S -Wall zz.C -fno-inline > zz.C: In function ‘<built-in>’: > zz.C:14:13: warning: iteration 3u invokes undefined behavior > [-Waggressive-loop-optimizations] > z[i] = i; > ^ What is 3u? > zz.C:13:3: note: containing loop Wouldn't be more clear to say "within this loop"? Isn't this a regression?
(In reply to Manuel López-Ibáñez from comment #2) > (In reply to Martin Jambor from comment #0) > > mjambor@virgil:~/gcc/bisect/test/clonenames$ ~/gcc/bisect/inst/bin/g++ -O3 > > -S -Wall zz.C -fno-inline > > zz.C: In function ‘<built-in>’: > > zz.C:14:13: warning: iteration 3u invokes undefined behavior > > [-Waggressive-loop-optimizations] > > z[i] = i; > > ^ > > What is 3u? > > > zz.C:13:3: note: containing loop > > Wouldn't be more clear to say "within this loop"? > I really cannot believe that after all this time, there is no printf-like code to print a double_int. Why not %I? Interestingly, there is pp_double_int, so it is obvious to me that this %E trick is just an ugly hack. Index: tree-ssa-loop-niter.c =================================================================== --- tree-ssa-loop-niter.c (revision 208648) +++ tree-ssa-loop-niter.c (working copy) @@ -2626,15 +2626,17 @@ do_warn_aggressive_loop_optimizations (s edge e = single_exit (loop); if (e == NULL) return; gimple estmt = last_stmt (e->src); +#pragma GCC diagnostic ignored "-Wformat" +#pragma GCC diagnostic ignored "-Wformat-extra-args" if (warning_at (gimple_location (stmt), OPT_Waggressive_loop_optimizations, - "iteration %E invokes undefined behavior", - double_int_to_tree (TREE_TYPE (loop->nb_iterations), - i_bound))) - inform (gimple_location (estmt), "containing loop"); + "iteration %I invokes undefined behavior", + i_bound, TYPE_UNSIGNED (TREE_TYPE (loop->nb_iterations)))) + inform (gimple_location (estmt), "within this loop"); + loop->warned_aggressive_loop_optimizations = true; } /* Records that AT_STMT is executed at most BOUND + 1 times in LOOP. IS_EXIT is true if the loop is exited immediately after STMT, and this exit Index: tree-pretty-print.c =================================================================== --- tree-pretty-print.c (revision 208648) +++ tree-pretty-print.c (working copy) @@ -3435,32 +3435,8 @@ dump_function_header (FILE *dump_file, t } else fprintf (dump_file, ")\n\n"); } -/* Dump double_int D to pretty_printer PP. UNS is true - if D is unsigned and false otherwise. */ -void -pp_double_int (pretty_printer *pp, double_int d, bool uns) -{ - if (d.fits_shwi ()) - pp_wide_integer (pp, d.low); - else if (d.fits_uhwi ()) - pp_unsigned_wide_integer (pp, d.low); - else - { - unsigned HOST_WIDE_INT low = d.low; - HOST_WIDE_INT high = d.high; - if (!uns && d.is_negative ()) - { - pp_minus (pp); - high = ~high + !low; - low = -low; - } - /* Would "%x%0*x" or "%x%*0x" get zero-padding on all - systems? */ - sprintf (pp_buffer (pp)->digit_buffer, - HOST_WIDE_INT_PRINT_DOUBLE_HEX, - (unsigned HOST_WIDE_INT) high, low); - pp_string (pp, pp_buffer (pp)->digit_buffer); - } -} + + + Index: pretty-print.c =================================================================== --- pretty-print.c (revision 208648) +++ pretty-print.c (working copy) @@ -22,11 +22,12 @@ along with GCC; see the file COPYING3. #include "system.h" #include "coretypes.h" #include "intl.h" #include "pretty-print.h" #include "diagnostic-color.h" - +#include "double-int.h" +extern void pp_double_int (pretty_printer *pp, double_int d, bool uns); #include <new> // For placement-new. #if HAVE_ICONV #include <iconv.h> #endif @@ -261,10 +262,11 @@ pp_indent (pretty_printer *pp) %': apostrophe (should only be used in untranslated messages; translations should use appropriate punctuation directly). %.*s: a substring the length of which is specified by an argument integer. %Ns: likewise, but length specified as constant in the format string. + %I: double_int Flag 'q': quote formatted text (must come immediately after '%'). Arguments can be used sequentially, or through %N$ resp. *N$ notation Nth argument after the format string. If %N$ / *N$ notation is used, it must be used for all arguments, except %m, %%, @@ -605,10 +607,19 @@ pp_format (pretty_printer *pp, text_info s = va_arg (*text->args_ptr, const char *); pp_append_text (pp, s, s + n); } break; + + case 'I': + { + double_int i = va_arg (*text->args_ptr, double_int); + bool uns = va_arg (*text->args_ptr, int); + pp_double_int (pp, i, uns); + } + break; + default: { bool ok; gcc_assert (pp_format_decoder (pp)); @@ -896,10 +907,38 @@ pp_character (pretty_printer *pp, int c) } obstack_1grow (pp_buffer (pp)->obstack, c); ++pp_buffer (pp)->line_length; } +/* Dump double_int D to pretty_printer PP. UNS is true + if D is unsigned and false otherwise. */ +void +pp_double_int (pretty_printer *pp, double_int d, bool uns) +{ + if (d.fits_shwi ()) + pp_wide_integer (pp, d.low); + else if (d.fits_uhwi ()) + pp_unsigned_wide_integer (pp, d.low); + else + { + unsigned HOST_WIDE_INT low = d.low; + HOST_WIDE_INT high = d.high; + if (!uns && d.is_negative ()) + { + pp_minus (pp); + high = ~high + !low; + low = -low; + } + /* Would "%x%0*x" or "%x%*0x" get zero-padding on all + systems? */ + sprintf (pp_buffer (pp)->digit_buffer, + HOST_WIDE_INT_PRINT_DOUBLE_HEX, + (unsigned HOST_WIDE_INT) high, low); + pp_string (pp, pp_buffer (pp)->digit_buffer); + } +} + /* Append a STRING to the output area of PRETTY-PRINTER; the STRING may be line-wrapped if in appropriate mode. */ void pp_string (pretty_printer *pp, const char *str) {
Confirmed. IMHO the C++ FE should be less strict here...
4.8 prints t.ii: In function ‘void _Z3fooi.constprop.0()’: t.ii:14:8: warning: array subscript is above array bounds [-Warray-bounds] z[i] = i; ^ which isn't perfect either. Is there a way for the C++ FE to get at the original function decl that was cloned? Like with Index: gcc/cp/error.c =================================================================== --- gcc/cp/error.c (revision 209210) +++ gcc/cp/error.c (working copy) @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3. #include "pointer-set.h" #include "c-family/c-objc.h" #include "ubsan.h" +#include "cgraph.h" #include <new> // For placement-new. @@ -1145,7 +1146,17 @@ dump_decl (cxx_pretty_printer *pp, tree case FUNCTION_DECL: if (! DECL_LANG_SPECIFIC (t)) - pp_string (pp, M_("<built-in>")); + { + cgraph_node *node; + if ((node = cgraph_get_node (t)) + && node->former_clone_of) + { + dump_decl (pp, node->former_clone_of, flags); + pp_string (pp, M_(" <clone>")); + } + else + pp_string (pp, M_("<built-in>")); + } else if (DECL_GLOBAL_CTOR_P (t) || DECL_GLOBAL_DTOR_P (t)) dump_global_iord (pp, t); else which prints t.ii: In function 'void foo(int) <clone>': t.ii:14:13: warning: iteration 3u invokes undefined behavior [-Waggressive-loop-optimizations] z[i] = i; ^ t.ii:13:3: note: containing loop for (int i = 0; i < s; i++) ^ t.ii:14:8: warning: array subscript is above array bounds [-Warray-bounds] z[i] = i; ^ does former_clone_of apply recursively? Thus can we have a clone of a clone? Is there a way to "pretty-print" the kind of clone? That is, say <clone with foo = 1> for a ipa-cp clone with parameter foo replaced by 1?
Looks quite some usability regression to me, compared to 4.8. Making P1.
(In reply to Richard Biener from comment #5) > > does former_clone_of apply recursively? Thus can we have a clone of a clone? > Is there a way to "pretty-print" the kind of clone? That is, say > <clone with foo = 1> for a ipa-cp clone with parameter foo replaced by 1? Why would you want to do that? At least for diagnostics to the users (not debug dumps), that the function is a clone is an internal implementation detail.
(In reply to Richard Biener from comment #5) > which isn't perfect either. Is there a way for the C++ FE to get at the > original function decl that was cloned? Like with > > Index: gcc/cp/error.c > =================================================================== > --- gcc/cp/error.c (revision 209210) > +++ gcc/cp/error.c (working copy) > @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3. > #include "pointer-set.h" > #include "c-family/c-objc.h" > #include "ubsan.h" > +#include "cgraph.h" > > #include <new> // For placement-new. > > @@ -1145,7 +1146,17 @@ dump_decl (cxx_pretty_printer *pp, tree > > case FUNCTION_DECL: > if (! DECL_LANG_SPECIFIC (t)) > - pp_string (pp, M_("<built-in>")); > + { > + cgraph_node *node; > + if ((node = cgraph_get_node (t)) > + && node->former_clone_of) > + { > + dump_decl (pp, node->former_clone_of, flags); > + pp_string (pp, M_(" <clone>")); > + } > + else > + pp_string (pp, M_("<built-in>")); > + } > else if (DECL_GLOBAL_CTOR_P (t) || DECL_GLOBAL_DTOR_P (t)) > dump_global_iord (pp, t); > else > Wouldn't it be better for the middle end to provide its own diagnostic_starter function, which in turn calls the FE one and override current_function_declaration around the call to the FE one by the original function? This will also allow in the future the middle-end to override other stuff that it doesn't want the FE to deal with.
(In reply to Richard Biener from comment #5) > 4.8 prints > > t.ii: In function ‘void _Z3fooi.constprop.0()’: > t.ii:14:8: warning: array subscript is above array bounds [-Warray-bounds] > z[i] = i; > ^ I think that we can perhaps get closest to that with something like: pp_string (pp, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (t))); which prints zz.C: In function ‘_Z3fooi.constprop.0’: by using DECL_NAME instead of DECL_ASSEMBLER_NAME we can get avoid mangling: zz.C: In function ‘foo.constprop’: > > does former_clone_of apply recursively? I believe that (at least currently) former_clone_of will always be the original front-end declaration, i.e. the same as DECL_ABSTRACT_ORIGIN, despite that... > Thus can we have a clone of a clone? ...yes, the infrastructure allows that, although currently only such clones are inline clones and thus not posing this problem. > Is there a way to "pretty-print" the kind of clone? That is, say > <clone with foo = 1> for a ipa-cp clone with parameter foo replaced by 1? Hm, it would require changes elsewhere to do for aggregate replacements (we'd need to remember the old decl and we remember only offsets, not expressions), for scalar replacements it is already apparently quite possible: diff --git a/gcc/cp/error.c b/gcc/cp/error.c index 454feb5..bb8db31 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3. If not see #include "pointer-set.h" #include "c-family/c-objc.h" #include "ubsan.h" +#include "cgraph.h" #include <new> // For placement-new. @@ -1144,7 +1145,35 @@ dump_decl (cxx_pretty_printer *pp, tree t, int flags) /* Fall through. */ case FUNCTION_DECL: - if (! DECL_LANG_SPECIFIC (t)) + if (DECL_ABSTRACT_ORIGIN (t)) + { + dump_decl (pp, DECL_ABSTRACT_ORIGIN (t), flags); + + cgraph_node *node = cgraph_get_node (t); + if (node && node->clone.tree_map) + { + struct ipa_replace_map *rmap; + unsigned i; + bool first = true; + pp_string (pp, M_(" <clone with ")); + + FOR_EACH_VEC_ELT (*node->clone.tree_map, i, rmap) + { + if (first) + first = false; + else + pp_string (pp, (", ")); + + dump_expr (pp, rmap->new_tree, flags); + pp_string (pp, M_(" for ")); + dump_expr (pp, rmap->old_tree, flags); + } + pp_string (pp, ">"); + } + else + pp_string (pp, M_(" <clone>")); + } + else if (! DECL_LANG_SPECIFIC (t)) pp_string (pp, M_("<built-in>")); else if (DECL_GLOBAL_CTOR_P (t) || DECL_GLOBAL_DTOR_P (t)) dump_global_iord (pp, t); gives: zz.C: In function ‘void foo(int) <clone with 4 for s>’:
Created attachment 32574 [details] WIP patch displaying IPA-CP information aout clones This is an untested and very much WIP patch that shows how we can print additional information about clones, at least about those created by IPA-CP. Of course eventually it should not be in the front-end. It also retains more information in memory, although not that much. For the source below, it produces the following output: mjambor@virgil:~/gcc/mine/tests/pr60761-clonenames$ ~/gcc/mine/inst/bin/g++ -O3 -S -Wall aa.C -fno-inline aa.C: In function ‘void foo(int, A*) <clone with 4 for s, 10 for data pointed to by a at offset 32>’: aa.C:19:20: warning: iteration 3u invokes undefined behavior [-Waggressive-loop-optimizations] z[i] = i + a->j; ^ aa.C:18:3: note: containing loop for (int i = 0; i < s; i++) ^ aa.C:19:8: warning: array subscript is above array bounds [-Warray-bounds] z[i] = i + a->j; the source code is: extern int sum; void do_sum (char *p) { for (int i = 0; i < 2; i++) sum += p[i]; } struct A { int i, j, k; }; void foo (int s, struct A *a) { char z[3]; for (int i = 0; i < s; i++) z[i] = i + a->j; do_sum (z); } int bar (int i) { struct A a; a.j = 10; foo (4, &a); return 0; }
Won't that break say -g -O2 -Wall reporting with say: static inline void foo (char *p) { __builtin___memcpy_chk (p, "abc", 3, __builtin_object_size (p, 0)); } static inline void bar (char *p) { foo (p); } void baz (void) { char buf[2]; bar (buf); } I mean, DECL_ABSTRACT_ORIGIN is set also for inlines, fnsplit created functions and many other cases, printing <clone> after the function name in all those cases might not be appropriate.
Also, perhaps to make the change conservative enough for 4.9, might be best to not append anything now, and only look at DECL_ABSTRACT_ORIGIN (recurse on it) if !DECL_LANG_SPECIFIC. More verbose printing can be perhaps deferred for stage1.
(In reply to Jakub Jelinek from comment #11) > I mean, DECL_ABSTRACT_ORIGIN is set also for inlines, fnsplit created > functions and many other cases, printing <clone> after the function name in > all those cases might not be appropriate. It is not clear to me why you want to print <clone> at all. It is an internal detail.
(In reply to Manuel López-Ibáñez from comment #13) > It is not clear to me why you want to print <clone> at all. It is an > internal detail. Imagine a function void f(unsigned a, unsigned b) where gcc makes a clone specializing a=0. If the function contains a comparison a<=b, you might get a warning because the comparison is always true. As a user, I would be quite confused... Ok, this particular example won't happen, but I think it is still a reason why writing <clone> and maybe some more details can be useful.
(In reply to Jakub Jelinek from comment #12) > Also, perhaps to make the change conservative enough for 4.9, might be best > to not append anything now, and only look at DECL_ABSTRACT_ORIGIN (recurse > on it) if !DECL_LANG_SPECIFIC. More verbose printing can be perhaps > deferred for stage1. I agree, mostly because... (In reply to Manuel López-Ibáñez from comment #13) > It is not clear to me why you want to print <clone> at all. It is an > internal detail. ...just printing "<clone>" really provides fairly little value. If we (hopefully from the next version on) manage to print details about properties of the particular clone, it will become useful.
(In reply to Martin Jambor from comment #15) > (In reply to Manuel López-Ibáñez from comment #13) > > It is not clear to me why you want to print <clone> at all. It is an > > internal detail. > > ...just printing "<clone>" really provides fairly little value. If we > (hopefully from the next version on) manage to print details about > properties of the particular clone, it will become useful. I'm not entirely convinced by the arguments given above. Templated C++ code in diagnostics is already convoluted enough to add stuff that is not even in the language and that users will have no idea about. Imagine (real G++ output): std::basic_ostream<_CharT, _Traits>::__ostream_type& std::basic_ostream<_CharT, _Traits>::operator<<(std::basic_ostream<_CharT, _Traits>::__ostream_type& (*)(std::basic_ostream<_CharT, _Traits>::__ostream_type&)) [with _CharT = char, _Traits = std::char_traits<char>, std::basic_ostream<_CharT, _Traits>::__ostream_type = std::basic_ostream<char>] <clone> For developer/debug dumps, of course this doesn't apply, but this is not what this bug is about. (In reply to Marc Glisse from comment #14) > Imagine a function void f(unsigned a, unsigned b) where gcc makes a clone > specializing a=0. If the function contains a comparison a<=b, you might get > a warning because the comparison is always true. As a user, I would be quite > confused... This used to happen with template instantiations and it was considered a bug. That is, the compiler should not give that kind of warning for generated code if the original code does not show the same issue. If the original code shows the same issue, then we should give a warning for the original code. In general, there are at least two kinds of warnings: those for which there is a bug if we reach that code under certain conditions, and those for which the code looks suspicious but we cannot prove that there is a bug under any specific condition. Uninitialized warnings fall in the first category, whereas "comparison is always true/false" falls under the second one. The first kind of warnings would be improved by providing the exact conditions that may trigger the bug, whereas the second kind become noise if only triggered for specific conditions that are actually not present in the original code (like template specializations, macro expansion, and function cloning). For the first kind of bugs, if we explain the conditions better thanks to optimization, great, but saying: we warn because we created a clone of the function only leads to more questions: what is a clone? For the second kind, saying that we warn because we created a clone sounds more like an excuse than an actual analysis. ;-)
Author: jakub Date: Thu Apr 10 16:20:07 2014 New Revision: 209278 URL: http://gcc.gnu.org/viewcvs?rev=209278&root=gcc&view=rev Log: PR ipa/60761 * error.c (dump_decl) <case FUNCTION_DECL>: If DECL_LANG_SPECIFIC is NULL, but DECL_ABSTRACT_ORIGIN is not, recurse on DECL_ABSTRACT_ORIGIN instead of printing <built-in>. Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/error.c
No longer a regression, keeping open as enhancement request to improve the dumping for stage1.
GCC 4.9.0 has been released
GCC 4.9.1 has been released.
GCC 4.9.2 has been released.
GCC 4.9.3 has been released.
I filed PR 102061 for at least one of the issues mentioned here dealing with .constprop is being exposed.