[analyzer] Fix clang-tidy crash on GCCAsmStmt

[Nathan-Huckleberry]

Summary

Added entry in switch statement to recognize GCCAsmStmt
as a possible block terminator.

Handling to build CFG using GCCAsmStmt was already implemented.

[NoQ]

Hey, thanks! That's a nice catch.

The code looks fine, but i don't think your test actually tests it - see inline comments. Also i think we should put the test into the clang repo (i.e., test/Analysis), because that's where the change is. I'd like to know it if i break your test even if i don't enable clang-tools-extra at all. Because clang-tidy isn't available from within the clang sub-project, this would have to be implemented as a static analyzer test.

clang-tools-extra/test/clang-tidy/asm-goto.cpp

2 ↗

(On Diff #205478)

I suspect that the code you modified doesn't get covered by this test unless you enable analyzer-... checkers.

3–4 ↗

(On Diff #205478)

This is scary because the behavior may depend on the implementation details of the standard library that's installed on the current machine. In the Static Analyzer we use test/Analysis/Inputs/system-header-simulator*.h as a mocked-up standard library for testing purposes.

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

401 ↗

(On Diff #205478)

Please add a TODO to actually implement this functionality.

[nickdesaulniers]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

401 ↗

(On Diff #205478)

And an assert that this Stmt::isAsmGoto() == true. (We should not hit this for any inline assembly, just asm goto statements)

[Nathan-Huckleberry]

• [analyzer] Revise test case and add TODO

[NoQ]

clang/test/Analysis/egraph-asm-goto-no-crash.cpp

1 ↗

(On Diff #205642)

Ugh, you picked an exotic test as an example.

Let's try the following:

// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s

// expected-no-diagnostics

void clang_analyzer_warnIfReached();

void testAsmGoto() {
  asm goto("xor %0, %0\n je %l[label1]\n jl %l[label2]"
           : /* no outputs */
           : /* inputs */
           : /* clobbers */
           : label1, label2 /* any labels used */);

  label1:
  // FIXME: Should be reachable.
  clang_analyzer_warnIfReached();
  return;

  label2:
  // FIXME: Should be reachable.
  clang_analyzer_warnIfReached();
  return;
}

(and the egraph part in the main file is also out of place)

[NoQ]

clang/test/Analysis/egraph-asm-goto-no-crash.cpp

1 ↗

(On Diff #205642)

(wait, one of these shouldn't be reachable, right?)

[NoQ]

clang/test/Analysis/egraph-asm-goto-no-crash.cpp

1 ↗

(On Diff #205642)

(i mean, let's do something similar, just with the correct amount of FIXMEs)

[nickdesaulniers]

clang/test/Analysis/egraph-asm-goto-no-crash.cpp

1 ↗

(On Diff #205642)

You'd have to "peak" into the assembly to tell. Essentially asm goto is treated as a "black box" throughout Clang and LLVM, similar to vanilla inline assembly. Basically, the explicit list of labels are valid branch targets from the inline assembly, as is fallthrough. It's undefined behavior if the assembly jumps to a label not explicitly listed in the asm statement (but would likely fail to link, in the best case).

[nickdesaulniers]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

401 ↗

(On Diff #205478)

Take a look at other asserts in the codebase. They usually are in the form: assert(some_expr_should_be_true && "helpful error message when false");

[Nathan-Huckleberry]

clang/test/Analysis/egraph-asm-goto-no-crash.cpp

1 ↗

(On Diff #205642)

To answer the original question both labels and the 'fallthrough' case should all be technically reachable in this test case, but will not actually be reached during analysis since handling for asm goto branching doesn't exist.

[tmroeder]

I can confirm that this fixes the clang-tidy crash I observed in trying to analyze the kernel.

[xbolva00]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

401 ↗

(On Diff #205478)

or llvm_unreachable :)

[Nathan-Huckleberry]

• Add assertion message and simplify test case

[NoQ]

Looks great now, thanks!

[nickdesaulniers]

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

401 ↗

(On Diff #206919)

prefer: assert(shouldBeTrue() && "asdfasdf") to assert(shouldBeTrue() == true && "asfas").

[Nathan-Huckleberry]

• Minor style change on assert

cfe/trunk/lib/StaticAnalyzer/Core/CoreEngine.cpp

switch (Term->getStmtClass()) {
 
         SubEng.processSwitch(builder);
         return;
       }
 
       case Stmt::WhileStmtClass:
         HandleBranch(cast<WhileStmt>(Term)->getCond(), Term, B, Pred);
         return;
+
+      case Stmt::GCCAsmStmtClass:
+        assert(cast<GCCAsmStmt>(Term)->isAsmGoto() && "Encountered GCCAsmStmt without labels");
+        // TODO: Handle jumping to labels
+        return;
     }
   }
 
   if (B->getTerminator().isVirtualBaseBranch()) {
     HandleVirtualBaseBranch(B, Pred);
     return;
   }
 

cfe/trunk/test/Analysis/egraph-asm-goto-no-crash.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// expected-no-diagnostics
+
+void clang_analyzer_warnIfReached();
+
+void testAsmGoto() {
+  asm goto("xor %0, %0\n je %l[label1]\n jl %l[label2]"
+           : /* no outputs */
+           : /* inputs */
+           : /* clobbers */
+           : label1, label2 /* any labels used */);
+
+  // FIXME: Should be reachable.
+  clang_analyzer_warnIfReached();
+
+  label1:
+  // FIXME: Should be reachable.
+  clang_analyzer_warnIfReached();
+  return;
+
+  label2:
+  // FIXME: Should be reachable.
+  clang_analyzer_warnIfReached();
+  return;
+}