Re: [PATCH 0/11] warning control by group and location (PR 74765)

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2021-05-24 at 16:02 -0600, Martin Sebor wrote:
> Having just one bit control whether an expression or statement should
> be allowed to trigger warnings has been a source of bug reports about
> false negatives for years.  PR 74765 has a representative test case
> that shows how by setting the bit to avoid -Wparentheses the C++ front
> end also ends up preventing valid -Wuninitialized in the middle end,

For reference, PR 74765 has:

int foo(int x, int y)
{
    int i;
    if ((i ==0)) return x;
    return y;
}

int foo2(int x, int y)
{
    int i;
    if (i ==0) return x;
    return y;
}

where both fns have an uninitialized use of "i", but the double-parens
in "foo" have already been warned about, so TREE_NO_WARNING is set,
hence we don't get an uninit warning, right?

> but there are other such reports for C (e.g., PR 74762) as well as
> the middle end.


> This patch series solves the problem by associating an expression
> (whether a tree node or a GIMPLE statement) with more than one such
> bit through its location.  Each new bit in the mapping corresponds
> to a group of warnings (e.g., lexical, access, etc.), and each
> location may be associated with a simple bitmap with one bit for
> each group.  The initial groups are mostly ad hoc and should be
> refined over time.

The overall idea looks promising, thanks.

I didn't see any test cases in the patch kit; do you have some that
demonstrate the fixes for the above PRs?

I'm slightly nervous that, although we're gaining granularity in terms
of the different categories of warning, do we risk losing granularity
due to expressions sharing locations?

>   The rare expressions that have no location
> continue to have just one bit[1].

Where does this get stored?  I see the final patch in the kit removes
TREE_NO_WARNING, but I don't quite follow the logic for where the bit
would then be stored for an expr with UNKNOWN_LOCATION.

> 
> The first patch introduces three new APIs without making use of them
> in existing code:
> 
>    bool get_no_warning (..., int option);
>    void set_no_warning (..., int option, ...);
>    void copy_no_warning (...);

Is it possible to use "enum opt_code" (from the generated options.h)
rather than a plain "int" for these?  Or is this not available
everywhere we use these?

> 
> Subsequent patches then replace invocations of the TREE_NO_WARNING()
> macro and the gimple_no_warning_p() and gimple_set_no_warning()
> functions throughout GCC with those and remove the legacy APIs to
> keep them from being accidentally reintroduced along with the
> problem.
> These are mostly mechanical changes, except that most of the new
> invocations also specify the option whose disposition to query for
> the expression or location, or which to enable or disable[2].
> The last function, copy_no_warning(), copies the disposition from
> one expression or location to another.
> 
> A couple of design choices might be helpful to explain:
> 
> First, introducing "warning groups" rather than controlling each
> individual warning is motivated by a) the fact that the latter
> would make avoiding redundant warnings for related problems
> cumbersome (e.g., after issuing a -Warray-bounds we want to
> suppress -Wstringop-overflow as well -Wstringop-overread for
> the same access and vice versa), and b) simplicity and efficiency
> of the implementation (mapping each option would require a more
> complex data structure like a bitmap).
> 
> Second, using location_t to associate expressions/statements with
> the warning groups also turns out to be more useful in practice
> than a direct association between a tree or gimple*, and also
> simplifies managing the data structure.  Trees and gimple* come
> and go across passes, and maintaining a mapping for them that
> accounts for the possibility of them being garbage-collected
> and the same addresses reused is less than straightforward.

I find some of the terminology rather awkard due to it the negation
we're already using with TREE_NO_WARNING, in that we're turning on a
no_warning flag, and that this is a per-location/expr/stmt thing that's
different from the idea of enabling/disabling a specific warning
altogether (and the pragmas that control that).   Sometimes the patches
refer to enabling/disabling warnings and I think I want "enabling" and
"disabling" to mean the thing the user does with -Wfoo and -Wno-foo.

Would calling it a "warning suppression" or somesuch be more or less
klunky?

> 
> Martin
> 
> [1] My expectation is to work toward providing locations for all
> expressions/statements, even if it's the opening or closing brace
> of the function they're used in.)
> 
> [2] A number of {get,set}_no_warning() calls introduced by the patch
> don't provide an option argument and query or set just the one bit in
> the expression/statement.  Some of these may be correct as they are,
> but others could be refined to also specify an option.  I can do that
> in a follow-up patch if someone helps me find the right option.


Hope this is constructive
Dave