[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

3360

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

826

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

826

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

826

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 ↗

(On Diff #397983)

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 ↗

(On Diff #397983)

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

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());
 
     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.
+      if (vals[VD] == Initialized)
+        // If the variable is 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)) {

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