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

[Jeff Law <jeffreyalaw@gmail.com>]

On 5/24/2021 4:02 PM, Martin Sebor via Gcc-patches 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,
> 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 rare expressions that have no location
> continue to have just one bit[1].
>
> 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 (...);
>
> 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.
>
> 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.
So definitely on board with the broad approach here.  I think it's 
largely compatible with what Jakub suggested a year or two ago in this 
space as well.

I think you've got some API/naming suggestions, so let's get that 
settled and the bits reposted.

Thanks,

jeff