Page MenuHomePhabricator

Add a new warning to warn when passing uninitialized variables as const reference parameters to a function
AcceptedPublic

Authored by zequanwu on Wed, May 13, 11:47 AM.

Details

Reviewers
rsmith
aeubanks
Summary

Add a new warning -Wuninitialized-const-reference as a subgroup of -Wuninitialized to address a bug filed here: https://bugs.llvm.org/show_bug.cgi?id=45624
This warning is controlled by -Wuninitialized and can be disabled by -Wno-uninitialized-const-reference.

The warning is diagnosed when passing uninitialized variables as const reference parameters to a function.

Diff Detail

Event Timeline

zequanwu created this revision.Wed, May 13, 11:47 AM
rnk edited reviewers, added: aeubanks; removed: rnk.Wed, May 13, 12:50 PM
rnk added a subscriber: rnk.

Arthur recently went through the process of refining a diagnostic, so he should be able to help guide you in this if you have more questions.

clang/lib/Analysis/UninitializedValues.cpp
426

Based on Richard's comment here:
https://bugs.llvm.org/show_bug.cgi?id=45624#c7
... this should be its own classification kind, perhaps ConstRefUse or something like that.

This will allow us to put this finding under it's own diagnostic group, maybe named something like -Wuninitialized-const-reference, and then users can enable or disable it separately. It is likely that this new behavior will find many new instances over existing codebases, for example Chromium, that compile with -Wuninitialized enabled. So, if we don't leave a way for users to disable the new warnings (-Wno-uninitialized-const-ref), it is harder for them (us) to update the compiler.

zequanwu updated this revision to Diff 264076.Thu, May 14, 1:33 PM

Add new warning as subgroup of Uninitialized.

zequanwu updated this revision to Diff 264129.Thu, May 14, 5:04 PM

Use another mapvector constRefUses to store uninitialized const reference uses so that the new warning can be easily disable by -Wno-initialized-const-reference

The summary of this change should be "add warning..." not "fix warning..."

Could you add more detail into the commit description? About newly introduced diagnostic groups, etc.

Also, I thought I saw tests in a previous revision but now they seem to be gone?

zequanwu retitled this revision from Fix warning about using uninitialized variable as function const reference parameter to Add a new warning to warn when passing uninitialized variables as const reference parameters to a function.Fri, May 15, 11:33 AM
zequanwu edited the summary of this revision. (Show Details)
zequanwu updated this revision to Diff 264300.Fri, May 15, 11:40 AM

Since the new warning is controlled by -Wuninitialized, I disabled it in existing test case and added a separate test case for -Wuninitialized-const-reference.

aeubanks added inline comments.Mon, May 18, 8:36 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2110

s/passes/passed

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp
24

For my knowledge, is this to make sure that this other warning takes precedence over the one introduced in this change? If it is, a comment would be nice.

rnk added inline comments.Mon, May 18, 3:58 PM
clang/lib/Sema/AnalysisBasedWarnings.cpp
1517

If possible, it would be nice to avoid having a second map.

zequanwu added inline comments.Mon, May 18, 4:43 PM
clang/lib/Sema/AnalysisBasedWarnings.cpp
1517

I use second map to let the new warning be orthogonal to existing warnings. That means when we have both -Wuninitialized and -Wno-uninitialized-const-reference, the old test cases about uninitialized variables should all passed. Otherwise, I need to somehow detect the presence of -Wno-uninitialized-const-reference, which I don't see a way to do that. Consider this code if we have only one map:

void consume_const_ref(const int &n);
int test_const_ref() {
  int n;
  consume_const_ref(n);
  return n;
}

No matter if the flag`-Wno-uninitialized-const-reference` is present or not, we still need to add n in consume_const_ref(n); to uses map and then let S.Diag to decide to diagnose it or not depend on the presence of -Wno-uninitialized-const-reference. If the flag is given, S.Diag will just ignore the warning, but uninit var warning will diagnose if Use.getKind() returns Always in https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/AnalysisBasedWarnings.cpp#L814 . That is not desired. Since the original behavior is to ignore the n in consume_const_ref(n); by not adding it to the uses map.

If there is a way to detect the presence of -Wno-uninitialized-const-reference or to know if Sema::Diag(SourceLocation Loc, unsigned DiagID) actually diagnosed a warning, the use of second map can be avoid.

rsmith added inline comments.Mon, May 18, 5:46 PM
clang/lib/Sema/AnalysisBasedWarnings.cpp
1517

Interesting testcase :)

Ideally we want the set of diagnostics from a particular compilation to be the same you'd get by building with -Weverything and then filtering out the ones you didn't want. So it's better to not ask whether -Wuninitialized-const-reference is enabled or not.

I think we only populate this map after we've found an uninitialized use; if so, the cost of a second map seems likely to be acceptable.

FYI, there is a mechanism to ask the "is this diagnostic enabled?" question, but generally we'd prefer if it's only used to avoid doing work that would be redundant if the warning is disabled. You can use Diag.isIgnored(diag::warn_..., Loc) to determine if a diagnostic with the given ID would be suppressed at the given location. (In fact, it looks like this is already being called in a later change in this file.)

1590–1600

Do we want any idiomatic-self-init special-case handling here? For example:

void f(const int&);
void g() {
  int a = a;
  f(a);
}

Following the logic above, should that warn on the int a = a; not on the f(a) call? Or should we warn only on the f(a) call itself in this case? It seems like it might be surprising to say that a is "uninitialized" here, since an initializer was provided, even though it was a garbage one.

Can you provide some compile time data with warning enabled/disabled?

zequanwu updated this revision to Diff 264976.Tue, May 19, 11:14 AM
zequanwu marked 3 inline comments as done.

Fix typo.
Diagnose self-init warning if the self-init variable is used as const reference later.

zequanwu marked an inline comment as done.Tue, May 19, 11:17 AM
zequanwu added inline comments.
clang/lib/Sema/AnalysisBasedWarnings.cpp
1517

I think second map is necessary. So, Diag.isIgnored is not abused.

1590–1600

For this case, I think we should warn at int a = a, like the comment in DiagnoseUninitializedUse said, https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/AnalysisBasedWarnings.cpp#L986-L996

f(a) is considered as accessing a.

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp
24

No, original behavior of -Wuninitialized for line 24 is silence. Now the new warning will be emitted because it is const use of uninitialized variable.

zequanwu marked an inline comment as done.Tue, May 19, 11:36 AM

Can you provide some compile time data with warning enabled/disabled?

I compiled the test case with warning enabled and disabled. Since it is controlled by -Wuninitialzed, it will diagnose when the flag is given.

$ clang -ftime-report -fsyntax-only warn-uninitialized-const-reference.cpp
===-------------------------------------------------------------------------===
                          Clang front-end time report
===-------------------------------------------------------------------------===
  Total Execution Time: 0.0094 seconds (0.0094 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.0091 (100.0%)   0.0003 (100.0%)   0.0094 (100.0%)   0.0094 (100.0%)  Clang front-end timer
   0.0091 (100.0%)   0.0003 (100.0%)   0.0094 (100.0%)   0.0094 (100.0%)  Total

$ clang -ftime-report -Wuninitialized -fsyntax-only warn-uninitialized-const-reference.cpp
===-------------------------------------------------------------------------===
                          Clang front-end time report
===-------------------------------------------------------------------------===
  Total Execution Time: 0.0131 seconds (0.0131 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.0046 (100.0%)   0.0086 (100.0%)   0.0131 (100.0%)   0.0131 (100.0%)  Clang front-end timer
   0.0046 (100.0%)   0.0086 (100.0%)   0.0131 (100.0%)   0.0131 (100.0%)  Total

$ clang -ftime-report -Wuninitialized -Wno-uninitialized-const-reference -fsyntax-only warn-uninitialized-const-reference.cpp
===-------------------------------------------------------------------------===
                          Clang front-end time report
===-------------------------------------------------------------------------===
  Total Execution Time: 0.0109 seconds (0.0109 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.0073 (100.0%)   0.0036 (100.0%)   0.0109 (100.0%)   0.0109 (100.0%)  Clang front-end timer
   0.0073 (100.0%)   0.0036 (100.0%)   0.0109 (100.0%)   0.0109 (100.0%)  Total
clang/test/SemaCXX/warn-uninitialized-const-reference.cpp
24

I meant "const reference use". It was typo.

aeubanks accepted this revision.Thu, May 21, 1:42 PM

One last nit, otherwise lgtm. But I'm not super familiar with this area, it'd be good to get another review from somebody more familiar.

clang/lib/Sema/AnalysisBasedWarnings.cpp
1522–1523

this is very confusing since it shadows the uses member variable from UninitValsDiagReporter

1590–1600

Doesn't DiagnoseUninitializedUse say that we shouldn't warn at int a = a?

This revision is now accepted and ready to land.Thu, May 21, 1:42 PM
zequanwu updated this revision to Diff 265605.Thu, May 21, 2:33 PM
zequanwu marked 2 inline comments as done.

rename parameter uses to um

clang/lib/Sema/AnalysisBasedWarnings.cpp
1590–1600

The last sentence of the comment says that int a = a left a as uninitialized state, and we should warn at int a = a if there is an access of a in uninitialized state. Otherwise, we don't need to warn.

@rsmith Are you okay with this patch?