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

[David Malcolm <dmalcolm@redhat.com>]

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