Re: [PATCH 1/13] v2 [PATCH 1/13] Add support for per-location warning groups (PR 74765)

[Martin Sebor <msebor@gmail.com>]

On 9/1/21 1:35 PM, Thomas Schwinge wrote:
> 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.  ;-)

Cleanup is definitely okay by me.  I can't formally approve anything
but this looks clearly correct and an improvement.

> 
>> --- /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.

That's fine with me too and also like worthwhile cleanup.

FWIW, I used different key_type_t while prototyping the solution but
now that we've settled on location_t I see no reason not to use it
directly.

By the way, it seems we should probably also use a manifest constant
for Empty (probably UNKNOWN_LOCATION since we're reserving it).

> 
> 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?

You're right that all expressions/statements with no location end
up sharing the same entry in the map when one is written to.
The entry should never be read from for expressions or statements
(they should fall back on their no-warning bit).  The entry should
only be read by a call to warning_suppressed_at(loc, ...).
The Empty problem aside, I would think it's reasonable to return
the union of all suppressions for location zero (or for the decls
of all builtins).  But I never tested this (I'm not really sure
how), and I forgot to consider that UNKNOWN_LOCATION has the same
value as Empty.  So I agree that it ought to be fixed.

> 
> 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?)

It seems like six of one vs half a dozen of the other.  I'd say go
with whatever makes more sense to you here :)

> 
>    - '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?

copy_warning(location_t, location_t) is called [only] from
gimple_set_location().  The middle end does clear the location of
some statements for which it was previously valid (e.g., return
statements).  So I wouldn't expect this assumption to be safe.  If
that happens, we have no choice but to lose the per-warning detail
and fall back on the no-warning bit.

> 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?

I haven't.  Thanks for offering to take it on!  I'm curious to
hear how your change fixes that problem.

> 
> 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.

Hmm.  That makes me wonder if the change makes PR101204 go latent
rather than fixing the root cause.

> 
> Are you aware of any other PRs related to this functionality that I
> should re-evaluate?

Nope, just these two.  Thanks again for working on it!

(Either David or a middle end maintainer will need to approve the last
patch once it's final.)

Martin

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