[PATCH 1/13] v2 [PATCH 1/13] Add support for per-location warning groups (PR 74765)
Thomas Schwinge
thomas@codesourcery.com
Wed Sep 1 19:35:42 GMT 2021
Hi!
On 2021-06-23T13:47:08-0600, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> On 6/22/21 5:28 PM, David Malcolm wrote:
>> On Tue, 2021-06-22 at 19:18 -0400, David Malcolm wrote:
>>> On Fri, 2021-06-04 at 15:41 -0600, Martin Sebor wrote:
>>>> The attached patch introduces the suppress_warning(),
>>>> warning_suppressed(), and copy_no_warning() APIs [etc.]
Martin, great work on this!
I was a bit surprised to see this key on 'location_t's -- but indeed it
appears to do the right thing.
I now had a bit of a deep dive into some aspects of this, in context of
<https://gcc.gnu.org/PR101574> "gcc/sparseset.h:215:20: error: suggest
parentheses around assignment used as truth value [-Werror=parentheses]"
that I recently filed. This seems difficult to reproduce, but I'm still
able to reliably reproduce it in one specific build
configuration/directory/machine/whatever. Initially, we all quickly
assumed that it'd be some GC issue -- but "alas", it's not, at least not
directly. (But I'll certainly assume that some GC aspects are involved
which make this issue come and go across different GCC sources revisions,
and difficult to reproduce.)
First, two pieces of cleanup:
> --- /dev/null
> +++ b/gcc/warning-control.cc
> +template <class ToType, class FromType>
> +void copy_warning (ToType to, FromType from)
> +{
> + const key_type_t to_key = convert_to_key (to);
> +
> + if (nowarn_spec_t *from_map = get_nowarn_spec (from))
> + {
> + /* If there's an entry in the map the no-warning bit must be set. */
> + gcc_assert (get_no_warning_bit (from));
> +
> + if (!nowarn_map)
> + nowarn_map = xint_hash_map_t::create_ggc (32);
OK to push "Simplify 'gcc/diagnostic-spec.h:nowarn_map' setup", see
attached? If we've just read something from the map, we can be sure that
it exists. ;-)
> --- /dev/null
> +++ b/gcc/diagnostic-spec.h
> +typedef location_t key_type_t;
> +typedef int_hash <key_type_t, 0, UINT_MAX> xint_hash_t;
> +typedef hash_map<xint_hash_t, nowarn_spec_t> xint_hash_map_t;
> +
> +/* A mapping from the location of an expression to the warning spec
> + set for it. */
> +extern GTY(()) xint_hash_map_t *nowarn_map;
More on that data structure setup in a later email; here I'd like to
"Clarify 'key_type_t' to 'location_t' as used for
'gcc/diagnostic-spec.h:nowarn_map'", see attached. OK to push? To make
it obvious what exactly the key type is. No change in behavior.
Why is this relevant? Via current 'int_hash<key_type_t, 0, UINT_MAX>',
we create a 'int_hash' using "spare" value '0' for 'Empty' marker, and
"spare" value 'UINT_MAX' for 'Deleted' marker. Now, the latter is
unlikely to ever trigger (but still not correct -- patch in testing), but
the former triggers very much so: value '0' is, per 'gcc/input.h':
#define UNKNOWN_LOCATION ((location_t) 0)
..., and there are no safe-guards in the code here, so we'll happily put
key 'UNKNOWN_LOCATION' into the 'nowarn_map', and all the
'UNKNOWN_LOCATION' entries share (replace?) one single warning
disposition (problem!), and at the same time that key value is also used
as the 'Empty' marker (problem!). I have not tried to understand why
this doesn't cause much greater breakage, but propose to fix this as per
the attached "Don't maintain a warning spec for
'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' [PR101574]". OK to push?
Leaving aside that for 'UNKNOWN_LOCATION' -- per my understanding, at
least, as per above -- the current implementation isn't doing the right
thing anyway, Richard had in
<http://mid.mail-archive.com/CAFiYyc2am7_pcN4+dvgFSxumnQzYuNRefFWf-zDHiJv-n8EA_w@mail.gmail.com>
toyed with the idea that we for "UNKNOWN_LOCATION create a new location
with the source location being still UNKNOWN but with the appropriate
ad-hoc data to disable the warning". On the other hand, we have Martin's
initial goal,
<http://mid.mail-archive.com/92db3776-af59-fa20-483b-aa67b17d0751@gmail.com>,
that he'd like to "work toward providing locations for all
expressions/statements". (I agree -- and get rid of "location wrapper"
nodes at the same time...) So there certainly is follow-on work to be
done re 'UNKNOWN_LOCATION's, but that's orthogonal to the issue I'm
fixing here. (Just mentioning all this for context.)
I'm reasonably confident that my changes are doing the right things in
general, but please carefully review, especially here:
- 'gcc/warning-control.cc:suppress_warning' functions: is it correct to
conditionalize on '!RESERVED_LOCATION_P' the 'suppress_warning_at'
calls and 'supp' update? Or, should instead 'suppress_warning_at'
handle the case of '!RESERVED_LOCATION_P'? (How?)
- 'gcc/diagnostic-spec.c:copy_warning' and
'gcc/warning-control.cc:copy_warning': is the rationale correct for
the 'gcc_checking_assert (!from_spec)': "If we cannot set no-warning
dispositions for 'to', ascertain that we don't have any for 'from'.
Otherwise, we'd lose these."? If the rationale is correct, then
observing that in 'gcc/warning-control.cc:copy_warning' this
currently "triggers during GCC build" is something to be looked into,
later, I suppose, and otherwise, how should I change this code?
Gating on 'RESERVED_LOCATION_P' is what other similar code in GCC is
doing. Conveniently, that frees up the two values
'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' for use as 'Empty'/'Deleted'
markers -- patch in testing.
Plus some more cleanup that fell out during analysis/development --
patches in testing.
Oh, and one of these actually (unintentially so) happens to resolve
<https://gcc.gnu.org/PR101204> "[12 Regression] infinite recursion in
gtype-desc.c since r12-1801-g7036e9ef462fde8181bece4ac4e03f3aa27204dc",
so unless you've done any work on that, may I take over that PR?
What my patches are not addressing is <https://gcc.gnu.org/PR101292>
"[12 Regression] recent valgrind error in warning-control.cc since
r12-1804-g65870e75616ee4359d1c13b99be794e6a577bc65", and with the
'gcc_assert (*from_map != 0xa5a5a5a5);' I'm actually able to trigger
that during GCC build itself.
Are you aware of any other PRs related to this functionality that I
should re-evaluate?
Grüße
Thomas
PS. Relevant code quoted for reference, in case that's useful:
> --- /dev/null
> +++ b/gcc/diagnostic-spec.h
> [...]
> +typedef location_t key_type_t;
> +typedef int_hash <key_type_t, 0, UINT_MAX> xint_hash_t;
> +typedef hash_map<xint_hash_t, nowarn_spec_t> xint_hash_map_t;
> +
> +/* A mapping from the location of an expression to the warning spec
> + set for it. */
> +extern GTY(()) xint_hash_map_t *nowarn_map;
> [...]
> --- /dev/null
> +++ b/gcc/diagnostic-spec.c
> [...]
> +/* Map from location to its no-warning disposition. */
> +
> +GTY(()) xint_hash_map_t *nowarn_map;
> +
> +/* Return the no-warning disposition for location LOC and option OPT
> + or for all/any otions by default. */
> +
> +bool
> +warning_suppressed_at (location_t loc, opt_code opt /* = all_warnings */)
> +{
> + if (!nowarn_map)
> + return false;
> +
> + if (const nowarn_spec_t* const pspec = nowarn_map->get (loc))
> + {
> + const nowarn_spec_t optspec (opt);
> + return *pspec & optspec;
> + }
> +
> + return false;
> +}
> +
> + /* Change the supression of warnings at location LOC.
> + OPT controls which warnings are affected.
> + The wildcard OPT of -1 controls all warnings.
> + If SUPP is true (the default), enable the suppression of the warnings.
> + If SUPP is false, disable the suppression of the warnings. */
> +
> +bool
> +suppress_warning_at (location_t loc, opt_code opt /* = all_warnings */,
> + bool supp /* = true */)
> +{
> + const nowarn_spec_t optspec (supp ? opt : opt_code ());
> +
> + if (nowarn_spec_t *pspec = nowarn_map ? nowarn_map->get (loc) : NULL)
> + {
> + if (supp)
> + {
> + *pspec |= optspec;
> + return true;
> + }
> +
> + *pspec &= optspec;
> + if (*pspec)
> + return true;
> +
> + nowarn_map->remove (loc);
> + return false;
> + }
> +
> + if (!supp || opt == no_warning)
> + return false;
> +
> + if (!nowarn_map)
> + nowarn_map = xint_hash_map_t::create_ggc (32);
> +
> + nowarn_map->put (loc, optspec);
> + return true;
> +}
> +
> +/* Copy the no-warning disposition from one location to another. */
> +
> +void
> +copy_warning (location_t to, location_t from)
> +{
> + if (!nowarn_map)
> + return;
> +
> + if (nowarn_spec_t *pspec = nowarn_map->get (from))
> + nowarn_map->put (to, *pspec);
> + else
> + nowarn_map->remove (to);
> +}
> --- /dev/null
> +++ b/gcc/warning-control.cc
> [...]
> +/* Return the no-warning bit for EXPR. */
> +
> +static inline bool
> +get_no_warning_bit (const_tree expr)
> +{
> + return expr->base.nowarning_flag;
> +}
> +
> +/* Return the no-warning bit for statement STMT. */
> +
> +static inline bool
> +get_no_warning_bit (const gimple *stmt)
> +{
> + return stmt->no_warning;
> +}
> +
> +/* Set the no-warning bit for EXPR to VALUE. */
> +
> +static inline void
> +set_no_warning_bit (tree expr, bool value)
> +{
> + expr->base.nowarning_flag = value;
> +}
> +
> +/* Set the no-warning bit for statement STMT to VALUE. */
> +
> +static inline void
> +set_no_warning_bit (gimple *stmt, bool value)
> +{
> + stmt->no_warning = value;
> +}
> +
> +/* Return EXPR location or zero. */
> +
> +static inline key_type_t
> +convert_to_key (const_tree expr)
> +{
> + if (DECL_P (expr))
> + return DECL_SOURCE_LOCATION (expr);
> + if (EXPR_P (expr))
> + return EXPR_LOCATION (expr);
> + return 0;
> +}
> +
> +/* Return STMT location (may be zero). */
> +
> +static inline key_type_t
> +convert_to_key (const gimple *stmt)
> +{
> + return gimple_location (stmt);
> +}
> +
> +/* Return the no-warning bitmap for decl/expression EXPR. */
> +
> +static nowarn_spec_t *
> +get_nowarn_spec (const_tree expr)
> +{
> + const key_type_t key = convert_to_key (expr);
> +
> + if (!get_no_warning_bit (expr) || !key)
> + return NULL;
> +
> + return nowarn_map ? nowarn_map->get (key) : NULL;
> +}
> +
> +/* Return the no-warning bitmap for stateemt STMT. */
> +
> +static nowarn_spec_t *
> +get_nowarn_spec (const gimple *stmt)
> +{
> + const key_type_t key = convert_to_key (stmt);
> +
> + if (!get_no_warning_bit (stmt))
> + return NULL;
> +
> + return nowarn_map ? nowarn_map->get (key) : NULL;
> +}
> +
> +/* Return true if warning OPT is suppressed for decl/expression EXPR.
> + By default tests the disposition for any warning. */
> +
> +bool
> +warning_suppressed_p (const_tree expr, opt_code opt /* = all_warnings */)
> +{
> + const nowarn_spec_t *spec = get_nowarn_spec (expr);
> +
> + if (!spec)
> + return get_no_warning_bit (expr);
> +
> + const nowarn_spec_t optspec (opt);
> + bool dis = *spec & optspec;
> + gcc_assert (get_no_warning_bit (expr) || !dis);
> + return dis;
> +}
> +
> +/* Return true if warning OPT is suppressed for statement STMT.
> + By default tests the disposition for any warning. */
> +
> +bool
> +warning_suppressed_p (const gimple *stmt, opt_code opt /* = all_warnings */)
> +{
> + const nowarn_spec_t *spec = get_nowarn_spec (stmt);
> +
> + if (!spec)
> + /* Fall back on the single no-warning bit. */
> + return get_no_warning_bit (stmt);
> +
> + const nowarn_spec_t optspec (opt);
> + bool dis = *spec & optspec;
> + gcc_assert (get_no_warning_bit (stmt) || !dis);
> + return dis;
> +}
> +
> +/* Enable, or by default disable, a warning for the expression.
> + The wildcard OPT of -1 controls all warnings. */
> +
> +void
> +suppress_warning (tree expr, opt_code opt /* = all_warnings */,
> + bool supp /* = true */)
> +{
> + if (opt == no_warning)
> + return;
> +
> + const key_type_t key = convert_to_key (expr);
> +
> + supp = suppress_warning_at (key, opt, supp) || supp;
> + set_no_warning_bit (expr, supp);
> +}
> +
> +/* Enable, or by default disable, a warning for the statement STMT.
> + The wildcard OPT of -1 controls all warnings. */
> +
> +void
> +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
> + bool supp /* = true */)
> +{
> + if (opt == no_warning)
> + return;
> +
> + const key_type_t key = convert_to_key (stmt);
> +
> + supp = suppress_warning_at (key, opt, supp) || supp;
> + set_no_warning_bit (stmt, supp);
> +}
> +
> +/* Copy the warning disposition mapping between an expression and/or
> + a statement. */
> +
> +template <class ToType, class FromType>
> +void copy_warning (ToType to, FromType from)
> +{
> + const key_type_t to_key = convert_to_key (to);
> +
> + if (nowarn_spec_t *from_map = get_nowarn_spec (from))
> + {
> + /* If there's an entry in the map the no-warning bit must be set. */
> + gcc_assert (get_no_warning_bit (from));
> +
> + if (!nowarn_map)
> + nowarn_map = xint_hash_map_t::create_ggc (32);
> +
> + nowarn_map->put (to_key, *from_map);
> + set_no_warning_bit (to, true);
> + }
> + else
> + {
> + if (nowarn_map)
> + nowarn_map->remove (to_key);
> +
> + /* The no-warning bit might be set even if there's no entry
> + in the map. */
> + set_no_warning_bit (to, get_no_warning_bit (from));
> + }
> +}
> +
> +/* Copy the warning disposition mapping from one expression to another. */
> +
> +void
> +copy_warning (tree to, const_tree from)
> +{
> + copy_warning<tree, const_tree>(to, from);
> +}
> +
> +/* Copy the warning disposition mapping from a statement to an expression. */
> +
> +void
> +copy_warning (tree to, const gimple *from)
> +{
> + copy_warning<tree, const gimple *>(to, from);
> +}
> +
> +/* Copy the warning disposition mapping from an expression to a statement. */
> +
> +void
> +copy_warning (gimple *to, const_tree from)
> +{
> + copy_warning<gimple *, const_tree>(to, from);
> +}
> +
> +/* Copy the warning disposition mapping from one statement to another. */
> +
> +void
> +copy_warning (gimple *to, const gimple *from)
> +{
> + copy_warning<gimple *, const gimple *>(to, from);
> +}
Grüße
Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Simplify-gcc-diagnostic-spec.h-nowarn_map-setup.patch
Type: text/x-diff
Size: 1026 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210901/17051247/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Clarify-key_type_t-to-location_t-as-used-for-gcc-dia.patch
Type: text/x-diff
Size: 5440 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210901/17051247/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Don-t-maintain-a-warning-spec-for-UNKNOWN_LOCATION-B.patch
Type: text/x-diff
Size: 5857 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210901/17051247/attachment-0005.bin>
More information about the Gcc-patches
mailing list