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

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9280 bytes --]

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

[-- Attachment #2: gcc-98512-1.diff --]
[-- Type: text/x-patch, Size: 11926 bytes --]

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;
 }