On Tue, 2018-07-31 at 13:06 -0600, Martin Sebor wrote: > The GCC internal %G directive takes a gcall* argument and prints > the call's inlining stack in diagnostics. The argument type makes > it unsuitable for gimple expressions such as those diagnosed by > -Warray-bounds. > > As the first step in adding inlining context to -Warray-bounds > warnings the attached patch changes the %G argument to accept > gimple* instead of gcall*. (More work is needed for %G to > preserve the location range within diagnostics so this patch > just implements the first step.)
Thanks for the patch. I'm afraid I've been touching some of the same code recently (as part of my work on dumpfile.c), so I think this patch needs rebasing and retesting (sorry!). In particular, my r263181: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01765.html ("c-family: clean up the data tables in c-format.c", aka 98605dea9f97f74e6a5e75308774c117292b184e) cleaned up part of c-format.c that your patch touches; I think your patch is from before then. Also, I noticed that your patch conflicts with my (not yet approved) patch here: [PATCH 5/5] Formatted printing for dump_* in the middle-end https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01763.html (which *adds* a usage of "gimple *" for a new pretty_printer subclass, whereas yours changes the "gcall *" usage to "gimple *"), so we need to sync up on that - I'm volunteering for me to wait for you (but please send me a heads-up email when you eventually commit). > PR tree-optimization/86650 - -Warray-bounds missing inlining context > > gcc/c/ChangeLog: > > PR tree-optimization/86650 > * c-objc-common.c (c_tree_printer): Adjust. I feel a bit hypocritical saying this, as I dislike writing ChangeLog entries, but I find this one too terse: I find myself asking "what adjustment is being made, and why?" How about something like: gcc/c/ChangeLog: PR tree-optimization/86650 * c-objc-common.c (c_tree_printer): Move usage of EXPR_LOCATION (t) and TREE_BLOCK (t) from within percent_K_format to this callsite. or somesuch (assuming that I've read the intent of the change correctly); *is* that the intent of this part of the patch? > gcc/c-family/ChangeLog: > > PR tree-optimization/86650 > * c-format.c (local_gcall_ptr_node): Rename... > (local_gimple_ptr_node): ...to this. > * c-format.h (T89_G): Adjust. Likewise, I find this too terse, and it's incomplete: it's missing the changes to gcc_diag_char_table and to init_dynamic_diag_info. How about something like this: * c-format.c (local_gcall_ptr_node): Rename... (local_gimple_ptr_node): ...to this. (gcc_diag_char_table): Update comment for "%G". (init_dynamic_diag_info): Update from "gcall *" to "gimple *". * c-format.h (T89_G): Update to be "gimple *" rather than "gcall *". FWIW I use this script to help ChangeLog entries. It saves a lot of gruntwork: https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/generate-changelog.py (but the remaining work is still tedious, alas) > gcc/cp/ChangeLog: > > PR tree-optimization/86650 > * error.c (cp_printer): Adjust. See the suggestion above for c-objc-common.c (c_tree_printer). > gcc/ChangeLog: > > PR tree-optimization/86650 > * gimple-pretty-print.c (percent_G_format): Simplify. > * tree-diagnostic.c (default_tree_printer): Adjust. > * tree-pretty-print.c (percent_K_format): Add argument. > * tree-pretty-print.h: Add argument. > * gimple-fold.c (gimple_fold_builtin_strncpy): Adjust. > * gimple-ssa-warn-restrict.h (check_bounds_or_overlap): Replace > gcall* argument with gimple*. > * gimple-ssa-warn-restrict.c (check_call): Same. > (wrestrict_dom_walker::before_dom_children): Same. > (builtin_access::builtin_access): Same. > (check_bounds_or_overlap): Same. > * tree-ssa-ccp.c (pass_post_ipa_warn::execute): Adjust. > * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Adjust. The filenames in this changelog entry aren't in alphabetical order. Was that deliberate, or an accident of the way you generated them? It makes it harder to review the change, as the changes aren't in the same order as the patch itself. I think it can occasionally be useful to do them out-of-order if it helps express the intent of the change, but I don't think that's the case here; keeping them in alphabetical order is probably best here. Please can you provide a less terse ChangeLog describing the intent of the changes. I believe the two essential things in your patch are: (a) moving the usage of EXPR_LOCATION (t) and TREE_BLOCK (t) from within percent_K/G_format out to all of their callsites, and (b) the change from gcall * to gimple *, assuming I'm reading things right, but in my pre-caffeinated state I'd greatly prefer the ChangeLog spell that out. > gcc/testsuite/ChangeLog: > > PR tree-optimization/86650 > * gcc.dg/format/gcc_diag-10.c: Adjust. [...snip...] The patch is almost ready. Please rebase to past r263181, updating accordingly, and retest, and please provide a less terse ChangeLog (then repost for re-review). Thanks Dave
