Warn of uninitialized variables on asm goto's indirect branch

[void]

Summary

Outputs from an asm goto block cannot be used on the indirect branch.
It's not supported and may result in invalid code generation.

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

590

Is this more concise with llvm::any_of?

789

Should we mark the vals MayUninitialized here, rather than below? Then I think we can remove the isAsmGoto check you added in getUninitUse?

clang/test/Analysis/uninit-asm-goto.cpp

16

I wonder if it's straight forward to make this a "maybe uninitialized" warning, instead of "is uninitialized?" Consider the inline asm:

asm volatile goto("ja %l1; mov %0, 42" : "=r"(foo) ::: bar);

Since we can't introspect the inline asm, we're not sure whether foo gets initialized by the asm or not (as the asm could transfer control flow back to the C label before any assignments to the output). Telling the user it's definitely uninitialized is technically correct in this case, but definitely incorrect for:

asm volatile goto("mov %0, 42; ja %l1;" : "=r"(foo) ::: bar);

38

Are we able to catch backwards branches from asm goto? (if so, would you mind please added that as an additional unit test).

int foo;
goto 1;
2:
return foo; // should warn?
1:
asm goto ("": "=r"(foo):::2);
return foo;

[void]

Use "any_of" and improve tests.

clang/test/Analysis/uninit-asm-goto.cpp

16

The back end doesn't know how to generate code for a use in the indirect branches. It's really complicated and may result in code that doesn't actually work. I don't want to give off the impression that the code may work in these cases, because it would be essentially working by accident.

38

Yes, we should be able to warn about this. I added a testcase.

[nickdesaulniers]

strange, the below test isn't warning for me with this patch applied:

$ clang -O2 foo.c -c
$ cat foo.c
int quux(void) {
  int y;
  asm volatile goto("ja %l1" : "=r"(y) ::: err);
  return y;
err:
  return y;
}

[void]

In D71314#1788797, @nickdesaulniers wrote:

strange, the below test isn't warning for me with this patch applied:

$ clang -O2 foo.c -c
$ cat foo.c
int quux(void) {
  int y;
  asm volatile goto("ja %l1" : "=r"(y) ::: err);
  return y;
err:
  return y;
}

Use -Wuninitialized.

[void]

Check that the use isn't in the fallthrough block.

[nickdesaulniers]

Use -Wuninitialized.

D'oh!

clang/lib/Analysis/UninitializedValues.cpp

789

Bumping comment.

clang/test/Analysis/uninit-asm-goto.cpp

57

I think if you left out indirect, it would be clearer what this test is testing.

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

789

Ah, I see this was added in https://reviews.llvm.org/D69876.

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

798

Can you walk me through the logic of this function?

I would assume for changes to asm goto, the above early return would have been removed? Otherwise we seem to be changing the logic of the non-asm goto case?

clang/test/Analysis/uninit-asm-goto.cpp

57

bump

[void]

Use "auto" and change labels to clarify test.

[void]

Variable can't be 'const'

[void]

clang/lib/Analysis/UninitializedValues.cpp

798

Sure. If the variable hasn't been initialized by the time it gets to the asm goto, then we mark it as "potentially uninitialized" so that it's caught on the indirect branch. Uses on the default/fallthrough path should resolve to "initialized".

I'll add a comment.

[void]

Explain what's going on in VisitGCCAsmStmt better.

[void]

Rebasing.

[void]

Friendly ping. :-)

clang/lib/Analysis/UninitializedValues.cpp

while (!Queue.empty()) {
           // from a block that initializes the variable. We can't guarantee to
           // give an earlier location for the diagnostic (and it appears that
           // this code is intended to be reachable) so give a diagnostic here
           // and go no further down this path.
           Use.setUninitAfterDecl();
           continue;
         }
 
+        if (AtPredExit == MayUninitialized) {
+          // If the predecessor's terminator is an "asm goto" that initializes
+          // the variable, then it won't be counted as "initialized" on the
+          // non-fallthrough paths.
+          CFGTerminator term = Pred->getTerminator();
+          if (const auto *as = dyn_cast_or_null<GCCAsmStmt>(term.getStmt())) {
+            const CFGBlock *fallthrough = *Pred->succ_begin();
+            if (as->isAsmGoto() &&
+                llvm::any_of(as->outputs(), [&](const Expr *output) {
+                    return vd == findVar(output).getDecl() &&
+                        llvm::any_of(as->labels(),
+                                     [&](const AddrLabelExpr *label) {
+                          return label->getLabel()->getStmt() == B->Label &&
+                              B != fallthrough;
+                        });
+                })) {
+              Use.setUninitAfterDecl();
+              continue;
+            }
+          }
+        }
+
         unsigned &SV = SuccsVisited[Pred->getBlockID()];
         if (!SV) {
           // When visiting the first successor of a block, mark all NULL
           // successors as having been visited.
           for (CFGBlock::const_succ_iterator SI = Pred->succ_begin(),
                                              SE = Pred->succ_end();
                SI != SE; ++SI)
             if (!*SI)
if (VD && isTrackedVar(VD)) {
       }
     }
   }
 }
 
 void TransferFunctions::VisitGCCAsmStmt(GCCAsmStmt *as) {
   // An "asm goto" statement is a terminator that may initialize some variables.
   if (!as->isAsmGoto())
     return;
 
   for (const Expr *o : as->outputs())
     if (const VarDecl *VD = findVar(o).getDecl())
-      vals[VD] = Initialized;
+      if (vals[VD] != Initialized)
+        // If the variable isn't initialized by the time we get here, then we
+        // mark it as potentially uninitialized for those cases where it's used
+        // on an indirect path, where it's not guaranteed to be defined.
+        vals[VD] = MayUninitialized;
 }
 
 void TransferFunctions::VisitObjCMessageExpr(ObjCMessageExpr *ME) {
   // If the Objective-C message expression is an implicit no-return that
   // is not modeled in the CFG, set the tracked dataflow values to Unknown.
   if (objCNoRet.isImplicitNoReturn(ME)) {
     vals.setAllScratchValues(Unknown);
   }
 }
static bool runOnBlock(const CFGBlock *block, const CFG &cfg,
   }
   // Apply the transfer function.
   TransferFunctions tf(vals, cfg, block, ac, classification, handler);
   for (const auto &I : *block) {
     if (Optional<CFGStmt> cs = I.getAs<CFGStmt>())
       tf.Visit(const_cast<Stmt *>(cs->getStmt()));
   }
   CFGTerminator terminator = block->getTerminator();
-  if (GCCAsmStmt *as = dyn_cast_or_null<GCCAsmStmt>(terminator.getStmt()))
+  if (auto *as = dyn_cast_or_null<GCCAsmStmt>(terminator.getStmt()))
     if (as->isAsmGoto())
       tf.Visit(as);
   return vals.updateValueVectorWithScratch(block);
 }
 
 namespace {
 
 /// PruneBlocksHandler is a special UninitVariablesHandler that is used

clang/test/Analysis/uninit-asm-goto.cpp

 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
     int y;
     asm goto("nop" : "=r"(y) : "r"(x) : : err);
     return y;
   err:
     return -1;
 }
+
+int test2(int x) {
+  int y; // expected-warning {{variable 'y' is used uninitialized whenever its declaration is reached}} \
+         // expected-note {{initialize the variable}}
+  if (x < 42)
+    asm volatile goto("testl %0, %0; testl %1, %2; jne %l3" : "+S"(x), "+D"(y) : "r"(x) :: indirect_1, indirect_2);
+  else
+    asm volatile goto("testl %0, %1; testl %2, %3; jne %l5" : "+S"(x), "+D"(y) : "r"(x), "r"(y) :: indirect_1, indirect_2);
+  return x + y;
+indirect_1:
+  return -42;
+indirect_2:
+  return y; // expected-note {{uninitialized use occurs here}}
+}
+
+int test3(int x) {
+  int y; // expected-warning {{variable 'y' is used uninitialized whenever its declaration is reached}} \
+         // expected-note {{initialize the variable}}
+  asm goto("xorl %1, %0; jmp %l2" : "=&r"(y) : "r"(x) : : fail);
+normal:
+  y += x;
+  return y;
+  if (x) {
+fail:
+    return y; // expected-note {{uninitialized use occurs here}}
+  }
+  return 0;
+}
+
+int test4(int x) {
+  int y; // expected-warning {{variable 'y' is used uninitialized whenever its declaration is reached}} \
+         // expected-note {{initialize the variable}}
+  goto forward;
+backward:
+  return y; // expected-note {{uninitialized use occurs here}}
+forward:
+  asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : backward);
+  return y;
+}
+
+// test5: Expect no diagnostics
+int test5(int x) {
+  int y;
+  asm volatile goto("testl %0, %0; testl %1, %2; jne %l3" : "+S"(x), "+D"(y) : "r"(x) :: indirect, fallthrough);
+fallthrough:
+  return y;
+indirect:
+  return -2;
+}