On 5/24/21 5:08 PM, David Malcolm wrote:
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,
Thanks for the quick reply!
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?
Yes, close. The double parens are used to suppress warning about
assignment in the if condition (-Wparentheses). The suppression is
applied to the equality expression which then turns into a GIMPLE_COND
statement with the no-warning set. -Wuninitialized then avoids
triggering because it sees the no-warning bit set on the GIMPLE_COND.
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?
Yes. I have a test case for PR 74765 in my tree and but I missed
including it in the patch series. I have also started going through
Bugzilla looking for other similar reports. I'll include those I
find in the revised patch.
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?
If it's possible for two expressions or statements, say A and B, to
have the same location (is it?) then suppressing warning X for A and
a different warning Y for B would also suppress X for B. (Note that
both A and B would have to have the existing no-warning bit set for
this to happen). I haven't seen this happen and I can't off hand
think of how to get into a situation like that.
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 patch just removes the TREE_NO_WARNING macro (along with
the gimple_no_warning_p/gimple_set_no_warning) functions but not
the no-warning bit itself. It removes them to avoid accidentally
modifying the bit alone without going through the new API and
updating the location -> warning group mapping. The bit is still
needed for expression/statements with no 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?
I was tempted to do it but I resisted because the existing warning
APIs take an int as an argument. It doesn't look like there's
a problem with #including "options.h" in the diagnostic subsystem
so I'm down with changing the argument to opt_code.
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?
I like warning suppression :) But I'm not sure where you suggest
I use the phrase.
I don't particularly care for the "no" in the API names either
(existing or new) and would prefer a positive form. I considered
enable_warning() and warning_enabled() but I chose the names I did
because they're close to their established gimple namesakes. I'm
fine changing them to the alternatives, or if you or someone else
has a preference for different names I'm open to suggestions. Let
me know.
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
Yes, thanks.
Martin
