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

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.

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?

> 
> 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?

>    /* 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.

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)

Maybe rename it to "inlining_info"?

How involved would it be to make it be a class with private fields?

Can the field names have "m_" prefixes, please?

> +  /* 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).

> +  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)



> +/* 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?


[...snip...]

Hope this is constructive and makes sense; thanks again for the patch
Dave

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

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

On Tue, 2021-06-15 at 17:00 -0600, 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).
> 
> 

[...]

> > 
> 
> 
> Yep, thanks.  Please see the attached revision.
> 
> Martin

Various nits inline below:

> diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
> index d58586f2526..3a22d4d26a6 100644
> --- a/gcc/diagnostic.c
> +++ b/gcc/diagnostic.c
> @@ -991,51 +991,93 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc,
>    pp_set_prefix (pp, saved_prefix);
>  }
>  
> -/* Update the diag_class of DIAGNOSTIC based on its location
> -   relative to any
> +/* Update the inlininig context in CONTEXT for a DIAGNOSTIC.  */
                 ^^^^^^^^^^^^^^^^^

It's inlining_info now, so please update this comment.

> +
> +static void
> +update_inlining_context (diagnostic_context *context,
> +			 diagnostic_info *diagnostic)

...and please rename to "get_any_inlining_info".

> +{
> +  auto &ilocs = diagnostic->m_iinfo.m_ilocs;
> +
> +  if (context->set_locations_cb)
> +    {
> +      /* Retrieve the locations into which the expression about to be
> +	 diagnosed has been inlined, including those of all the callers
> +	 all the way down the inlining stack.  */
> +      context->set_locations_cb (context, diagnostic);
> +      location_t loc = diagnostic->m_iinfo.m_ilocs.last ();


> +      if (diagnostic->m_iinfo.m_ao && loc != UNKNOWN_LOCATION)
> +	diagnostic->message.set_location (0, loc, SHOW_RANGE_WITH_CARET);

What is the purpose of the above two lines of code?
(I believe it's to replace the %G/%K stuff, right?)
Please can you add a suitable comment.


> +    }
> +  else
> +    {
> +      /* When there's no callback use just the one location provided
> +	 by the caller of the diagnostic function.  */
> +      location_t loc = diagnostic_location (diagnostic);
> +      ilocs.safe_push (loc);
> +      diagnostic->m_iinfo.m_allsyslocs = in_system_header_at (loc);
> +    }
> +}
> +
> +/* Update the kind of DIAGNOSTIC based on its location(s), including
> +   any of those in its inlining context, relative to any
                                   ^^^^^^^

"stack" rather than "context" here; I think we're overusing the word "context".

>       #pragma GCC diagnostic
>     directives recorded within CONTEXT.
>  
> -   Return the new diag_class of DIAGNOSTIC if it was updated, or
> -   DK_UNSPECIFIED otherwise.  */
> +   Return the new kind of DIAGNOSTIC if it was updated, or DK_UNSPECIFIED
> +   otherwise.  */
>  
>  static diagnostic_t
>  update_effective_level_from_pragmas (diagnostic_context *context,
>  				     diagnostic_info *diagnostic)
>  {
> -  diagnostic_t diag_class = DK_UNSPECIFIED;
> -
> -  if (context->n_classification_history > 0)
> +  if (diagnostic->m_iinfo.m_allsyslocs && !context->dc_warn_system_headers)
>      {
> -      location_t location = diagnostic_location (diagnostic);
> +      /* Ignore the diagnostic if all the inlined locations are
> +	 in system headers and -Wno-system-headers is in effect.  */
> +      diagnostic->kind = DK_IGNORED;
> +      return DK_IGNORED;
> +    }
>  
> +  if (context->n_classification_history <= 0)
> +    return DK_UNSPECIFIED;
> +
> +  /* Iterate over the locations, checking the diagnostic disposition
> +     for the diagnostic at each.  If it's explicitly set as opposed
> +     to unspecified, update the disposition for this instance of
> +     the diagnostic and return it.  */
> +  for (location_t loc: diagnostic->m_iinfo.m_ilocs)
> +    {
>        /* FIXME: Stupid search.  Optimize later. */
>        for (int i = context->n_classification_history - 1; i >= 0; i --)
>  	{
> -	  if (linemap_location_before_p
> -	      (line_table,
> -	       context->classification_history[i].location,
> -	       location))
> +	  const diagnostic_classification_change_t &hist
> +	    = context->classification_history[i];
> +
> +	  location_t pragloc = hist.location;
> +	  if (!linemap_location_before_p (line_table, pragloc, loc))
> +	    continue;
> +
> +	  if (hist.kind == (int) DK_POP)
>  	    {
> -	      if (context->classification_history[i].kind == (int) DK_POP)
> -		{
> -		  i = context->classification_history[i].option;
> -		  continue;
> -		}
> -	      int option = context->classification_history[i].option;
> -	      /* The option 0 is for all the diagnostics.  */
> -	      if (option == 0 || option == diagnostic->option_index)
> -		{
> -		  diag_class = context->classification_history[i].kind;
> -		  if (diag_class != DK_UNSPECIFIED)
> -		    diagnostic->kind = diag_class;
> -		  break;
> -		}
> +	      /* Move on to the next region.  */
> +	      i = hist.option;
> +	      continue;
> +	    }
> +
> +	  int option = hist.option;
> +	  /* The option 0 is for all the diagnostics.  */
> +	  if (option == 0 || option == diagnostic->option_index)
> +	    {
> +	      diagnostic_t kind = hist.kind;
> +	      if (kind != DK_UNSPECIFIED)
> +		diagnostic->kind = kind;
> +	      return kind;
>  	    }
>  	}
>      }
>  
> -  return diag_class;
> +  return DK_UNSPECIFIED;
>  }
>  
>  /* Generate a URL string describing CWE.  The caller is responsible for
> @@ -1129,6 +1171,9 @@ static bool
>  diagnostic_enabled (diagnostic_context *context,
>  		    diagnostic_info *diagnostic)
>  {
> +  /* Update the inlining context for this diagnostic.  */
> +  update_inlining_context (context, diagnostic);

Please rename as described above.

[...]

Dave

On 6/30/21 4:55 PM, David Malcolm wrote:
> On Tue, 2021-06-15 at 17:00 -0600, 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).
>>
>>
> 
> [...]
> 
>>>
>>
>>
>> Yep, thanks.  Please see the attached revision.
>>
>> Martin
> 
> Various nits inline below:
> 
>> diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
>> index d58586f2526..3a22d4d26a6 100644
>> --- a/gcc/diagnostic.c
>> +++ b/gcc/diagnostic.c
>> @@ -991,51 +991,93 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc,
>>     pp_set_prefix (pp, saved_prefix);
>>   }
>>   
>> -/* Update the diag_class of DIAGNOSTIC based on its location
>> -   relative to any
>> +/* Update the inlininig context in CONTEXT for a DIAGNOSTIC.  */
>                   ^^^^^^^^^^^^^^^^^
> 
> It's inlining_info now, so please update this comment.

Inlining_info is the name of the struct (per your request) that
captures what's commonly referred to as the inlining context:
the context from which a function call is made and into which
it's inlined.  Another common name for it is inlining stack.
These are also the terms using which I documented the purpose
of the struct in diagnostic.h.  As far as I know no one refers
to it as the "info of a function call" and while it's not wrong
it per se I don't think it helps make the comment clearer.

I've made the requested naming changes against my better judgment.

> 
>> +
>> +static void
>> +update_inlining_context (diagnostic_context *context,
>> +			 diagnostic_info *diagnostic)
> 
> ...and please rename to "get_any_inlining_info".

Done (with the same reservation as above).

> 
>> +{
>> +  auto &ilocs = diagnostic->m_iinfo.m_ilocs;
>> +
>> +  if (context->set_locations_cb)
>> +    {
>> +      /* Retrieve the locations into which the expression about to be
>> +	 diagnosed has been inlined, including those of all the callers
>> +	 all the way down the inlining stack.  */
>> +      context->set_locations_cb (context, diagnostic);
>> +      location_t loc = diagnostic->m_iinfo.m_ilocs.last ();
> 
> 
>> +      if (diagnostic->m_iinfo.m_ao && loc != UNKNOWN_LOCATION)
>> +	diagnostic->message.set_location (0, loc, SHOW_RANGE_WITH_CARET);
> 
> What is the purpose of the above two lines of code?
> (I believe it's to replace the %G/%K stuff, right?)
> Please can you add a suitable comment.

The purpose of this code was to restore the caret in case it was
suppressed for a rich location by a call to add_range().  I didn't
note down the test that failed and that prompted me to add it (it's
what I was chasing down when I noticed the semi_embedded_vec and
rich_location bugs), but removing it now doesn't trigger any
failures anymore.

>> +    }
>> +  else
>> +    {
>> +      /* When there's no callback use just the one location provided
>> +	 by the caller of the diagnostic function.  */
>> +      location_t loc = diagnostic_location (diagnostic);
>> +      ilocs.safe_push (loc);
>> +      diagnostic->m_iinfo.m_allsyslocs = in_system_header_at (loc);
>> +    }
>> +}
>> +
>> +/* Update the kind of DIAGNOSTIC based on its location(s), including
>> +   any of those in its inlining context, relative to any
>                                     ^^^^^^^
> 
> "stack" rather than "context" here; I think we're overusing the word "context".

I don't foresee anyone getting confused by either but again, no
point in spending time arguing about it.  I used stack instead.

> 
...>>   /* Generate a URL string describing CWE.  The caller is 
responsible for
>> @@ -1129,6 +1171,9 @@ static bool
>>   diagnostic_enabled (diagnostic_context *context,
>>   		    diagnostic_info *diagnostic)
>>   {
>> +  /* Update the inlining context for this diagnostic.  */
>> +  update_inlining_context (context, diagnostic);
> 
> Please rename as described above.

Done.

The revised patch is in the attachment.  I plan to go with it unless
there are requests for code changes.

Martin