On Wed, 2018-08-01 at 13:53 -0600, Martin Sebor wrote: > On 08/01/2018 08:33 AM, David Malcolm wrote: > > 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!). > > No problem. I have seen some of your changes but didn't spot > any serious conflicts. > > > > > 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. > > Yes, I think the only adjustments to be made here should be > to the gcall*/gimple* comments. > > > > > 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). > > I saw this patch yesterday but didn't notice a conflict. Your > changes are more extensive than mine so thanks for the offer to > deal with any potential collisions. If your patch is approved > first it shouldn't be too hard for me to adjust to yours. I > think we can just wait and see.
I'll wait until yours is committed (see below). > > > 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? > > The intent is to be able to call percent_K_format() with location > and block arguments that are different from those embedded in arg. > This is relied on in percent_G_format(), otherwise the change has > no effect on percent_K_format() or its other callers. > > Most of the changes in this patch were mechanical, and this one > is in my mind obvious: call the function with its new arguments. > No functionality has been added or removed here. But I agree > that your description is more -- descriptive? -- so I've replaced > the entry with the text as you suggest. I'm of two minds as to the merits of ChangeLog entries, but I do like to see some kind of high-level description of the proposed change somewhere in the email, either in the "blurb" in the covering email, or in the ChangeLog itself. The thing that I felt was missing in the initial patch was a description of the refactoring of percent_K_format's arguments; to me it felt like something that should be captured *somewhere* (either in the mailing list archive and/or in the ChangeLog; the latter feels like a better place). > > > 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 *". > > I've changed it to the above. (I didn't know that trivial changes > like updating comments was expected to be documented in ChangeLogs, > but fine.) For patch review in general (not just in GCC) I like every hunk in a proposed patch to be tied to some kind of explanation (however brief) in the covering material. Normally if there's a hunk that changes a comment, there's typically nearby code changes so the comment changes don't need calling out, but in this case, the hunk was for a comment in a data table and stood alone, so I felt it makes sense to document that as a change to the data table. > > > > FWIW I use this script to help ChangeLog entries. > > It saves a lot of gruntwork: > > > > https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/mast > > er/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). > > Ditto. > > > > > > 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. > > I do have a script that generates ChangeLog templates for me to > fill in. It relies on lsdiff that I think sorts files on its > own. I don't know why these aren't sorted. Probably because > I moved things around. I had no idea that anyone expected > ChangeLog entries to be in any particular order. > > I've made the change you suggest and I can see that it could > make the changes easier to cross-reference (I myself search > for each entry as go through my own review). With no > disrespect, though, I find insisting on it excessive. AFAIK, > the GNU Coding Standard doesn't require it, neither do the > GCC Coding Conventions, and there are plenty of examples of > entries by others that aren't sorted (probably for the same > reason mine weren't). If you feel that it's important they > be sorted then please bring it up for discussion. If there > is consensus to adopt this requirement then I will certainly > follow it. It's not a requirement and I'm not insisting on it, but it does make it easier for me to review patches. During review I have the ChangeLog entries in one window, and the changes themselves in another. If they ChangeLog entries are in the same order as those of the changes themselves (or close to it), it's trivial to read through the proposed change, compare each item between the two windows. If they're not in order, I have to jump around as I go through them, which makes review harder. > > 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). > > Attached is an updated and retested patch. The only code > change I had to make was resolve a conflict in a comment. > Otherwise the patch is the same. Thanks for updating it. > PR tree-optimization/86650 - -Warray-bounds missing inlining context > > 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. Nit: I believe there's a stray tab character in the above (between "percent_K_format" and "to"); please consider fixing when committing. > gcc/c-family/ChangeLog: > > PR tree-optimization/86650 Looks like you're missing a "* c-format.c" here. Also, this lost the: (local_gcall_ptr_node): Rename... (local_gimple_ptr_node): ...to this. from the previous patch. > (gcc_tdiag_char_table): Update comment for "%G". > (gcc_cdiag_char_table, gcc_cxxdiag_char_table): Same. > (init_dynamic_diag_info): Update from "gcall *" to "gimple *". > * c-format.h (T89_G): Update to be "gimple *" rather than > "gcall *". [...snip...] > gcc/ChangeLog: > > PR tree-optimization/86650 > * gimple-pretty-print.c (percent_G_format): Simplify. Again "Simplify." is too terse in my opinion; how about: * gimple-pretty-print.c (percent_G_format): Accept a "gimple *" rather than a "gcall *". Directly pass the data of interest to percent_K_format, rather than building a temporary CALL_EXPR to hold it. or somesuch - avoiding building that temporary was the point of the changes to percent_K_format, right? If I'm reading this right, there are no functional changes here, but it avoids building the temporary, and it sounds like you have follow-up patches planned that make use of the new percent_K_format args. > * gimple-fold.c (gimple_fold_builtin_strncpy): Adjust. FWIW, this also touches gimple_fold_builtin_strncat. > * 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. also "Same." for maybe_diag_overlap and maybe_diag_offset_bounds. > * tree-diagnostic.c (default_tree_printer): Move usage of > EXPR_LOCATION (t) and TREE_BLOCK (t) from within percent_K_format > to this callsite. > * tree-pretty-print.c (percent_K_format): Add argument. > * tree-pretty-print.h: Add argument. > * tree-ssa-ccp.c (pass_post_ipa_warn::execute): Adjust. > * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Adjust. FWIW, this last one misses a couple of functions, I believe it should be: * tree-ssa-strlen.c (handle_builtin_strcpy): Adjust. (maybe_diag_stxncpy_trunc): Same. (handle_builtin_strcat): Same. [...snip...] OK for trunk, with the ChangeLog nits above. Please can you mail me when you commit, so I can rebase and retest my patch accordingly. Thanks! Dave
