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] include more detail in -Warray-bounds (PR 86650)


On Mon, 2018-07-23 at 17:49 -0600, Martin Sebor wrote:
> (David, I'm hoping your your help here.  Please see the end.)
> 
> While looking into a recent -Warray-bounds instance in Glibc
> involving inlining of large functions it became apparent that
> GCC could do a better job of pinpointing the source of
> the problem.
> 
> The attached patch makes a few adjustments to both
> the pretty printer infrastructure and to VRP to make this
> possible.  The diagnostic pretty printer already has directives
> to print the inlining context for both tree and gcall* arguments,
> so most of the changes just adjust things to be able to pass in
> gimple* argument instead.
> 
> The only slightly interesting change is to print the declaration
> to which the out-of-bounds array refers if one is known.
> 
> Tested on x86_64-linux with one regression.
> 
> The regression is in the gcc.dg/Warray-bounds.c test: the column
> numbers of the warnings are off.  Adding the %G specifier to
> the array bounds warnings in VRP has the unexpected effect of
> expanding the extent of the underling. For instance, for a test
> case like this:
> 
>    int a[10];
> 
>    void f (void)
>    {
>      a[-1] = 0;
>    }
> 
> from the expected:
> 
>     a[-1] = 0;
>     ~^~~~
> 
> to this:
> 
>    a[-1] = 0;
>     ~~~~~~^~~
> 
> David, do you have any idea how to avoid this?

Are you referring to the the various places in your patch (in e.g.
  vrp_prop::check_array_ref
  vrp_prop::check_mem_ref
  vrp_prop::search_for_addr_array
) where the patch changed things from this form:

  warning_at (location, OPT_Warray_bounds,
              "[...format string...]", ARGS...);

to this form:

  warning_at (location, OPT_Warray_bounds,
              "%G[...format string...]", stmt, ARGS...);

If so, there are two location_t values of interest here:
(a) the "location" value, and
(b) gimple_location (stmt)

My recollection is that %G and %K override the "location" value passed
in as the first param to the diagnostic call, overwriting it within the
diagnostic_info's text_info with the location value from the %K/%G
(which also set up the pp_ti_abstract_origin of the text_info from the
block information stashed in the ad-hoc data part of the location, so
that the pretty-printer prints the inlining chain).

[aside, why don't we always just print the inlining chain?  IIRC, %K
and %G feel too much like having to jump through hoops to me, given
that gimple_block is looking at gimple_location anyway, why not just
use the location in the location_t's ad-hoc data; I have a feeling
there's a PR open about this, but I don't have it to hand right now].

It looks like the old location was (a), and now you're seeing (b),
since (b) is the location of the statement.

Whether or not this is a problem is difficult to tell; what does the
full diagnostic look like? (you only quoted the diagnostic_show_locus
part of it).

Hope this is helpful
Dave


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