[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 246363.
void added a comment.

Explain what's going on in VisitGCCAsmStmt better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314

Files:
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/uninit-asm-goto.cpp

Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,52 @@
   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" : "="(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;
+}
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -576,6 +576,28 @@
   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(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  = SuccsVisited[Pred->getBlockID()];
 if (!SV) {
   // When visiting the first successor of a block, mark all NULL
@@ -767,8 +789,12 @@
 return;
 
   for (const auto  : as->outputs())
-if (const VarDecl *VD = findVar(o).getDecl())
-  vals[VD] = Initialized;
+if (const auto *VD = findVar(o).getDecl())
+  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) {
@@ -809,7 +835,7 @@
   tf.Visit(const_cast(cs->getStmt()));
   }
   CFGTerminator terminator = block->getTerminator();
-  if (GCCAsmStmt *as = dyn_cast_or_null(terminator.getStmt()))
+  if (auto *as = dyn_cast_or_null(terminator.getStmt()))
 if (as->isAsmGoto())
   tf.Visit(as);
   return vals.updateValueVectorWithScratch(block);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void marked an inline comment as done.
void added inline comments.



Comment at: clang/lib/Analysis/UninitializedValues.cpp:856
+vals[VD] = MayUninitialized;
 }
 

nickdesaulniers wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 245063.
void added a comment.

Variable can't be 'const'


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314

Files:
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/uninit-asm-goto.cpp

Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,52 @@
   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" : "="(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;
+}
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -576,6 +576,28 @@
   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(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  = SuccsVisited[Pred->getBlockID()];
 if (!SV) {
   // When visiting the first successor of a block, mark all NULL
@@ -767,8 +789,9 @@
 return;
 
   for (const auto  : as->outputs())
-if (const VarDecl *VD = findVar(o).getDecl())
-  vals[VD] = Initialized;
+if (const auto *VD = findVar(o).getDecl())
+  if (vals[VD] != Initialized)
+vals[VD] = MayUninitialized;
 }
 
 void TransferFunctions::VisitObjCMessageExpr(ObjCMessageExpr *ME) {
@@ -809,7 +832,7 @@
   tf.Visit(const_cast(cs->getStmt()));
   }
   CFGTerminator terminator = block->getTerminator();
-  if (GCCAsmStmt *as = dyn_cast_or_null(terminator.getStmt()))
+  if (auto *as = dyn_cast_or_null(terminator.getStmt()))
 if (as->isAsmGoto())
   tf.Visit(as);
   return vals.updateValueVectorWithScratch(block);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 244844.
void added a comment.
Herald added a subscriber: martong.

Use "auto" and change labels to clarify test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314

Files:
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/uninit-asm-goto.cpp

Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- /dev/null
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
+
+// test1: Expect no diagnostics
+int test1(int x) {
+int y;
+asm goto("# %0 %1 %2" : "=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" : "="(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) :: fallthrough_1, fallthrough_2);
+fallthrough_1:
+  return y;
+fallthrough_2:
+  return -2;
+}
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -575,6 +575,28 @@
   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(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  = SuccsVisited[Pred->getBlockID()];
 if (!SV) {
   // When visiting the first successor of a block, mark all NULL
@@ -760,6 +782,17 @@
   }
 }
 
+void TransferFunctions::VisitGCCAsmStmt(GCCAsmStmt *as) {
+  // An "asm goto" statement is a terminator that may initialize some variables.
+  if (!as->isAsmGoto())
+return;
+
+  for (const auto  : as->outputs())
+if (const auto *VD = findVar(o).getDecl())
+  if (vals[VD] != Initialized)
+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.
@@ -797,6 +830,10 @@
 if (Optional cs = I.getAs())
   tf.Visit(const_cast(cs->getStmt()));
   }
+  CFGTerminator terminator = block->getTerminator();
+  if (const auto *as = dyn_cast_or_null(terminator.getStmt()))
+if (as->isAsmGoto())
+  tf.Visit(as);
   return vals.updateValueVectorWithScratch(block);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Analysis/UninitializedValues.cpp:856
+vals[VD] = MayUninitialized;
 }
 

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?



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:57
+  return y;
+indirect:
+  return -2;

nickdesaulniers wrote:
> I think if you left out `indirect`, it would be clearer what this test is 
> testing.
bump


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Analysis/UninitializedValues.cpp:856
   if (!as->isAsmGoto())
 return;
 

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > Should we mark the vals `MayUninitialized` here, rather than below? Then I 
> > think we can remove the `isAsmGoto` check you added in `getUninitUse`?
> Bumping comment.
Ah, I see this was added in https://reviews.llvm.org/D69876.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

> Use -Wuninitialized.

D'oh!




Comment at: clang/lib/Analysis/UninitializedValues.cpp:856
   if (!as->isAsmGoto())
 return;
 

nickdesaulniers wrote:
> Should we mark the vals `MayUninitialized` here, rather than below? Then I 
> think we can remove the `isAsmGoto` check you added in `getUninitUse`?
Bumping comment.



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:57
+  return y;
+indirect:
+  return -2;

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 234657.
void added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314

Files:
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/uninit-asm-goto.cpp

Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,52 @@
   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" : "="(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;
+}
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -637,6 +637,28 @@
   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 (GCCAsmStmt *as = dyn_cast_or_null(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  = SuccsVisited[Pred->getBlockID()];
 if (!SV) {
   // When visiting the first successor of a block, mark all NULL
@@ -829,7 +851,8 @@
 
   for (const auto  : as->outputs())
 if (const VarDecl *VD = findVar(o).getDecl())
-  vals[VD] = Initialized;
+  if (vals[VD] != Initialized)
+vals[VD] = MayUninitialized;
 }
 
 void TransferFunctions::VisitObjCMessageExpr(ObjCMessageExpr *ME) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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;
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 233738.
void marked 3 inline comments as done.
void added a comment.

Use "any_of" and improve tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314

Files:
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/uninit-asm-goto.cpp


Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,42 @@
   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" : "="(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;
+}
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -637,6 +637,26 @@
   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 (GCCAsmStmt *as = dyn_cast_or_null(term.getStmt())) {
+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;
+});
+})) {
+  Use.setUninitAfterDecl();
+  continue;
+}
+  }
+}
+
 unsigned  = SuccsVisited[Pred->getBlockID()];
 if (!SV) {
   // When visiting the first successor of a block, mark all NULL
@@ -829,7 +849,8 @@
 
   for (const auto  : as->outputs())
 if (const VarDecl *VD = findVar(o).getDecl())
-  vals[VD] = Initialized;
+  if (vals[VD] != Initialized)
+vals[VD] = MayUninitialized;
 }
 
 void TransferFunctions::VisitObjCMessageExpr(ObjCMessageExpr *ME) {


Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,42 @@
   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 

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:16
+  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

nickdesaulniers wrote:
> 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);
> ```
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.



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:38
+  return 0;
+}

nickdesaulniers wrote:
> 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;
> ```
Yes, we should be able to warn about this. I added a testcase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Analysis/UninitializedValues.cpp:651
+  continue;
+for (const auto  : as->labels()) {
+  const LabelStmt *ls = label->getLabel()->getStmt();

Is this more concise with `llvm::any_of`?



Comment at: clang/lib/Analysis/UninitializedValues.cpp:856
   if (!as->isAsmGoto())
 return;
 

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



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:16
+  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

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);
```



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:38
+  return 0;
+}

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;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71314/new/

https://reviews.llvm.org/D71314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Bill Wendling via Phabricator via cfe-commits]

void created this revision.
void added reviewers: jyknight, nickdesaulniers, hfinkel.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71314

Files:
  clang/lib/Analysis/UninitializedValues.cpp
  clang/test/Analysis/uninit-asm-goto.cpp


Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,31 @@
   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 foo(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" : "="(y) : "r"(x) : : fail);
+normal:
+  y += x;
+  return y;
+  if (x) {
+fail:
+return y; // expected-note {{uninitialized use occurs here}}
+  }
+  return 0;
+}
Index: clang/lib/Analysis/UninitializedValues.cpp
===
--- clang/lib/Analysis/UninitializedValues.cpp
+++ clang/lib/Analysis/UninitializedValues.cpp
@@ -637,6 +637,34 @@
   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 terminator = Pred->getTerminator();
+  if (GCCAsmStmt *as = 
dyn_cast_or_null(terminator.getStmt()))
+if (as->isAsmGoto()) {
+  bool uninitedAfterDecl = false;
+  for (const auto  : as->outputs()) {
+if (vd != findVar(o).getDecl())
+  continue;
+for (const auto  : as->labels()) {
+  const LabelStmt *ls = label->getLabel()->getStmt();
+  if (ls == B->Label) {
+uninitedAfterDecl = true;
+break;
+  }
+}
+if (uninitedAfterDecl)
+  break;
+  }
+  if (uninitedAfterDecl) {
+Use.setUninitAfterDecl();
+continue;
+  }
+}
+}
+
 unsigned  = SuccsVisited[Pred->getBlockID()];
 if (!SV) {
   // When visiting the first successor of a block, mark all NULL
@@ -829,7 +857,8 @@
 
   for (const auto  : as->outputs())
 if (const VarDecl *VD = findVar(o).getDecl())
-  vals[VD] = Initialized;
+  if (vals[VD] != Initialized)
+vals[VD] = MayUninitialized;
 }
 
 void TransferFunctions::VisitObjCMessageExpr(ObjCMessageExpr *ME) {


Index: clang/test/Analysis/uninit-asm-goto.cpp
===
--- clang/test/Analysis/uninit-asm-goto.cpp
+++ clang/test/Analysis/uninit-asm-goto.cpp
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wuninitialized -verify %s
-// expected-no-diagnostics
 
+// test1: Expect no diagnostics
 int test1(int x) {
 int y;
 asm goto("# %0 %1 %2" : "=r"(y) : "r"(x) : : err);
@@ -8,3 +8,31 @@
   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 foo(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" : "="(y) : "r"(x) : : fail);
+normal:
+  y += x;
+  return y;
+  if (x) {
+fail:
+return y; // expected-note {{uninitialized use occurs here}}
+  }
+  return