Hi, On Mon, Nov 11 2019, Martin Sebor wrote: > On 11/8/19 5:41 AM, Martin Jambor wrote: >> Hi, >> >> this patch is an attempt to implement my idea from a previous thread >> about moving -Wmaybe-uninitialized to -Wextra: >> >> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00220.html >> >> Specifically, it attempts to split -Wmaybe-uninitialized into those that >> are about SRA DECLs and those which are not, and move to -Wextra only >> the former ones. The main idea is that false -Wmaybe-uninitialized >> warings about values that are scalar in user's code are easy to silence >> by initializing them to zero or something, as opposed to bits of >> aggregates such as a value in std::optional which are not. Therefore, >> the warnings about user-scalars can remain in -Wall but warnings about >> SRAed pieces should be moved to -Wextra. >> >> The patch is a bit bigger because of documentation (which I'll be happy >> to improve based on your suggestions) and testsuite churn, but the main >> bit is the following added test in warn_uninit function: >> >> if (wc == OPT_Wmaybe_uninitialized >> && SSA_NAME_VAR (t) >> && DECL_ARTIFICIAL (SSA_NAME_VAR (t)) >> && DECL_HAS_DEBUG_EXPR_P (SSA_NAME_VAR (t))) >> { >> if (warn_maybe_uninitialized_aggregates) >> wc = OPT_Wmaybe_uninitialized_aggregates; >> else >> return; >> } >> >> The reason why I also test DECL_HAS_DEBUG_EXPR_P is >> gfortran.dg/pr39666-2.f90 - without it the test silences a warning about >> a decl representing the return value which is an artificial scalar >> variable (probably all the way from the front-end). We can of course >> not care about not warning for it but then I don't know how to call and >> document the new option :-) Generally, if someone can think of a better >> test to identify SRA DECLs, I'll be happy to change that. We might put >> a bit to identify SRA decls in the decl tree, but I tend to think that >> is not a good use of the few remaining bits there. >> >> What do you think, is something along these lines a good idea? >> >> Thanks, >> >> Martin >> >> >> >> 2019-11-08 Martin Jambor <mjam...@suse.cz> >> >> * common.opt (Wmaybe-uninitialized-aggregates): New. >> * tree-ssa-uninit.c (gate_warn_uninitialized): Also run if >> warn_maybe_uninitialized_aggregates is set. >> (warn_uninit): Warn for artificial DECLs only if >> warn_maybe_uninitialized_aggregates is set. >> * doc/invoke.texi (Warning Options): Add >> -Wmaybe-uninitialized-aggregates to the list. >> (-Wextra): Likewise. >> (-Wmaybe-uninitialized): Document that it only works on scalars. >> (-Wmaybe-uninitialized-aggregates): Document. >> >> testsuite/ >> * gcc.dg/pr45083.c: Add Wmaybe-uninitialized-aggregates to options. >> * gcc.dg/ubsan/pr81981.c: Likewise. >> * gfortran.dg/pr25923.f90: Likewise. >> * g++.dg/warn/pr80635.C: New. >> --- >> gcc/common.opt | 4 +++ >> gcc/doc/invoke.texi | 18 +++++++++-- >> gcc/testsuite/g++.dg/warn/pr80635.C | 45 +++++++++++++++++++++++++++ >> gcc/testsuite/gcc.dg/pr45083.c | 2 +- >> gcc/testsuite/gcc.dg/ubsan/pr81981.c | 2 +- >> gcc/testsuite/gfortran.dg/pr25923.f90 | 2 +- >> gcc/tree-ssa-uninit.c | 14 ++++++++- >> 7 files changed, 81 insertions(+), 6 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C >> >> diff --git a/gcc/common.opt b/gcc/common.opt >> index cc279f411d7..03769299df8 100644 >> --- a/gcc/common.opt >> +++ b/gcc/common.opt >> @@ -783,6 +783,10 @@ Wmaybe-uninitialized >> Common Var(warn_maybe_uninitialized) Warning EnabledBy(Wuninitialized) >> Warn about maybe uninitialized automatic variables. >> >> +Wmaybe-uninitialized-aggregates >> +Common Var(warn_maybe_uninitialized_aggregates) Warning EnabledBy(Wextra) >> +Warn about maybe uninitialized automatic parts of aggregates. >> + >> Wunreachable-code >> Common Ignore Warning >> Does nothing. Preserved for backward compatibility. >> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >> index faa7fa95a0e..dbc3219b770 100644 >> --- a/gcc/doc/invoke.texi >> +++ b/gcc/doc/invoke.texi >> @@ -328,7 +328,8 @@ Objective-C and Objective-C++ Dialects}. >> -Wzero-length-bounds @gol >> -Winvalid-pch -Wlarger-than=@var{byte-size} @gol >> -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol >> --Wmain -Wmaybe-uninitialized -Wmemset-elt-size -Wmemset-transposed-args >> @gol >> +-Wmain -Wmaybe-uninitialized -Wmaybe-uninitialized-aggregates @gol >> +-Wmemset-elt-size -Wmemset-transposed-args @gol >> -Wmisleading-indentation -Wmissing-attributes -Wmissing-braces @gol >> -Wmissing-field-initializers -Wmissing-format-attribute @gol >> -Wmissing-include-dirs -Wmissing-noreturn -Wmissing-profile @gol >> @@ -4498,6 +4499,7 @@ name is still supported, but the newer name is more >> descriptive.) >> -Wempty-body @gol >> -Wignored-qualifiers @gol >> -Wimplicit-fallthrough=3 @gol >> +-Wmaybe-uninitialized-aggregates @gol >> -Wmissing-field-initializers @gol >> -Wmissing-parameter-type @r{(C only)} @gol >> -Wold-style-declaration @r{(C only)} @gol >> @@ -5690,10 +5692,22 @@ in fact be called at the place that would cause a >> problem. >> >> Some spurious warnings can be avoided if you declare all the functions >> you use that never return as @code{noreturn}. @xref{Function >> -Attributes}. >> +Attributes}. This option only warns about scalar variables, use >> +@option{-Wmaybe-uninitialized-aggregates} to enable the warnings on parts of >> +aggregates which the compiler can track. >> >> This warning is enabled by @option{-Wall} or @option{-Wextra}. >> >> +@item -Wmaybe-uninitialized-aggregates >> +@opindex Wmaybe-uninitialized-aggregates >> +@opindex Wmaybe-uninitialized-aggregates >> + >> +This option enables the same warning like @option{-Wmaybe-uninitialized} but >> +for parts of aggregates (i.g.@: structures, arrays or unions) that GCC >> +optimizers can track. These warnings are only possible in optimizing >> +compilation, because otherwise GCC does not keep track of the state of >> +variables. This warning is enabled by @option{-Wextra}. >> + > > Let me ask a question. Suppose I have code like this: > > struct S { char a[4], b[4]; }; > > void* g (void) > { > struct S *p = malloc (sizeof *p); > strcpy (p->a + 1, p->b + 1); > return p; > } > > (I include the offsets only because they make an interesting > difference in the internal representation. My question is > the same even without them.) > > With this new warning, would the appropriate diagnostic to > issue be -Wmaybe-uninitialized-aggregates or -Wuninitialized?
The patch should not change the behavior of -Wuninitialized so that if an uninitialized value use is detected on a spot which is always executed, a warning should be emitted regardless if the underpinning DECL is a result of SRA or not - the user should fix their code, not silence a spurious warning because it is not spurious. It's the tricky maybe-uninitialized cases where I wanted to mitigate a common source of false positives which are difficult to silence. As far as the strcpy example is concerned, ideally it would be emitted as part of both -Wuninitialized and -Wmaybe-uninitialized-aggregates depending on whether it is a maybe warning or not, but not with only -Wmaybe-uninitialized. > > The description makes it sound like the former but I'm not > sure that's what I would want, either as an implementer of > the uninitialized strcpy warning (I plan to add one) or as > its user. What are the problems for the user? I think that the distinction between maybe uninitialized and always uninitialized is genuinely useful one. And as an implementor of a new similar warning, don't you need to distinguish between them even now? > > On the other hand, if the answer is the latter (or that it > depends) then introducing an option for it would seem like > exposing an interface to an internal detail (limitation) > with unspecified effects. I agree that describing the new option would be tricky and I am opened to suggestions for improvement/overhaul of that. On the other hand, the options already depend on internal details and limitations because they only work on stuff that GCC can track. E.g. they stop working as soon as a variable - even a scalar one - happens to be addressable, for example if we do less inlining. Likewise if SRA suddenly decided not to scalarize something the warning for that bit would be gone too. > PS In the first sentence of the description, the phrase should > read "enables the same warning as", not "like". OK, thanks, Martin
