Eliminates uninitialized warning for volatile variables.

[lethalantidote]

Summary

Eliminates uninitialized warning for volatile variables.

[thakis]

Thanks! Can you add a test somewhere? grep -R W.*uninitialized test/Sema* will probably show you the existing test for this code. (Another technique: Comment some of the existing tests, run ninja check-clang, and see which tests start failing.)

You can run an individual test via bin/llvm-lit ../../llvm/tools/clang/test/Sema/mytest.cc.

clang/lib/Sema/AnalysisBasedWarnings.cpp

905

Should the check be in DiagUninitUse()? Or is there a reason this one should happen for volatiles?

[lethalantidote]

Addresses thakis' comments and adds a test.

clang/lib/Sema/AnalysisBasedWarnings.cpp

905

Good point. Thanks for catching that. I hoped I understood this use case correctly, lemme know if the update makes sense.

[thakis]

Thanks for the test!

clang/lib/Sema/AnalysisBasedWarnings.cpp

699

What about the other cases in this switch? Does a volatile still warn in those cases? Should it? (Probably not?)

Right now the approach is to add an early return when the warning is emitted. Maybe this change should instead be somewhere where we compute if a var could be uninitialized, and that should always be false for volatile variables? Then future other warnings looking at that bit would get volatiles right for free.

905

Is it possible to test that both branches have this early return now?

[lethalantidote]

Addresses moving check further up, during analysis.
Adds test to check for sometimes branch. Please review.

[thakis]

Thanks, this is looking pretty good! From clicking around a bit on cs, do you think it's better to put the check where you have it, or is maybe http://llvm-cs.pcc.me.uk/tools/clang/lib/Analysis/UninitializedValues.cpp#36 more appropriate? I think having it where you have it means saying "we can reason about volatile vars, and we want to treat them as initialized" while the other spot means "we can't reason about volatile variables at all". I don't know which place is better (of someone else reading this knows, please speak up). If the other place makes your test fail, having the check where you have it is probably fine. Else I'd say that moving the check up is probably better.

clang/lib/Analysis/UninitializedValues.cpp

763 ↗

(On Diff #84026)

No idea what should happen with volatile int x = x -- but I've never seen that in practice, so it probably doesn't matter too much either way.

[lethalantidote]

Moves check in IsTracked().

[lethalantidote]

So I tried the update you suggested (moving it up into IsTracked) and it seems to work and the reasoning makes sense to me.

[thakis]

Looks great to me, thanks! Unless someone else shouts, I'll land this for you soon.

[lethalantidote]

Any updates on this?

[hfinkel]

What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized).

[thakis]

In D28543#671953, @hfinkel wrote:

What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized).

volatile means that the value can change at any time, which might initialize the variable, no?

[joerg]

Please fix the spelling errors in the titel / summary before commit.

I somewhat agree with Hal -- I think this is too aggressive. Common use cases for local volatile include atomic ops or returns-twice functions like setjmp/longjmp.
Disabling the warning in those cases has a high chance of hiding real problems. I would find it much more useful to move this case into a warning subgroup, so that
it can be selectively disabled by command line or pragma.

[lethalantidote]

Is there any subgroup that one could suggest for this warning to fall under?

clang/lib/Sema/AnalysisBasedWarnings.cpp

void HandleUnreachable(reachable_code::UnreachableKind UK,
         case reachable_code::UK_Loop_Increment:
           diag = diag::warn_unreachable_loop_increment;
           break;
         case reachable_code::UK_Other:
           break;
       }
 
       S.Diag(L, diag) << R1 << R2;
-      
       SourceLocation Open = SilenceableCondVal.getBegin();
       if (Open.isValid()) {
         SourceLocation Close = SilenceableCondVal.getEnd();
         Close = S.getLocForEndOfToken(Close);
         if (Close.isValid()) {
           S.Diag(Open, diag::note_unreachable_silence)
             << FixItHint::CreateInsertion(Open, "/* DISABLES CODE */ (")
             << FixItHint::CreateInsertion(Close, ")");

 /// DiagUninitUse -- Helper function to produce a diagnostic for an
 /// uninitialized use of a variable.
 static void DiagUninitUse(Sema &S, const VarDecl *VD, const UninitUse &Use,
                           bool IsCapturedByBlock) {
   bool Diagnosed = false;
 
   switch (Use.getKind()) {
   case UninitUse::Always:
+    if (VD->getType().isVolatileQualified()) {
+      return;
+    }
     S.Diag(Use.getUser()->getLocStart(), diag::warn_uninit_var)
         << VD->getDeclName() << IsCapturedByBlock
         << Use.getUser()->getSourceRange();
     return;
 
   case UninitUse::AfterDecl:
   case UninitUse::AfterCall:
     S.Diag(VD->getLocation(), diag::warn_sometimes_uninit_var)

 
 /// DiagnoseUninitializedUse -- Helper function for diagnosing uses of an
 /// uninitialized variable. This manages the different forms of diagnostic
 /// emitted for particular types of uses. Returns true if the use was diagnosed
 /// as a warning. If a particular use is one we omit warnings for, returns
 /// false.
 static bool DiagnoseUninitializedUse(Sema &S, const VarDecl *VD,
                                      const UninitUse &Use,
-                                     bool alwaysReportSelfInit = false) {
+                                     bool alwaysReportSelfInit = false) { 
   if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Use.getUser())) {
     // Inspect the initializer of the variable declaration which is
     // being referenced prior to its initialization. We emit
     // specialized diagnostics for self-initialization, and we
     // specifically avoid warning about self references which take the
     // form of:
     //
     //   int x = x;
if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Use.getUser())) {
     DiagUninitUse(S, VD, Use, false);
   } else {
     const BlockExpr *BE = cast<BlockExpr>(Use.getUser());
     if (VD->getType()->isBlockPointerType() && !VD->hasAttr<BlocksAttr>())
       S.Diag(BE->getLocStart(),
              diag::warn_uninit_byref_blockvar_captured_by_block)
         << VD->getDeclName();
     else
       DiagUninitUse(S, VD, Use, true);
   }
 
   // Report where the variable was declared when the use wasn't within
   // the initializer of that declaration & we didn't already suggest
   // an initialization fixit.
   if (!SuggestInitializationFixit(S, VD))
     S.Diag(VD->getLocStart(), diag::note_var_declared_here)
       << VD->getDeclName();

clang/test/Sema/uninit-variables.c

int test3() {
   int x;
   x = 0;
   return x; // no-warning
 }
 
 int test4() {
   int x; // expected-note{{initialize the variable 'x' to silence this warning}}
   ++x; // expected-warning{{variable 'x' is uninitialized when used here}}
-  return x; 
+  return x;
 }
 
 int test5() {
   int x, y; // expected-note{{initialize the variable 'y' to silence this warning}}
   x = y; // expected-warning{{variable 'y' is uninitialized when used here}}
   return x;
 }
 
void test54() {
   int c;  // expected-note {{initialize the variable 'c' to silence this warning}}
   ASSIGN(int, c, d);  // expected-warning {{variable 'c' is uninitialized when used here}}
 }
 
 // Taking the address is fine
 struct { struct { void *p; } a; } test55 = { { &test55.a }}; // no-warning
 struct { struct { void *p; } a; } test56 = { { &(test56.a) }}; // no-warning
 
+int test57() {
+  volatile int x;
+  return x; // no-warning
+}
+
 void uninit_in_loop() {
   int produce(void);
   void consume(int);
   for (int n = 0; n < 100; ++n) {
     int k; // expected-note {{initialize}}
     consume(k); // expected-warning {{variable 'k' is uninitialized}}
     k = produce();
   }