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
