This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix clang-tidy crash on GCCAsmStmt
ClosedPublic

Authored by Nathan-Huckleberry on Jun 18 2019, 5:06 PM.

Details

Summary

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

Handling to build CFG using GCCAsmStmt was already implemented.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2019, 5:06 PM
Szelethus edited reviewers, added: NoQ; removed: dergachev.a.Jun 18 2019, 5:15 PM
NoQ added a comment.Jun 18 2019, 5:54 PM

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.

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)

  • [analyzer] Revise test case and add TODO
NoQ added inline comments.Jun 19 2019, 1:50 PM
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 added inline comments.Jun 19 2019, 1:54 PM
clang/test/Analysis/egraph-asm-goto-no-crash.cpp
1 ↗(On Diff #205642)

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

NoQ added inline comments.Jun 19 2019, 1:56 PM
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)

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

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 marked an inline comment as done.Jun 19 2019, 2:41 PM
Nathan-Huckleberry added inline comments.
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.

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

xbolva00 added inline comments.
clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
401 ↗(On Diff #205478)

or llvm_unreachable :)

Nathan-Huckleberry marked an inline comment as not done.Jun 20 2019, 9:48 AM
  • Add assertion message and simplify test case
NoQ accepted this revision.Jun 27 2019, 1:38 PM

Looks great now, thanks!

This revision is now accepted and ready to land.Jun 27 2019, 1:38 PM
clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
401 ↗(On Diff #206919)

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

  • Minor style change on assert
nickdesaulniers accepted this revision.Jun 27 2019, 3:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 3:52 PM