[PATCH 0/4] improve warning suppression for inlined functions (PR 98512)

[Martin Sebor <msebor@gmail.com>]

David, this is a revised patch set based on your feedback below
and our discussion.  It works with the existing warning_at()
interface without introducing any new overloads (for now).  It
resolves PR middle-end/98871 and 98512.  PR 98465 was handled
in GCC 11 differently so this has no impact on that problem
anymore.

The first diff in the series introduces the diagnostic
infrastructure changes.

The second one removes the uses of %G and %K from all warning_at()
calls throughout GCC front end and middle end.  The inlining context
is included in diagnostic output whenever it's present.

The third removes the uses of %K from error() calls in the arm and
aarch64 back end.

The fourth and final diff then removes the handlers from the pretty
printer and the support for the directives from c-format.c.

On 5/19/21 7:41 AM, David Malcolm wrote:
> On Thu, 2021-01-21 at 16:46 -0700, Martin Sebor via Gcc-patches wrote:
> 
> Martin and I had a chat about this patch, but it's best to discuss code
> on the mailing list rather than in a silo, so here goes...
> 
> 
>> The initial patch I posted is missing initialization for a couple
>> of locals.  I'd noticed it in testing but forgot to add the fix to
>> the patch before posting it.  I have corrected that in the updated
>> revision and also added the test case from pr98512, and retested
>> the whole thing on x86_64-linux.
>>
>> On 1/19/21 11:58 AM, Martin Sebor wrote:
>>> std::string tends to trigger a class of false positive out of bounds
>>> access warnings for code GCC cannot prove is unreachable because of
>>> missing aliasing constrains, and that ends up expanded inline into
>>> user code.  Simply inserting the contents of a constant char array
>>> does that.  In GCC 10 these false positives are suppressed due to
>>> -Wno-system-headers, but in GCC 11, to help detect calls rendered
>>> invalid by user code passing in either incorrect or insufficiently
>>> constrained arguments, -Wno-system-header no longer has this effect
>>> on invalid access warnings.
>>>
>>> To solve the problem without at least partially reverting the change
>>> and going back to the GCC 10 way of things for the affected subset
>>> of calls (just memcpy and memmove), the attached patch enhances
>>> the #pragma GCC diagnostic machinery to consider not just a single
>>> location for inlined code but all locations at which an expression
>>> and its callers are inlined all the way up the stack.  This gives
>>> each author of a function involved in inlining the ability to
>>> control a warning issued for the code, not just the user into whose
>>> code all the calls end up inlined.  To resolve PR 98465, it lets us
>>> suppress the false positives selectively in std::string rather
>>> than across the board in GCC.
> 
> I like the idea of checking the whole of the inlining stack for
> pragmas, but I don't like the way the patch implements it.
> 
> The patch provides a hook for getting a vec of locations for a
> diagnostic for use when checking for pragmas, and uses it the hook on a
> few specific diagnostics.
> 
> Why wouldn’t we always do this?  It seems to me like something we
> should always do when there's inlining information associated with a
> location, rather than being a per-diagnostic thing - but maybe there's
> something I'm missing here.  The patch adds diag_inlining_context
> instances on the stack in various places, and doing that feels to me
> like a special-case hack, when it should be fixed more generally in
> diagnostics.c
> 
> I don't like attaching the "override the location" hook to the
> diagnostic_metadata; I intended the latter to be about diagnostic
> taxonomies (CWE, coding standards, etc), rather than a place to stash
> location overrides.
> 
> One issue is that the core of the diagnostics subsystem doesn't have
> knowledge of "tree", but the inlining information requires tree-ish
> knowledge.
> 
> It's still possible to get at the inlining information from the
> diagnostics.c, but only as a void *:
> 
> input.h's has:
> 
> #define LOCATION_BLOCK(LOC) \
>    ((tree) ((IS_ADHOC_LOC (LOC)) ? get_data_from_adhoc_loc (line_table,
> (LOC)) \
>     : NULL))
> 
> Without knowing about "tree", diagnostic.c could still query a
> location_t to get at the data as a void *:
> 
>    if (IS_ADHOC_LOC (loc)
>      return get_data_from_adhoc_loc (line_table, loc);
>    else
>      return NULL;
> 
> 
> If we make the "get all the pertinent locations" hook a part of the
> diagnostic_context, we could have diagnostic_report_diagnostic check to
> see if there's ad-hoc data associated with the location and a non-NULL
> hook on the context, and if so, call it.  This avoids adding an
> indirect call for the common case where there isn't any inlining
> information, and lets us stash the implementation of the hook in the
> tree-diagnostic.c, keeping the separation of trees from diagnostic.c
> 
> One design question here is: what if there are multiple pragmas on the
> inlining stack, e.g. explicitly enabling a warning at one place, and
> explicitly ignoring that warning in another?  I don't think it would
> happen in the cases you're interested in, but it seems worth
> considering.  Perhaps the closest place to the user's code "wins".
> 
>>>
>>> The solution is to provide a new pair of overloads for warning
>>> functions that, instead of taking a single location argument, take
>>> a tree node from which the location(s) are determined.  The tree
>>> argument is indirect because the diagnostic machinery doesn't (and
>>> cannot without more intrusive changes) at the moment depend on
>>> the various tree definitions.  A nice feature of these overloads
>>> is that they do away with the need for the %K directive (and in
>>> the future also %G, with another enhancement to accept a gimple*
>>> argument).
> 
> We were chatting about this, and I believe we don't need the new
> overloads or the %K and %G directives: all of the information about
> inlining can be got from the location_t via the line_table (albeit as a
> void *) as seen above.
> 
>    gimple_block (const gimple *g)
> 
> is merely:
> 
>    return LOCATION_BLOCK (g->location);
> 
> and is thus merely looking up the information from the location_t and
> line_table; it doesn't actually use anything else about the gimple
> stmt.
> 
> 
> Does the above make sense?
> 
> Hope this is constructive
> Dave
> 
>>>
>>> This patch depends on the fix for PR 98664 (already approved but
>>> not yet checked in).  I've tested it on x86_64-linux.
>>>
>>> To avoid fallout I tried to keep the changes to a minimum, and
>>> so the design isn't as robust as I'd like it ultimately to be.
>>> I plan to enhance it in stage 1.
>>>
>>> Martin
>>
> 
>