Page MenuHomePhabricator

[AST, analyzer] Transform rvalue cast outputs to lvalues (fheinous-gnu-extensions)
ClosedPublic

Authored by a.sidorin on Apr 8 2018, 9:49 AM.

Details

Summary

Despite the fact that cast expressions return rvalues, GCC still handles such outputs as lvalues when compiling inline assembler. In this commit, we are treating it by removing LValueToRValue casts inside GCCAsmStmt outputs.

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin created this revision.Apr 8 2018, 9:49 AM
george.karpenkov requested changes to this revision.Apr 9 2018, 11:05 AM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Core/ExprEngine.cpp
3082 ↗(On Diff #141551)

From my understanding, the code inside the if-block is not tested below

This revision now requires changes to proceed.Apr 9 2018, 11:05 AM
a.sidorin added inline comments.Apr 9 2018, 11:15 AM
lib/StaticAnalyzer/Core/ExprEngine.cpp
3082 ↗(On Diff #141551)

It is tested: without this code, "TRUE" will be printed. The reason is that the lvalue of 'global' is removed from Environment after the cast happens so X becomes Unknown. But the way of retrieving the value is definitely not the best so if you have any suggestions on improvement - they are welcome.

george.karpenkov accepted this revision.Apr 9 2018, 11:24 AM

Right, sorry. LGTM, but maybe Artem has something to add as well.
I don't have any suggestions, and trying to modifying lifetimes of expressions in environment for the sake of a GCC extensions seems suboptimal as well.

This revision is now accepted and ready to land.Apr 9 2018, 11:24 AM
NoQ accepted this revision.Apr 9 2018, 2:40 PM

Thanks!

Eww. Weird AST.

I wonder how this should work:

// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection -fheinous-gnu-extensions -w %s -verify

int clang_analyzer_eval(int);

int global;
void testRValueOutput() {
  int &ref = global;
  ref = 1;
  __asm__("" : "=r"((int)ref));
  clang_analyzer_eval(ref == 1); // currently says UNKNOWN
  clang_analyzer_eval(global == 1); // currently says TRUE
}

The ultimate solution would probably be to add a fake cloned asm statement to the CFG (instead of the real asm statement) that would point to the correct output child-expression(s) that are untouched themselves but simply have their noop casts removed.

Or we could try

The ultimate solution would probably be to add a fake cloned asm statement to the CFG (instead of the real asm statement) that would point to the correct output child-expression(s) that are untouched themselves but simply have their noop casts removed.

I have some concerns about this solution. It will result in difference between AST nodes and nodes that user will receive during analysis and I'm not sure that it is good. What is even worse here is that a lot of AST stuff won't work properly - ParentMap, for example.

Or we could try

Looks like something is missed here :)

Maybe we should just remove the condition and leave a FIXME?

NoQ added a comment.Apr 10 2018, 11:09 AM

The ultimate solution would probably be to add a fake cloned asm statement to the CFG (instead of the real asm statement) that would point to the correct output child-expression(s) that are untouched themselves but simply have their noop casts removed.

I have some concerns about this solution. It will result in difference between AST nodes and nodes that user will receive during analysis and I'm not sure that it is good. What is even worse here is that a lot of AST stuff won't work properly - ParentMap, for example.

Yep. But i'd rather avoid using the ParentMap. Also we already do this for DeclStmts that declare more than one VarDecl (split them up into single-decl statements), and the practical effect of such simplification is barely noticeable even though DeclStmts are so much more common.

Or we could try

Looks like something is missed here :)

Whoops sry nvm. And and and mmm you didn't include the diff context :p

Maybe we should just remove the condition and leave a FIXME?

The fix is already there and it's valid and it makes things better overall, why not keep it around. We still need a FIXME though.

NoQ added a comment.Apr 10 2018, 8:02 PM

I mean, like, if we try to work with the existing AST then we're stuck with a prvalue expression that represents an lvalue and will be assigned a Loc value, which is pretty weird anyway. Getting rid of the ParentMap in favor of providing enough context (eg. in the CFG or in checker callbacks) whenever we might want to use it sounds like a much saner solution. There might be other "unknown unknowns" about rewriting the AST, but our current problem looks as safe as we'll ever get.

Rewrite the GCCAsmStmt in the CFG.

NoQ accepted this revision.Apr 13 2018, 8:31 PM

Wow, you actually did that.

Ok, now we can decide if we want this to be analyzer-only (with a CFG::BuildOptions flag) or get someone else to have a look at that as a global CFG change (i.e. it may potentially affect compiler warnings). Or at least make sure to run all clang tests :)

NoQ added a comment.Apr 13 2018, 8:32 PM

(i'd much rather do the latter)

NoQ added a comment.Apr 16 2018, 2:44 PM

(also we'll need CFG dump tests)

Add a test for CFG dump; replace static_cast with an initialization.
No test failures on check-all were observed.

NoQ accepted this revision.Apr 27 2018, 5:05 PM
NoQ added a subscriber: rsmith.

Woohoo LGTM.

Heads up to @rsmith because we're about to break the CFG again, and also yay we've found another use case for rewriting AST in our CFG.

rsmith added inline comments.Apr 27 2018, 5:15 PM
test/Analysis/asm.cpp
9 ↗(On Diff #143959)

Ugh, do we really need to support this nonsense? :(

If so, it'd seem reasonable to me to do the cleanup when building the AST rather than in the CFG builder (that is, convert the LValueToRValue cast here to a NoOp lvalue cast node, since that matches its actual semantics).

a.sidorin updated this revision to Diff 144998.May 3 2018, 4:18 AM
a.sidorin retitled this revision from [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs to [AST, analyzer] Transform rvalue cast outputs to lvalues (fheinous-gnu-extensions).
a.sidorin edited the summary of this revision. (Show Details)
a.sidorin added a reviewer: rsmith.

Transform AST instead.
I don't remove analyzer guys from reviewers because the test is still for the analyzer.
@rsmith Feel free to correct me if I'm doing something wrong.

NoQ added a comment.Sep 6 2018, 5:44 PM

Dunno, i guess you can just commit it.

lib/Sema/SemaStmtAsm.cpp
57 ↗(On Diff #144998)

Like father, like son :D

NoQ accepted this revision.Sep 6 2018, 5:44 PM
This revision was automatically updated to reflect the committed changes.