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
