[Clang][CFG] check children statements of asm goto

[nickdesaulniers]

Summary

When performing CFG based analyses, don't forget to check the child
statements of an asm goto, such as the expressions used for
inputs+outputs.

Fixes: https://github.com/llvm/llvm-project/issues/51024
Fixes: https://github.com/ClangBuiltLinux/linux/issues/1439

[nickdesaulniers]

clang/lib/Analysis/CFG.cpp

3356

I think this should actually be return VisitChildren(G);; it produces a simpler CFG with the asm goto Node marked as the terminator directly. Will update this and the test.

[nickdesaulniers]

• VisitChildren rather than VisitStmt

[efriedma]

clang/lib/Analysis/UninitializedValues.cpp

828

The old and the new code here seem to be doing very different things. It isn't clear why.

Actually, I'm not sure why we're checking whether the variable is initialized. Whether the variable is initialized going into the inline asm isn't really related to whether it's initialized when we exit the inline asm. For register outputs, the output isn't even in the same register as the old value of the variable.

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

828

I suspect we're now eagerly marking variables initialized by visiting the child nodes, which we weren't doing previously.

See also: https://reviews.llvm.org/D71314.

[void]

clang/lib/Analysis/UninitializedValues.cpp

828

I'm very confused as well. You're basically going from a strong statement - (This variable is initialized) - to a weaker statement - (This variable may be uninitialized). That doesn't make a lot of sense to me. Could you explain it better in the comment?

[nickdesaulniers]

• don't bother checking if a Val is Initialized

[efriedma]

Do we have testcases for how -Wuninitialized/-Wmaybe-uninitialized interact with asm goto? If not, can you add them?

Also, an explicit test for -Warray-bounds or whatever would be nice, although I guess the CFG tests sort of cover that.

[nickdesaulniers]

In D116059#3225862, @efriedma wrote:

Do we have testcases for how -Wuninitialized/-Wmaybe-uninitialized interact with asm goto?

We have tests for -Wuninitialized; clang/test/Analysis/uninit-asm-goto.cpp (D71314).

Clang doesn't implement -Wmaybe-unitialized; perhaps you're thinking of -Wsometimes-unitialized?

clang/test/Analysis/uninit-sometimes.cpp does not contain any asm goto tests (for -Wsometimes-unitialized).

If not, can you add them?

-Wsometimes-unitialized is part of the diag group for -Wunitialized. Adding another RUN line to clang/test/Analysis/uninit-asm-goto.cpp that tests -Wsometimes-unitialized rather than -Wunitialized produces the same result, so unless there's a different case you can think of, I don't see testing -Wsometimes-unitialized w/ asm goto as improving our test coverage at all.

Also, an explicit test for -Warray-bounds or whatever would be nice, although I guess the CFG tests sort of cover that.

Sure, I can add one.

[efriedma]

Yes, messed up the warning flag.

Did some brief experiments on trunk; I'd like to see the following two testcases as regression tests:

int a(int z) {
    int x;
    if (z)
      asm goto ("":"=r"(x):::A1,A2);
    return 0;
    A1:
    A2:
    return x; // expected warning: conditional use of uninitialized var
}

int b() {
    int x = 0;
    asm goto ("":"=r"(x):::A1,A2);
    return 0;
    A1:
    A2:
    return x; // expected warning: use of uninitialized variable
}

[nickdesaulniers]

• add previously red -Warray-bounds test

[nickdesaulniers]

will add more tests

[nickdesaulniers]

• MOAR TESTS RAWR!!1one

[efriedma]

LGTM

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

84

Looks like this patch fixes the false negative on test8; that's good.

It looks like there's a issue with the printed diagnostic on a bunch of these tests; we say the variable is used uninitialized "whenever its declaration is reached", which isn't right. We can probably leave that for a followup; this is clearly an improvement. But please file a bug.

[nickdesaulniers]

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

84

https://github.com/llvm/llvm-project/issues/53060

[nathanchance]

The two false positive warnings in the kernel are fixed with this patch and there were no new warnings introduced.

[jyu2]

Looks good to me. Thanks.

clang/lib/Analysis/CFG.cpp

CFGBlock *CFGBuilder::VisitGCCAsmStmt(GCCAsmStmt *G, AddStmtChoice asc) {
   }
   Block = createBlock();
   Block->setTerminator(G);
   // We will backpatch this block later for all the labels.
   BackpatchBlocks.push_back(JumpSource(Block, ScopePos));
   // Save "Succ" in BackpatchBlocks. In the backpatch processing, "Succ" is
   // used to avoid adding "Succ" again.
   BackpatchBlocks.push_back(JumpSource(Succ, ScopePos));
-  return Block;
+  return VisitChildren(G);
 }
 
 CFGBlock *CFGBuilder::VisitForStmt(ForStmt *F) {
   CFGBlock *LoopSuccessor = nullptr;
 
   // Save local scope position because in case of condition variable ScopePos
   // won't be restored when traversing AST.
   SaveAndRestore<LocalScope::const_iterator> save_scope_pos(ScopePos);
 

clang/lib/Analysis/UninitializedValues.cpp

void TransferFunctions::VisitGCCAsmStmt(GCCAsmStmt *as) {
   for (const Expr *O : as->outputs()) {
     const Expr *Ex = stripCasts(C, O);
 
     // Strip away any unary operators. Invalid l-values are reported by other
     // semantic analysis passes.
     while (const auto *UO = dyn_cast<UnaryOperator>(Ex))
       Ex = stripCasts(C, UO->getSubExpr());
 
+    // Mark the variable as potentially uninitialized for those cases where
+    // it's used on an indirect path, where it's not guaranteed to be
+    // defined.
     if (const VarDecl *VD = findVar(Ex).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) {
   // 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);
   }
 }

clang/test/Analysis/asm-goto.cpp

-// RUN: %clang_analyze_cc1  -triple i386-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1  -triple x86_64-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
+// RUN: %clang_analyze_cc1 -triple i386-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
+// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu -analyzer-checker=debug.DumpCFG %s 2>&1 | FileCheck %s
 
 int foo(int cond)
 {
 label_true:
   asm goto("testl %0, %0; jne %l1;" :: "r"(cond)::label_true, loop);
   return 0;
 loop:
   return 0;
 }
 
 // CHECK-LABEL: loop
 // CHECK-NEXT: 0
 // CHECK-NEXT: return
 // CHECK-NEXT: Preds (1): B3
 // CHECK-NEXT: Succs (1): B0
 
 // CHECK-LABEL: label_true
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: cond
+// CHECK-NEXT: [B3.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (2): B3 B4
 // CHECK-NEXT: Succs (3): B2 B3 B1
 
-
 int bar(int cond)
 {
   asm goto("testl %0, %0; jne %l1;" :: "r"(cond)::L1, L2);
   return 0;
 L1:
 L2:
   return 0;
 }
 
 // CHECK: [B4]
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: cond
+// CHECK-NEXT: [B4.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (1): B5
 // CHECK-NEXT: Succs (3): B3 B2 B1
 
 int zoo(int n)
 {
 A5:
 A1:
   asm goto("testl %0, %0; jne %l1;" :: "r"(n)::A1, A2, A3, A4, A5);
 A2:
 A3:
 A4:
   return 0;
 }
 
 // CHECK-LABEL: A1
-// CHECK-NEXT: asm goto
+// CHECK-NEXT: n
+// CHECK-NEXT: [B4.1]
+// CHECK-NEXT: T: asm goto
 // CHECK-NEXT: Preds (2): B5 B4
 // CHECK-NEXT: Succs (5): B3 B4 B2 B1 B5
+
+void baz(void)
+{
+  asm goto("" :: "r"(1 ? 2 : 0 << -1) :: error);
+error:;
+}
+
+// CHECK: [B2]
+// CHECK-NEXT: 1: [B5.2] ? [B3.1] : [B4.4]
+// CHECK-NEXT: T: asm goto ("" :  : "r" ([B2.1]) :  : error);
+// CHECK-NEXT: Preds (2): B3 B4
+// CHECK-NEXT: Succs (1): B1

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

 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -std=c++11 -Wuninitialized -verify %s
 
 // test1: Expect no diagnostics
 int test1(int x) {
     int y;
-    asm goto("nop" : "=r"(y) : "r"(x) : : err);
+    asm goto("" : "=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}}
+  int y; // expected-warning {{variable 'y' is used uninitialized whenever its declaration is reached}}
+         // expected-note@-1 {{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);
+    asm goto("" : "+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);
+    asm goto("" : "+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);
+  int y; // expected-warning {{variable 'y' is used uninitialized whenever its declaration is reached}}
+         // expected-note@-1 {{initialize the variable}}
+  asm goto("" : "=&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}}
+  int y; // expected-warning {{variable 'y' is used uninitialized whenever its declaration is reached}}
+         // expected-note@-1 {{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);
+  asm goto("" : "=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);
+  asm goto("" : "+S"(x), "+D"(y) : "r"(x) :: indirect, fallthrough);
 fallthrough:
   return y;
 indirect:
   return -2;
 }
 
 // test6: Expect no diagnostics.
 int test6(unsigned int *x) {
   unsigned int val;
 
   // See through casts and unary operators.
-  asm goto("nop" : "=r" (*(unsigned int *)(&val)) ::: indirect);
+  asm goto("" : "=r" (*(unsigned int *)(&val)) ::: indirect);
   *x = val;
   return 0;
 indirect:
   return -1;
 }
+
+int test7(int z) {
+    int x; // expected-warning {{variable 'x' is used uninitialized whenever its declaration is reached}}
+           // expected-note@-1 {{initialize the variable 'x' to silence this warning}}
+    if (z)
+      asm goto ("":"=r"(x):::A1,A2);
+    return 0;
+    A1:
+    A2:
+    return x; // expected-note {{uninitialized use occurs here}}
+}
+
+int test8() {
+    int x = 0; // expected-warning {{variable 'x' is used uninitialized whenever its declaration is reached}}
+               // expected-note@-1 {{variable 'x' is declared here}}
+    asm goto ("":"=r"(x):::A1,A2);
+    return 0;
+    A1:
+    A2:
+    return x; // expected-note {{uninitialized use occurs here}}
+}

clang/test/Sema/array-bounds-ptr-arith.c

 typedef TPtr *TabHandle;
 struct RDar11387038_B { TabHandle x; };
 typedef struct RDar11387038_B RDar11387038_B;
 
 void radar11387038() {
   RDar11387038_B *pRDar11387038_B;
   struct RDar11387038* y = &(*pRDar11387038_B->x)->z[4];
 }
+
+void pr51682 (void) {
+  int arr [1];
+  switch (0) {
+    case 0:
+      break;
+    case 1:
+      asm goto (""::"r"(arr[42] >> 1)::failed); // no-warning
+      break;
+  }
+failed:;
+}