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

Message ID e96b8441-a02f-8c0d-24b5-0a7208c8bf9a@gmail.com
State New
Headers show
Series
  • improve warning suppression for inlined functions (PR 98512)
Related show

Commit Message

Antoni Boucher via Gcc-patches June 10, 2021, 11:26 p.m.
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.

Comments

Antoni Boucher via Gcc-patches June 11, 2021, 5:04 p.m. | #1
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
Antoni Boucher via Gcc-patches June 15, 2021, 11 p.m. | #2
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
Improve warning suppression for inlined functions [PR 98512].

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

gcc/ChangeLog:

	* diagnostic.c (update_inlining_context): New.
	(update_effective_level_from_pragmas): Handle inlining context.
	(diagnostic_enabled): Same.
	(diagnostic_report_diagnostic): Same.
	* diagnostic.h (struct diagnostic_info): Add ctor.
	(struct diagnostic_context): Add new member.
	* tree-diagnostic.c (set_inlining_locations): New.
	(tree_diagnostics_defaults): Set new callback pointer.

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.  */
+
+static void
+update_inlining_context (diagnostic_context *context,
+			 diagnostic_info *diagnostic)
+{
+  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);
+    }
+  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
      #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);
+
   /* Diagnostics with no option or -fpermissive are always enabled.  */
   if (!diagnostic->option_index
       || diagnostic->option_index == permissive_error_option (context))
@@ -1194,9 +1239,17 @@ diagnostic_report_diagnostic (diagnostic_context *context,
 
   /* Give preference to being able to inhibit warnings, before they
      get reclassified to something else.  */
-  if ((diagnostic->kind == DK_WARNING || diagnostic->kind == DK_PEDWARN)
-      && !diagnostic_report_warnings_p (context, location))
-    return false;
+  bool report_warning_p = true;
+  if (diagnostic->kind == DK_WARNING || diagnostic->kind == DK_PEDWARN)
+    {
+      if (context->dc_inhibit_warnings)
+	return false;
+      /* Remember the result of the overall system header warning setting
+	 but proceed to also check the inlining context.  */
+      report_warning_p = diagnostic_report_warnings_p (context, location);
+      if (!report_warning_p && diagnostic->kind == DK_PEDWARN)
+	return false;
+    }
 
   if (diagnostic->kind == DK_PEDWARN)
     {
@@ -1204,7 +1257,7 @@ diagnostic_report_diagnostic (diagnostic_context *context,
       /* We do this to avoid giving the message for -pedantic-errors.  */
       orig_diag_kind = diagnostic->kind;
     }
- 
+
   if (diagnostic->kind == DK_NOTE && context->inhibit_notes_p)
     return false;
 
@@ -1228,9 +1281,19 @@ diagnostic_report_diagnostic (diagnostic_context *context,
       && diagnostic->kind == DK_WARNING)
     diagnostic->kind = DK_ERROR;
 
+  diagnostic->message.x_data = &diagnostic->x_data;
+
+  /* Check to see if the diagnostic is enabled at the location and
+     not disabled by #pragma GCC diagnostic anywhere along the inlining
+     stack.  .  */
   if (!diagnostic_enabled (context, diagnostic))
     return false;
 
+  if (!report_warning_p && diagnostic->m_iinfo.m_allsyslocs)
+    /* Bail if the warning is not to be reported because all locations
+       in the inlining stack (if there is one) are in system headers.  */
+    return false;
+
   if (diagnostic->kind != DK_NOTE && diagnostic->kind != DK_ICE)
     diagnostic_check_max_errors (context);
 
@@ -1270,8 +1333,6 @@ diagnostic_report_diagnostic (diagnostic_context *context,
     }
   context->diagnostic_group_emission_count++;
 
-  diagnostic->message.x_data = &diagnostic->x_data;
-  diagnostic->x_data = NULL;
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 1b9d6b1f64d..9d02615fb7a 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -87,6 +87,11 @@ enum diagnostics_extra_output_kind
    list in diagnostic.def.  */
 struct diagnostic_info
 {
+  diagnostic_info ()
+    : message (), richloc (), metadata (), x_data (), kind (), option_index (),
+      m_iinfo ()
+  { }
+
   /* Text to be formatted.  */
   text_info message;
 
@@ -103,6 +108,18 @@ struct diagnostic_info
   diagnostic_t kind;
   /* Which OPT_* directly controls this diagnostic.  */
   int option_index;
+
+  /* Inlining context containing locations for each call site along
+     the inlining stack.  */
+  struct inlining_info
+  {
+    /* Locations along the inlining stack.  */
+    auto_vec<location_t, 8> m_ilocs;
+    /* The abstract origin of the location.  */
+    void *m_ao;
+    /* Set if every M_ILOCS element is in a system header.  */
+    bool m_allsyslocs;
+  } m_iinfo;
 };
 
 /* Each time a diagnostic's classification is changed with a pragma,
@@ -343,6 +360,12 @@ struct diagnostic_context
 
   /* Callback for final cleanup.  */
   void (*final_cb) (diagnostic_context *context);
+
+  /* Callback to set the locations of call sites along the inlining
+     stack corresponding to a diagnostic location.  Needed to traverse
+     the BLOCK_SUPERCONTEXT() chain hanging off the LOCATION_BLOCK()
+     of a diagnostic's location.  */
+  void (*set_locations_cb)(diagnostic_context *, diagnostic_info *);
 };
 
 static inline void
diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
index 95b8ef30070..1b636d72a29 100644
--- a/gcc/tree-diagnostic.c
+++ b/gcc/tree-diagnostic.c
@@ -305,6 +305,73 @@ default_tree_printer (pretty_printer *pp, text_info *text, const char *spec,
   return true;
 }
 
+/* Set the locations of call sites along the inlining stack corresponding
+   to the DIAGNOSTIC location.  */
+
+static void
+set_inlining_locations (diagnostic_context *,
+			diagnostic_info *diagnostic)
+{
+  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;
+
+  /* Use a reference to the vector of locations for convenience.  */
+  auto &ilocs = diagnostic->m_iinfo.m_ilocs;
+
+  while (block && TREE_CODE (block) == BLOCK
+	 && BLOCK_ABSTRACT_ORIGIN (block))
+    {
+      tree ao = BLOCK_ABSTRACT_ORIGIN (block);
+      if (TREE_CODE (ao) == FUNCTION_DECL)
+	{
+	  if (!diagnostic->m_iinfo.m_ao)
+	    diagnostic->m_iinfo.m_ao = block;
+
+	  location_t bsloc = BLOCK_SOURCE_LOCATION (block);
+	  ilocs.safe_push (bsloc);
+	  if (in_system_header_at (bsloc))
+	    ++nsyslocs;
+	}
+      else if (TREE_CODE (ao) != BLOCK)
+	break;
+
+      block = BLOCK_SUPERCONTEXT (block);
+    }
+
+  if (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.  */
+      location_t sysloc = expansion_point_location_if_in_system_header (loc);
+      if (sysloc != loc)
+	{
+	  loc = sysloc;
+	  ++nsyslocs;
+	}
+    }
+  else
+    {
+      /* When there's no inlining context use the original location
+	 and set NSYSLOCS accordingly.  */
+      nsyslocs = in_system_header_at (loc) != 0;
+    }
+
+  ilocs.safe_push (loc);
+
+  /* Set if all locations are in a system header.  */
+  diagnostic->m_iinfo.m_allsyslocs = nsyslocs == ilocs.length ();
+
+  if (tree *ao = pp_ti_abstract_origin (&diagnostic->message))
+    *ao = (tree)diagnostic->m_iinfo.m_ao;
+}
+
 /* Sets CONTEXT to use language independent diagnostics.  */
 void
 tree_diagnostics_defaults (diagnostic_context *context)
@@ -314,4 +381,5 @@ tree_diagnostics_defaults (diagnostic_context *context)
   diagnostic_format_decoder (context) = default_tree_printer;
   context->print_path = default_tree_diagnostic_path_printer;
   context->make_json_for_path = default_tree_make_json_for_path;
+  context->set_locations_cb = set_inlining_locations;
 }
Antoni Boucher via Gcc-patches June 28, 2021, 6:10 p.m. | #3
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
Antoni Boucher via Gcc-patches June 30, 2021, 10:55 p.m. | #4
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
Antoni Boucher via Gcc-patches July 1, 2021, 7:43 p.m. | #5
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
Improve warning suppression for inlined functions [PR 98512].

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

gcc/ChangeLog:

	* diagnostic.c (get_any_inlining_info): New.
	(update_effective_level_from_pragmas): Handle inlining context.
	(diagnostic_enabled): Same.
	(diagnostic_report_diagnostic): Same.
	* diagnostic.h (struct diagnostic_info): Add ctor.
	(struct diagnostic_context): Add new member.
	* tree-diagnostic.c (set_inlining_locations): New.
	(tree_diagnostics_defaults): Set new callback pointer.

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index d58586f2526..8361f68aace 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -991,51 +991,88 @@ 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 inlining info in CONTEXT for a DIAGNOSTIC.  */
+
+static void
+get_any_inlining_info (diagnostic_context *context,
+		       diagnostic_info *diagnostic)
+{
+  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);
+  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 stack, relative to any
      #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 +1166,9 @@ static bool
 diagnostic_enabled (diagnostic_context *context,
 		    diagnostic_info *diagnostic)
 {
+  /* Update the inlining stack for this diagnostic.  */
+  get_any_inlining_info (context, diagnostic);
+
   /* Diagnostics with no option or -fpermissive are always enabled.  */
   if (!diagnostic->option_index
       || diagnostic->option_index == permissive_error_option (context))
@@ -1194,9 +1234,17 @@ diagnostic_report_diagnostic (diagnostic_context *context,
 
   /* Give preference to being able to inhibit warnings, before they
      get reclassified to something else.  */
-  if ((diagnostic->kind == DK_WARNING || diagnostic->kind == DK_PEDWARN)
-      && !diagnostic_report_warnings_p (context, location))
-    return false;
+  bool report_warning_p = true;
+  if (diagnostic->kind == DK_WARNING || diagnostic->kind == DK_PEDWARN)
+    {
+      if (context->dc_inhibit_warnings)
+	return false;
+      /* Remember the result of the overall system header warning setting
+	 but proceed to also check the inlining context.  */
+      report_warning_p = diagnostic_report_warnings_p (context, location);
+      if (!report_warning_p && diagnostic->kind == DK_PEDWARN)
+	return false;
+    }
 
   if (diagnostic->kind == DK_PEDWARN)
     {
@@ -1204,7 +1252,7 @@ diagnostic_report_diagnostic (diagnostic_context *context,
       /* We do this to avoid giving the message for -pedantic-errors.  */
       orig_diag_kind = diagnostic->kind;
     }
- 
+
   if (diagnostic->kind == DK_NOTE && context->inhibit_notes_p)
     return false;
 
@@ -1228,9 +1276,19 @@ diagnostic_report_diagnostic (diagnostic_context *context,
       && diagnostic->kind == DK_WARNING)
     diagnostic->kind = DK_ERROR;
 
+  diagnostic->message.x_data = &diagnostic->x_data;
+
+  /* Check to see if the diagnostic is enabled at the location and
+     not disabled by #pragma GCC diagnostic anywhere along the inlining
+     stack.  .  */
   if (!diagnostic_enabled (context, diagnostic))
     return false;
 
+  if (!report_warning_p && diagnostic->m_iinfo.m_allsyslocs)
+    /* Bail if the warning is not to be reported because all locations
+       in the inlining stack (if there is one) are in system headers.  */
+    return false;
+
   if (diagnostic->kind != DK_NOTE && diagnostic->kind != DK_ICE)
     diagnostic_check_max_errors (context);
 
@@ -1270,8 +1328,6 @@ diagnostic_report_diagnostic (diagnostic_context *context,
     }
   context->diagnostic_group_emission_count++;
 
-  diagnostic->message.x_data = &diagnostic->x_data;
-  diagnostic->x_data = NULL;
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 1b9d6b1f64d..9d02615fb7a 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -87,6 +87,11 @@ enum diagnostics_extra_output_kind
    list in diagnostic.def.  */
 struct diagnostic_info
 {
+  diagnostic_info ()
+    : message (), richloc (), metadata (), x_data (), kind (), option_index (),
+      m_iinfo ()
+  { }
+
   /* Text to be formatted.  */
   text_info message;
 
@@ -103,6 +108,18 @@ struct diagnostic_info
   diagnostic_t kind;
   /* Which OPT_* directly controls this diagnostic.  */
   int option_index;
+
+  /* Inlining context containing locations for each call site along
+     the inlining stack.  */
+  struct inlining_info
+  {
+    /* Locations along the inlining stack.  */
+    auto_vec<location_t, 8> m_ilocs;
+    /* The abstract origin of the location.  */
+    void *m_ao;
+    /* Set if every M_ILOCS element is in a system header.  */
+    bool m_allsyslocs;
+  } m_iinfo;
 };
 
 /* Each time a diagnostic's classification is changed with a pragma,
@@ -343,6 +360,12 @@ struct diagnostic_context
 
   /* Callback for final cleanup.  */
   void (*final_cb) (diagnostic_context *context);
+
+  /* Callback to set the locations of call sites along the inlining
+     stack corresponding to a diagnostic location.  Needed to traverse
+     the BLOCK_SUPERCONTEXT() chain hanging off the LOCATION_BLOCK()
+     of a diagnostic's location.  */
+  void (*set_locations_cb)(diagnostic_context *, diagnostic_info *);
 };
 
 static inline void
diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c
index 95b8ef30070..1b636d72a29 100644
--- a/gcc/tree-diagnostic.c
+++ b/gcc/tree-diagnostic.c
@@ -305,6 +305,73 @@ default_tree_printer (pretty_printer *pp, text_info *text, const char *spec,
   return true;
 }
 
+/* Set the locations of call sites along the inlining stack corresponding
+   to the DIAGNOSTIC location.  */
+
+static void
+set_inlining_locations (diagnostic_context *,
+			diagnostic_info *diagnostic)
+{
+  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;
+
+  /* Use a reference to the vector of locations for convenience.  */
+  auto &ilocs = diagnostic->m_iinfo.m_ilocs;
+
+  while (block && TREE_CODE (block) == BLOCK
+	 && BLOCK_ABSTRACT_ORIGIN (block))
+    {
+      tree ao = BLOCK_ABSTRACT_ORIGIN (block);
+      if (TREE_CODE (ao) == FUNCTION_DECL)
+	{
+	  if (!diagnostic->m_iinfo.m_ao)
+	    diagnostic->m_iinfo.m_ao = block;
+
+	  location_t bsloc = BLOCK_SOURCE_LOCATION (block);
+	  ilocs.safe_push (bsloc);
+	  if (in_system_header_at (bsloc))
+	    ++nsyslocs;
+	}
+      else if (TREE_CODE (ao) != BLOCK)
+	break;
+
+      block = BLOCK_SUPERCONTEXT (block);
+    }
+
+  if (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.  */
+      location_t sysloc = expansion_point_location_if_in_system_header (loc);
+      if (sysloc != loc)
+	{
+	  loc = sysloc;
+	  ++nsyslocs;
+	}
+    }
+  else
+    {
+      /* When there's no inlining context use the original location
+	 and set NSYSLOCS accordingly.  */
+      nsyslocs = in_system_header_at (loc) != 0;
+    }
+
+  ilocs.safe_push (loc);
+
+  /* Set if all locations are in a system header.  */
+  diagnostic->m_iinfo.m_allsyslocs = nsyslocs == ilocs.length ();
+
+  if (tree *ao = pp_ti_abstract_origin (&diagnostic->message))
+    *ao = (tree)diagnostic->m_iinfo.m_ao;
+}
+
 /* Sets CONTEXT to use language independent diagnostics.  */
 void
 tree_diagnostics_defaults (diagnostic_context *context)
@@ -314,4 +381,5 @@ tree_diagnostics_defaults (diagnostic_context *context)
   diagnostic_format_decoder (context) = default_tree_printer;
   context->print_path = default_tree_diagnostic_path_printer;
   context->make_json_for_path = default_tree_make_json_for_path;
+  context->set_locations_cb = set_inlining_locations;
 }

Patch

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

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.

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index d58586f2526..d1c8c655f7a 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -991,51 +991,92 @@  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.  */
+
+static void
+update_inlining_context (diagnostic_context *context,
+			 diagnostic_info *diagnostic)
+{
+  context->ictx.reset ();
+
+  auto &ilocs = context->ictx.ilocs;
+
+  if (context->get_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->get_locations_cb (context, diagnostic);
+  else
+    {
+      /* When there's no metadata use just the one location provided
+	 by the caller of the diagnostic function.  */
+      location_t loc = diagnostic_location (diagnostic);
+      ilocs.safe_push (loc);
+      context->ictx.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
      #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 (context->ictx.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;
+
+  auto &ilocs = context->ictx.ilocs;
 
+  /* 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 (unsigned idx = 0; idx < ilocs.length (); ++idx)
+    {
       /* 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, ilocs[idx]))
+	    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 +1170,9 @@  static bool
 diagnostic_enabled (diagnostic_context *context,
 		    diagnostic_info *diagnostic)
 {
+  /* Update the inlining context for this diagnostic.  */
+  update_inlining_context (context, diagnostic);
+
   /* Diagnostics with no option or -fpermissive are always enabled.  */
   if (!diagnostic->option_index
       || diagnostic->option_index == permissive_error_option (context))
@@ -1194,9 +1238,17 @@  diagnostic_report_diagnostic (diagnostic_context *context,
 
   /* Give preference to being able to inhibit warnings, before they
      get reclassified to something else.  */
-  if ((diagnostic->kind == DK_WARNING || diagnostic->kind == DK_PEDWARN)
-      && !diagnostic_report_warnings_p (context, location))
-    return false;
+  bool report_warning_p = true;
+  if (diagnostic->kind == DK_WARNING || diagnostic->kind == DK_PEDWARN)
+    {
+      if (context->dc_inhibit_warnings)
+	return false;
+      /* Remember the result of the overall system header warning setting
+	 but proceed to also check the inlining context.  */
+      report_warning_p = diagnostic_report_warnings_p (context, location);
+      if (!report_warning_p && diagnostic->kind == DK_PEDWARN)
+	return false;
+    }
 
   if (diagnostic->kind == DK_PEDWARN)
     {
@@ -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;
     }
- 
+
   if (diagnostic->kind == DK_NOTE && context->inhibit_notes_p)
     return false;
 
@@ -1228,9 +1280,17 @@  diagnostic_report_diagnostic (diagnostic_context *context,
       && diagnostic->kind == DK_WARNING)
     diagnostic->kind = DK_ERROR;
 
+  /* Check to see if the diagnostic is enabled at the location and
+     not disabled by #pragma GCC diagnostic anywhere along the inlining
+     stack.  .  */
   if (!diagnostic_enabled (context, diagnostic))
     return false;
 
+  if (!report_warning_p && context->ictx.allsyslocs)
+    /* Bail if the warning is not to be reported because all locations
+       in the inlining stack (if there is one) are in system headers.  */
+    return false;
+
   if (diagnostic->kind != DK_NOTE && diagnostic->kind != DK_ICE)
     diagnostic_check_max_errors (context);
 
@@ -1270,8 +1330,14 @@  diagnostic_report_diagnostic (diagnostic_context *context,
     }
   context->diagnostic_group_emission_count++;
 
+  /* Move X_DATA into DIAGNOSTIC->MESSAGE before setting inlining context
+     abstract origin and location.  It uses X_DATA.  */
   diagnostic->message.x_data = &diagnostic->x_data;
   diagnostic->x_data = NULL;
+
+  if (context->set_location_cb)
+    context->set_location_cb (context, diagnostic);
+
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
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 ()
+  { }
+
   /* 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;
+
+  /* Callbacks to get and set the inlining context.  */
+  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 ();;
+}
+
+/* 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);
+}
+
 /* Sets CONTEXT to use language independent diagnostics.  */
 void
 tree_diagnostics_defaults (diagnostic_context *context)
@@ -314,4 +392,6 @@  tree_diagnostics_defaults (diagnostic_context *context)
   diagnostic_format_decoder (context) = default_tree_printer;
   context->print_path = default_tree_diagnostic_path_printer;
   context->make_json_for_path = default_tree_make_json_for_path;
+  context->get_locations_cb = get_inlining_locations;
+  context->set_location_cb = set_inlining_location;
 }