This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] change %G argument from gcall* to gimple*


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]