Hi!

Martin, thanks for your review.  Now need someone to formally approve the
third patch.

On 2021-09-01T18:14:46-0600, Martin Sebor <msebor@gmail.com> wrote:
> On 9/1/21 1:35 PM, Thomas Schwinge wrote:
>> 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.]

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

ACKed by Martin, again attached for convenience.


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

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

Yes, that will be part of another patch here -- waiting for approval of
"Generalize 'gcc/input.h:struct location_hash'" posted elsewhere.


>> attached "Don't maintain a warning spec for
>> 'UNKNOWN_LOCATION'/'BUILTINS_LOCATION' [PR101574]".  OK to push?
>
> [...].  So I agree that it ought to be fixed.

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

OK, was just trying to make sure that I don't fail to see any non-obvious
intentions 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).

What I observed was that the 'assert' never triggered for the
'location_t' variant "called [only] from gimple_set_location" -- but does
trigger for some other variant.  Anyway:

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

ACK.  I'm thus clarifying that as follows:

    --- gcc/diagnostic-spec.c
    +++ gcc/diagnostic-spec.c
    @@ -185,7 +185,5 @@ copy_warning (location_t to, location_t from)
       if (RESERVED_LOCATION_P (to))
    -    {
    -      /* If we cannot set no-warning dispositions for 'to', ascertain that we
    -    don't have any for 'from'.  Otherwise, we'd lose these.  */
    -      gcc_checking_assert (!from_spec);
    -    }
    +    /* We cannot set no-warning dispositions for 'to', so we have no chance but
    +       lose those potentially set for 'from'.  */
    +    ;
       else
    --- gcc/warning-control.cc
    +++ gcc/warning-control.cc
    @@ -197,9 +197,5 @@ void copy_warning (ToType to, FromType from)
       if (RESERVED_LOCATION_P (to_loc))
    -    {
    -#if 0 //TODO triggers during GCC build
    -      /* If we cannot set no-warning dispositions for 'to', ascertain that we
    -    don't have any for 'from'.  Otherwise, we'd lose these.  */
    -      gcc_checking_assert (!from_spec);
    -#endif
    -    }
    +    /* We cannot set no-warning dispositions for 'to', so we have no chance but
    +       lose those potentially set for 'from'.  */
    +    ;
       else

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

As far as I'm concerned that would be the attached third patch:
"Don't maintain a warning spec for 'UNKNOWN_LOCATION'/'BUILTINS_LOCATION'
[PR101574]".  OK to push?


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);
>>> +}


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