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

The attached patch introduces the suppress_warning(),
warning_suppressed(), and copy_no_warning() APIs without making
use of them in the rest of GCC.  They are in three files:

   diagnostic-spec.{h,c}: Location-centric overloads.
   warning-control.cc: Tree- and gimple*-centric overloads.

The location-centric overloads are suitable to use from the diagnostic
subsystem.  The rest can be used from the front ends and the middle end.

Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571973.html

Looking for a review of v2 of the diagnostic infrastructure bits.

On 6/4/21 3:41 PM, Martin Sebor wrote:
> The attached patch introduces the suppress_warning(),
> warning_suppressed(), and copy_no_warning() APIs without making
> use of them in the rest of GCC.  They are in three files:
> 
>    diagnostic-spec.{h,c}: Location-centric overloads.
>    warning-control.cc: Tree- and gimple*-centric overloads.
> 
> The location-centric overloads are suitable to use from the diagnostic
> subsystem.  The rest can be used from the front ends and the middle end.

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 without making
> use of them in the rest of GCC.  They are in three files:
> 
>    diagnostic-spec.{h,c}: Location-centric overloads.
>    warning-control.cc: Tree- and gimple*-centric overloads.
> 
> The location-centric overloads are suitable to use from the diagnostic
> subsystem.  The rest can be used from the front ends and the middle
> end.

[...snip...]

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

Looking through the patch, I don't like the use of "dis" for the "is
suppressed" bool, since...

[...snip...]

> +
> +/* Enable, or by default disable, a warning for the statement STMT.
> +   The wildcard OPT of -1 controls all warnings.  */

...I find the above comment to be confusingly worded due to the double-
negative.

If I'm reading your intent correctly, how about this wording:

/* Change the supression of warnings for statement STMT.
   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.  */

...and rename "dis" to "supp".

Or have I misread the intent of the patch?

> +
> +void
> +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
> +		  bool dis /* = true */)

> +{
> +  if (opt == no_warning)
> +    return;
> +
> +  const key_type_t key = convert_to_key (stmt);
> +
> +  dis = suppress_warning_at (key, opt, dis) || dis;
> +  set_no_warning_bit (stmt, dis);
> +}

[...snip...]

Dave

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 without making
> > use of them in the rest of GCC.  They are in three files:
> > 
> >    diagnostic-spec.{h,c}: Location-centric overloads.
> >    warning-control.cc: Tree- and gimple*-centric overloads.
> > 
> > The location-centric overloads are suitable to use from the
> > diagnostic
> > subsystem.  The rest can be used from the front ends and the middle
> > end.
> 
> [...snip...]
> 
> > +/* 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;
> 
> Looking through the patch, I don't like the use of "dis" for the "is
> suppressed" bool, since...
> 
> [...snip...]
> 
> > +
> > +/* Enable, or by default disable, a warning for the statement STMT.
> > +   The wildcard OPT of -1 controls all warnings.  */
> 
> ...I find the above comment to be confusingly worded due to the double-
> negative.
> 
> If I'm reading your intent correctly, how about this wording:
> 
> /* Change the supression of warnings for statement STMT.
>    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.  */
> 
> ...and rename "dis" to "supp".
> 
> Or have I misread the intent of the patch?
> 
> > +
> > +void
> > +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
> > +                 bool dis /* = true */)
> 
> > +{
> > +  if (opt == no_warning)
> > +    return;
> > +
> > +  const key_type_t key = convert_to_key (stmt);
> > +
> > +  dis = suppress_warning_at (key, opt, dis) || dis;
> > +  set_no_warning_bit (stmt, dis);
> > +}
> 
> [...snip...]

Also, I think I prefer having a separate entrypoints for the "all
warnings" case; on reading through the various patches I think that in
e.g.:

-	  TREE_NO_WARNING (*expr_p) = 1;
+	  suppress_warning (*expr_p);

I prefer:

          suppress_warnings (*expr_p);

(note the plural) since that way we can grep for them, and it seems
like better grammar to me.

Both entrypoints could be implemented by a static suppress_warning_1
internally if that makes it easier.

In that vein, "unsuppress_warning" seems far clearer to me that
"suppress_warning (FOO, false)"; IIRC there are very few uses of this
non-default arg (I couldn't find any in a quick look through the v2
kit).

Does this make sense?
Dave

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 without making
>>> use of them in the rest of GCC.  They are in three files:
>>>
>>>     diagnostic-spec.{h,c}: Location-centric overloads.
>>>     warning-control.cc: Tree- and gimple*-centric overloads.
>>>
>>> The location-centric overloads are suitable to use from the
>>> diagnostic
>>> subsystem.  The rest can be used from the front ends and the middle
>>> end.
>>
>> [...snip...]
>>
>>> +/* 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;
>>
>> Looking through the patch, I don't like the use of "dis" for the "is
>> suppressed" bool, since...

It's an argument to a a function, unlikely to confuse anyone.  But
I also don't think it's important enough to argue about so I changed
it along with the comment below.

>>
>> [...snip...]
>>
>>> +
>>> +/* Enable, or by default disable, a warning for the statement STMT.
>>> +   The wildcard OPT of -1 controls all warnings.  */
>>
>> ...I find the above comment to be confusingly worded due to the double-
>> negative.
>>
>> If I'm reading your intent correctly, how about this wording:
>>
>> /* Change the supression of warnings for statement STMT.
>>     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.  */
>>
>> ...and rename "dis" to "supp".
>>
>> Or have I misread the intent of the patch?
>>
>>> +
>>> +void
>>> +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
>>> +                 bool dis /* = true */)
>>
>>> +{
>>> +  if (opt == no_warning)
>>> +    return;
>>> +
>>> +  const key_type_t key = convert_to_key (stmt);
>>> +
>>> +  dis = suppress_warning_at (key, opt, dis) || dis;
>>> +  set_no_warning_bit (stmt, dis);
>>> +}
>>
>> [...snip...]
> 
> Also, I think I prefer having a separate entrypoints for the "all
> warnings" case; on reading through the various patches I think that in
> e.g.:
> 
> -	  TREE_NO_WARNING (*expr_p) = 1;
> +	  suppress_warning (*expr_p);
> 
> I prefer:
> 
>            suppress_warnings (*expr_p);
> 
> (note the plural) since that way we can grep for them, and it seems
> like better grammar to me.
> 
> Both entrypoints could be implemented by a static suppress_warning_1
> internally if that makes it easier.
> 
> In that vein, "unsuppress_warning" seems far clearer to me that
> "suppress_warning (FOO, false)"; IIRC there are very few uses of this
> non-default arg (I couldn't find any in a quick look through the v2
> kit).
> 
> Does this make sense?

In my experience, names that differ only slightly (such as in just
one letter) tend to easily lead to frustrating mistakes.  The new
warning_suppressed_p() added in this patch also handles multiple
warnings (via a wildcard) and uses a singular form, and as do
the gimple_{get,set}no_warning() functions that are being replaced.
So I don't think the suggested change is needed or would be
an improvement.

Similarly, there is no gimple_unset_no_warning() today and I don't
think adding unsuppress_warning() is necessary or would add value,
especially since there are just a handful of callers that re-enable
warnings.  For the callers that might need to enable or disable
a warning based on some condition, it's more convenient to do so
in a single call then by having to call different functions based
the value of the condition.

In the attached patch I've changed the comment as you asked and
also renamed the dis argument to supp but kept the function names
the same.  I've also moved a few changes to tree.h from a later
patch to this one to let it stand alone and regtested it on its
own on x86_64-linux.  I'll plan to go ahead with this version
unless requestsfor substantive changes come up in the review of
the rest  of the changes.

Thanks
Martin

On 6/23/2021 1:47 PM, Martin Sebor via Gcc-patches 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 without making
>>>> use of them in the rest of GCC.  They are in three files:
>>>>
>>>>     diagnostic-spec.{h,c}: Location-centric overloads.
>>>>     warning-control.cc: Tree- and gimple*-centric overloads.
>>>>
>>>> The location-centric overloads are suitable to use from the
>>>> diagnostic
>>>> subsystem.  The rest can be used from the front ends and the middle
>>>> end.
>>>
>>> [...snip...]
>>>
>>>> +/* 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;
>>>
>>> Looking through the patch, I don't like the use of "dis" for the "is
>>> suppressed" bool, since...
>
> It's an argument to a a function, unlikely to confuse anyone.  But
> I also don't think it's important enough to argue about so I changed
> it along with the comment below.
>
>>>
>>> [...snip...]
>>>
>>>> +
>>>> +/* Enable, or by default disable, a warning for the statement STMT.
>>>> +   The wildcard OPT of -1 controls all warnings.  */
>>>
>>> ...I find the above comment to be confusingly worded due to the double-
>>> negative.
>>>
>>> If I'm reading your intent correctly, how about this wording:
>>>
>>> /* Change the supression of warnings for statement STMT.
>>>     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.  */
>>>
>>> ...and rename "dis" to "supp".
>>>
>>> Or have I misread the intent of the patch?
>>>
>>>> +
>>>> +void
>>>> +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
>>>> +                 bool dis /* = true */)
>>>
>>>> +{
>>>> +  if (opt == no_warning)
>>>> +    return;
>>>> +
>>>> +  const key_type_t key = convert_to_key (stmt);
>>>> +
>>>> +  dis = suppress_warning_at (key, opt, dis) || dis;
>>>> +  set_no_warning_bit (stmt, dis);
>>>> +}
>>>
>>> [...snip...]
>>
>> Also, I think I prefer having a separate entrypoints for the "all
>> warnings" case; on reading through the various patches I think that in
>> e.g.:
>>
>> -      TREE_NO_WARNING (*expr_p) = 1;
>> +      suppress_warning (*expr_p);
>>
>> I prefer:
>>
>>            suppress_warnings (*expr_p);
>>
>> (note the plural) since that way we can grep for them, and it seems
>> like better grammar to me.
>>
>> Both entrypoints could be implemented by a static suppress_warning_1
>> internally if that makes it easier.
>>
>> In that vein, "unsuppress_warning" seems far clearer to me that
>> "suppress_warning (FOO, false)"; IIRC there are very few uses of this
>> non-default arg (I couldn't find any in a quick look through the v2
>> kit).
>>
>> Does this make sense?
>
> In my experience, names that differ only slightly (such as in just
> one letter) tend to easily lead to frustrating mistakes.  The new
> warning_suppressed_p() added in this patch also handles multiple
> warnings (via a wildcard) and uses a singular form, and as do
> the gimple_{get,set}no_warning() functions that are being replaced.
> So I don't think the suggested change is needed or would be
> an improvement.
>
> Similarly, there is no gimple_unset_no_warning() today and I don't
> think adding unsuppress_warning() is necessary or would add value,
> especially since there are just a handful of callers that re-enable
> warnings.  For the callers that might need to enable or disable
> a warning based on some condition, it's more convenient to do so
> in a single call then by having to call different functions based
> the value of the condition.
>
> In the attached patch I've changed the comment as you asked and
> also renamed the dis argument to supp but kept the function names
> the same.  I've also moved a few changes to tree.h from a later
> patch to this one to let it stand alone and regtested it on its
> own on x86_64-linux.  I'll plan to go ahead with this version
> unless requestsfor substantive changes come up in the review of
> the rest  of the changes.
>
> Thanks
> Martin
>
> gcc-no-warning-1.diff
>
> Add support for per-location warning groups.
>
> gcc/ChangeLog:
>
> 	* Makefile.in (OBJS-libcommon): Add diagnostic-spec.o.
> 	* gengtype.c (open_base_files): Add diagnostic-spec.h.
> 	* diagnostic-spec.c: New file.
> 	* diagnostic-spec.h: New file.
> 	* tree.h (no_warning, all_warnings, suppress_warning_at): New
> 	declarations.
> 	* warning-control.cc: New file.
>
> diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h
> new file mode 100644
> index 00000000000..62d9270ca6d
> --- /dev/null
> +++ b/gcc/diagnostic-spec.h
> @@ -0,0 +1,140 @@
> +/* Language-independent APIs to enable/disable per-location warnings.
> +
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   Contributed by Martin Sebor <msebor@redhat.com>
> +
> +   This file is part of GCC.
> +
> +   GCC is free software; you can redistribute it and/or modify it under
> +   the terms of the GNU General Public License as published by the Free
> +   Software Foundation; either version 3, or (at your option) any later
> +   version.
> +
> +   GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +   WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +   FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +   for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef DIAGNOSTIC_SPEC_H_INCLUDED
> +#define DIAGNOSTIC_SPEC_H_INCLUDED
> +
> +#include "hash-map.h"
> +
> +/* A "bitset" of warning groups.  */
> +
> +struct nowarn_spec_t
Should probably be "class" not "struct".  Though I don't think we're 
necessarily consistent with this.
> +private:
> +  /* Bitset of warning groups.  */
> +  unsigned bits;
Our conventions are to prefix member fields with m_.

Note that a simple "unsigned" may just be 32 bits.  Though with your 
grouping that may not matter in practice.

I think with the nits fixed this is fine.  I know there's some 
disagreement on singular vs plural, but having two names differing by 
just that is probably going to be more confusing in the end, so I'm on 
the side of using the names as-is.

jeff

On 6/23/21 11:26 PM, Jeff Law wrote:
> 
> 
> On 6/23/2021 1:47 PM, Martin Sebor via Gcc-patches 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 without making
>>>>> use of them in the rest of GCC.  They are in three files:
>>>>>
>>>>>     diagnostic-spec.{h,c}: Location-centric overloads.
>>>>>     warning-control.cc: Tree- and gimple*-centric overloads.
>>>>>
>>>>> The location-centric overloads are suitable to use from the
>>>>> diagnostic
>>>>> subsystem.  The rest can be used from the front ends and the middle
>>>>> end.
>>>>
>>>> [...snip...]
>>>>
>>>>> +/* 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;
>>>>
>>>> Looking through the patch, I don't like the use of "dis" for the "is
>>>> suppressed" bool, since...
>>
>> It's an argument to a a function, unlikely to confuse anyone.  But
>> I also don't think it's important enough to argue about so I changed
>> it along with the comment below.
>>
>>>>
>>>> [...snip...]
>>>>
>>>>> +
>>>>> +/* Enable, or by default disable, a warning for the statement STMT.
>>>>> +   The wildcard OPT of -1 controls all warnings.  */
>>>>
>>>> ...I find the above comment to be confusingly worded due to the double-
>>>> negative.
>>>>
>>>> If I'm reading your intent correctly, how about this wording:
>>>>
>>>> /* Change the supression of warnings for statement STMT.
>>>>     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.  */
>>>>
>>>> ...and rename "dis" to "supp".
>>>>
>>>> Or have I misread the intent of the patch?
>>>>
>>>>> +
>>>>> +void
>>>>> +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
>>>>> +                 bool dis /* = true */)
>>>>
>>>>> +{
>>>>> +  if (opt == no_warning)
>>>>> +    return;
>>>>> +
>>>>> +  const key_type_t key = convert_to_key (stmt);
>>>>> +
>>>>> +  dis = suppress_warning_at (key, opt, dis) || dis;
>>>>> +  set_no_warning_bit (stmt, dis);
>>>>> +}
>>>>
>>>> [...snip...]
>>>
>>> Also, I think I prefer having a separate entrypoints for the "all
>>> warnings" case; on reading through the various patches I think that in
>>> e.g.:
>>>
>>> -      TREE_NO_WARNING (*expr_p) = 1;
>>> +      suppress_warning (*expr_p);
>>>
>>> I prefer:
>>>
>>>            suppress_warnings (*expr_p);
>>>
>>> (note the plural) since that way we can grep for them, and it seems
>>> like better grammar to me.
>>>
>>> Both entrypoints could be implemented by a static suppress_warning_1
>>> internally if that makes it easier.
>>>
>>> In that vein, "unsuppress_warning" seems far clearer to me that
>>> "suppress_warning (FOO, false)"; IIRC there are very few uses of this
>>> non-default arg (I couldn't find any in a quick look through the v2
>>> kit).
>>>
>>> Does this make sense?
>>
>> In my experience, names that differ only slightly (such as in just
>> one letter) tend to easily lead to frustrating mistakes.  The new
>> warning_suppressed_p() added in this patch also handles multiple
>> warnings (via a wildcard) and uses a singular form, and as do
>> the gimple_{get,set}no_warning() functions that are being replaced.
>> So I don't think the suggested change is needed or would be
>> an improvement.
>>
>> Similarly, there is no gimple_unset_no_warning() today and I don't
>> think adding unsuppress_warning() is necessary or would add value,
>> especially since there are just a handful of callers that re-enable
>> warnings.  For the callers that might need to enable or disable
>> a warning based on some condition, it's more convenient to do so
>> in a single call then by having to call different functions based
>> the value of the condition.
>>
>> In the attached patch I've changed the comment as you asked and
>> also renamed the dis argument to supp but kept the function names
>> the same.  I've also moved a few changes to tree.h from a later
>> patch to this one to let it stand alone and regtested it on its
>> own on x86_64-linux.  I'll plan to go ahead with this version
>> unless requestsfor substantive changes come up in the review of
>> the rest  of the changes.
>>
>> Thanks
>> Martin
>>
>> gcc-no-warning-1.diff
>>
>> Add support for per-location warning groups.
>>
>> gcc/ChangeLog:
>>
>> 	* Makefile.in (OBJS-libcommon): Add diagnostic-spec.o.
>> 	* gengtype.c (open_base_files): Add diagnostic-spec.h.
>> 	* diagnostic-spec.c: New file.
>> 	* diagnostic-spec.h: New file.
>> 	* tree.h (no_warning, all_warnings, suppress_warning_at): New
>> 	declarations.
>> 	* warning-control.cc: New file.
>>
>> diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h
>> new file mode 100644
>> index 00000000000..62d9270ca6d
>> --- /dev/null
>> +++ b/gcc/diagnostic-spec.h
>> @@ -0,0 +1,140 @@
>> +/* Language-independent APIs to enable/disable per-location warnings.
>> +
>> +   Copyright (C) 2021 Free Software Foundation, Inc.
>> +   Contributed by Martin Sebor<msebor@redhat.com>
>> +
>> +   This file is part of GCC.
>> +
>> +   GCC is free software; you can redistribute it and/or modify it under
>> +   the terms of the GNU General Public License as published by the Free
>> +   Software Foundation; either version 3, or (at your option) any later
>> +   version.
>> +
>> +   GCC is distributed in the hope that it will be useful, but WITHOUT ANY
>> +   WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> +   FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> +   for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with GCC; see the file COPYING3.  If not see
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef DIAGNOSTIC_SPEC_H_INCLUDED
>> +#define DIAGNOSTIC_SPEC_H_INCLUDED
>> +
>> +#include "hash-map.h"
>> +
>> +/* A "bitset" of warning groups.  */
>> +
>> +struct nowarn_spec_t
> Should probably be "class" not "struct".  Though I don't think we're 
> necessarily consistent with this.
>> +private:
>> +  /* Bitset of warning groups.  */
>> +  unsigned bits;
> Our conventions are to prefix member fields with m_.
> 
> Note that a simple "unsigned" may just be 32 bits.  Though with your 
> grouping that may not matter in practice.
> 
> I think with the nits fixed this is fine.  I know there's some 
> disagreement on singular vs plural, but having two names differing by 
> just that is probably going to be more confusing in the end, so I'm on 
> the side of using the names as-is.

Done and pushed in r12-1801.

Martin

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

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
>

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