[PING][PATCH 1/4] introduce diagnostic infrastructure changes (PR 98512)

[Martin Sebor <msebor@gmail.com>]

Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572839.html

On 6/15/21 5:00 PM, Martin Sebor wrote:
> On 6/11/21 11:04 AM, David Malcolm wrote:
>> On Thu, 2021-06-10 at 17:26 -0600, Martin Sebor wrote:
>>> This diff introduces the diagnostic infrastructure changes to support
>>> controlling warnings at any call site in the inlining stack and
>>> printing
>>> the inlining context without the %K and %G directives.
>>
>> Thanks for working on this, looks very promising.
>>
>>> Improve warning suppression for inlined functions.
>>>
>>> Resolves:
>>> PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized at 
>>> declaration site
>>> PR middle-end/98512 - #pragma GCC diagnostic ignored ineffective in 
>>> conjunction with alias attribute
>>
>> Am I right in thinking that you add test coverage for both of these in
>> patch 2 of the kit?
> 
> Yes, the tests depend on the changes in patch 2 (some existing tests
> fail with just patch 1 applied because the initial location passed
> to warning_t() is different than with it).
> 
>>
>>>
>>> gcc/ChangeLog:
>>>
>>>     * diagnostic.c (update_inlining_context): New.
>>>     (update_effective_level_from_pragmas): Handle inlining context.
>>>     (diagnostic_report_diagnostic): Same.
>>>     * diagnostic.h (struct diagnostic_info): Add ctor.
>>>     (struct diagnostic_context): Add members.
>>>     * tree-diagnostic.c (get_inlining_locations): New.
>>>     (set_inlining_location): New.
>>>     (tree_diagnostics_defaults): Set new callback pointers.
>>
>> [..snip...]
>>
>>> @@ -1204,7 +1256,7 @@ diagnostic_report_diagnostic 
>>> (diagnostic_context *context,
>>>         /* We do this to avoid giving the message for 
>>> -pedantic-errors.  */
>>>         orig_diag_kind = diagnostic->kind;
>>>       }
>>> -
>>> +
>>
>> Stray whitespace change?  Though it looks like a fix of a stray space,
>> so not a big deal.
>>
>>>     if (diagnostic->kind == DK_NOTE && context->inhibit_notes_p)
>>>       return false;
>>
>> [..snip...]
>>
>>> diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
>>> index 1b9d6b1f64d..b95ee23dda0 100644
>>> --- a/gcc/diagnostic.h
>>> +++ b/gcc/diagnostic.h
>>> @@ -87,6 +87,10 @@ enum diagnostics_extra_output_kind
>>>      list in diagnostic.def.  */
>>>   struct diagnostic_info
>>>   {
>>> +  diagnostic_info ()
>>> +    : message (), richloc (), metadata (), x_data (), kind (), 
>>> option_index ()
>>> +  { }
>>> +
>>
>> Why does the patch add this ctor?
> 
> The new code relies on x_data being initially null, and to make it so
> I considered two alternatives explicitly initialize the struct or add
> a ctor.  I had started with the former but wound up with the latter
> after a few ICEs.
> 
> 
>>
>>>     /* Text to be formatted.  */
>>>     text_info message;
>>> @@ -343,6 +347,32 @@ struct diagnostic_context
>>>     /* Callback for final cleanup.  */
>>>     void (*final_cb) (diagnostic_context *context);
>>> +
>>> +  /* The inlining context of the diagnostic (may have just one
>>> +     element if a diagnostic is not for an inlined expression).  */
>>> +  struct inlining_ctx
>>> +  {
>>> +    void reset ()
>>> +    {
>>> +      ilocs.release ();
>>> +      loc = UNKNOWN_LOCATION;
>>> +      ao = NULL;
>>> +      allsyslocs = false;
>>> +    }
>>> +
>>> +    /* Locations along the inlining stack.  */
>>> +    auto_vec<location_t, 8> ilocs;
>>> +    /* The locus of the diagnostic. */
>>> +    location_t loc;
>>> +    /* The abstract origin of the location.  */
>>> +    void *ao;
>>> +    /* Set of every ILOCS element is in a system header.  */
>>> +    bool allsyslocs;
>>> +  } ictx;
>>
>> Why is the inlining ctx part of the diagnostic_context?  That feels
>> strange to me. This inlining information relates to a particular
>> diagnostic, so it seems more appropriate to me that it should be part
>> of the diagnostic_info (which might thus necessitate having a ctor for
>> diagnostic_info).  Doing that might avoid the need for "reset", if I'm
>> right in assuming that getting the data is done once per diagnostic
>> during diagnostic_report_diagnostic.
> 
> I thought that's what you'd suggested when we spoke but I must have
> have misremembered or misunderstood.  I agree it fits better in
> the diagnostic_info and I've moved it there.
> 
>>
>> Alternatively, could this be state that's created on the stack during
>> diagnostic_report_diagnostic and passed around by pointer as another
>> parameter?  (putting it in diagnostic_info might be simplest though)
> 
> Yes, that sounds good to me too.
> 
>>
>> Maybe rename it to "inlining_info"?
>>
>> How involved would it be to make it be a class with private fields?
> 
> Not too involved.  It would involve adding accessors and modifiers
> for all of them.  I would normally be in favor of it but I don't
> think it's worth the effort for such a small struct that's a member
> of another that doesn't use proper encapsulation.  If/when the other
> classes in this area are encapsulated it might be a good time to do
> it for this class too.
> 
>>
>> Can the field names have "m_" prefixes, please?
> 
> Done.
> 
>>
>>> +  /* Callbacks to get and set the inlining context.  */
>>
>> Probably should spell out in the comment here that doing so requires
>> knowledge of trees, which is why it's a callback (to avoid diagnostic.c
>> from having to know about trees).
> 
> Done.
> 
>>
>>> +  void (*get_locations_cb)(diagnostic_context *, diagnostic_info *);
>>> +  void (*set_location_cb)(const diagnostic_context *, 
>>> diagnostic_info *);
>>>   };
>>>   static inline void
>>> diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
>>> index 95b8ef30070..a8c5484849a 100644
>>> --- a/gcc/tree-diagnostic.c
>>> +++ b/gcc/tree-diagnostic.c
>>> @@ -305,6 +305,84 @@ default_tree_printer (pretty_printer *pp, 
>>> text_info *text, const char *spec,
>>>     return true;
>>>   }
>>> +/* Get the inlining stack corresponding to the DIAGNOSTIC location.  */
>>> +
>>> +static void
>>> +get_inlining_locations (diagnostic_context *context,
>>> +            diagnostic_info *diagnostic)
>>> +{
>>> +  context->ictx.reset ();
>>> +
>>> +  location_t loc = diagnostic_location (diagnostic);
>>> +  tree block = LOCATION_BLOCK (loc);
>>> +
>>> +  /* Count the number of locations in system headers.  When all are,
>>> +     warnings are suppressed by -Wno-system-headers.  Otherwise, they
>>> +     involve some user code, possibly inlined into a function in a 
>>> system
>>> +     header, and are not treated as coming from system headers.  */
>>> +  unsigned nsyslocs = 0;
>>> +
>>> +  while (block && TREE_CODE (block) == BLOCK
>>> +     && BLOCK_ABSTRACT_ORIGIN (block))
>>> +    {
>>> +      tree ao = BLOCK_ABSTRACT_ORIGIN (block);
>>> +      if (TREE_CODE (ao) == FUNCTION_DECL)
>>> +    {
>>> +      if (!context->ictx.ao)
>>> +        context->ictx.ao = block;
>>> +
>>> +      location_t loc = BLOCK_SOURCE_LOCATION (block);
>>> +      context->ictx.ilocs.safe_push (loc);
>>> +      if (in_system_header_at (loc))
>>> +        ++nsyslocs;
>>> +    }
>>> +      else if (TREE_CODE (ao) != BLOCK)
>>> +    break;
>>> +
>>> +      block = BLOCK_SUPERCONTEXT (block);
>>> +    }
>>> +
>>> +  if (context->ictx.ilocs.length ())
>>> +    {
>>> +      /* When there is an inlining context use the macro expansion
>>> +     location for the original location and bump up NSYSLOCS if
>>> +     it's in a system header since it's not counted above.  */
>>> +      context->ictx.loc = 
>>> expansion_point_location_if_in_system_header (loc);
>>> +      if (context->ictx.loc != loc)
>>> +    ++nsyslocs;
>>> +    }
>>> +  else
>>> +    {
>>> +      /* When there's no inlining context use the original location
>>> +     and set NSYSLOCS accordingly.  */
>>> +      context->ictx.loc = loc;
>>> +      nsyslocs = in_system_header_at (loc) != 0;
>>> +    }
>>> +
>>> +  context->ictx.ilocs.safe_push (context->ictx.loc);
>>> +
>>> +  /* Set if all locations are in a system header.  */
>>> +  context->ictx.allsyslocs = nsyslocs == context->ictx.ilocs.length 
>>> ();;
>>
>> Surplus trailing semicolon.
>>
>> Maybe store nsyslocs as private member data ("m_nsyslocs"), and have an
>> all_system_locations_p accessor?  (since if I'm reading things right
>> that's the question that diagnostic_report_diagnostic is asking of the
>> inlining_info that we need this information for)
> 
> nsyslocs is only needed in this function.  The bit of data we need
> is context->ictx.allsyslocs (a bool).  I could make it private and
> add an accessor but as I said above I don't think that would gain
> us anything unless we also did that for all the other members of
> diagnostic_info as well.
> 
>>> +/* Set the inlining location for to the DIAGNOSTIC based on the saved
>>> +   inlining context.  */
>>> +
>>> +static void
>>> +set_inlining_location (const diagnostic_context *context,
>>> +               diagnostic_info *diagnostic)
>>> +{
>>> +  if (!pp_ti_abstract_origin (&diagnostic->message)
>>> +      || !context->ictx.ao
>>> +      || context->ictx.loc == UNKNOWN_LOCATION)
>>> +    /* Do nothing when there's no inlining context.  */
>>> +    return;
>>> +
>>> +  *pp_ti_abstract_origin (&diagnostic->message) = 
>>> (tree)context->ictx.ao;
>>> +  diagnostic->message.set_location (0, context->ictx.loc,
>>> +                    SHOW_RANGE_WITH_CARET);
>>> +}
>>> +
>>
>> If the inlining_info becomes a member of the diagnostic_info, does the
>> need for this "set" callback go away?
> 
> Yes, that's a good observation!  In the attached revision I was able
> to do away with this callback and simplify the whole implementation
> a bit thanks to it.
> 
>>
>>
>> [...snip...]
>>
>> Hope this is constructive and makes sense; thanks again for the patch
>> Dave
>>
> 
> Yep, thanks.  Please see the attached revision.
> 
> Martin