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

[a.sidorin]

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.

[george.karpenkov]

lib/StaticAnalyzer/Core/ExprEngine.cpp

3082

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

[a.sidorin]

lib/StaticAnalyzer/Core/ExprEngine.cpp

3082

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]

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.

[NoQ]

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

[a.sidorin]

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

[a.sidorin]

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

[NoQ]

In D45416#1062901, @a.sidorin wrote:

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

In D45416#1063366, @a.sidorin wrote:

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]

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.

[a.sidorin]

Rewrite the GCCAsmStmt in the CFG.

[NoQ]

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]

(i'd much rather do the latter)

[NoQ]

(also we'll need CFG dump tests)

[a.sidorin]

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

[NoQ]

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]

test/Analysis/asm.cpp

10

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]

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.

[a.sidorin]

Gentle ping.

[NoQ]

Dunno, i guess you can just commit it.

lib/Sema/SemaStmtAsm.cpp

57 ↗

(On Diff #144998)

Like father, like son :D

include/clang/Analysis/CFG.h

public:
 
   /// Records a synthetic DeclStmt and the DeclStmt it was constructed from.
   ///
   /// The CFG uses synthetic DeclStmts when a single AST DeclStmt contains
   /// multiple decls.
   void addSyntheticDeclStmt(const DeclStmt *Synthetic,
                             const DeclStmt *Source) {
     assert(Synthetic->isSingleDecl() && "Can handle single declarations only");
-    assert(Synthetic != Source && "Don't include original DeclStmts in map");
-    assert(!SyntheticDeclStmts.count(Synthetic) && "Already in map");
-    SyntheticDeclStmts[Synthetic] = Source;
+    addSyntheticStmt(Synthetic, Source);
+  }
+
+  /// Records a synthetic Stmt and the Stmt it was constructed from.
+  void addSyntheticStmt(const Stmt *Synthetic, const Stmt *Source) {
+    assert(Synthetic != Source && "Don't include original Stmts in map");
+    assert(!SyntheticStmts.count(Synthetic) && "Already in map");
+    SyntheticStmts[Synthetic] = Source;
   }
 
   using synthetic_stmt_iterator =
-      llvm::DenseMap<const DeclStmt *, const DeclStmt *>::const_iterator;
+      llvm::DenseMap<const Stmt *, const Stmt *>::const_iterator;
   using synthetic_stmt_range = llvm::iterator_range<synthetic_stmt_iterator>;
 
   /// Iterates over synthetic DeclStmts in the CFG.
   ///
   /// Each element is a (synthetic statement, source statement) pair.
   ///
   /// \sa addSyntheticDeclStmt
   synthetic_stmt_iterator synthetic_stmt_begin() const {
-    return SyntheticDeclStmts.begin();
+    return SyntheticStmts.begin();
   }
 
   /// \sa synthetic_stmt_begin
   synthetic_stmt_iterator synthetic_stmt_end() const {
-    return SyntheticDeclStmts.end();
+    return SyntheticStmts.end();
   }
 
   /// \sa synthetic_stmt_begin
   synthetic_stmt_range synthetic_stmts() const {
     return synthetic_stmt_range(synthetic_stmt_begin(), synthetic_stmt_end());
   }
 
   //===--------------------------------------------------------------------===//
private:
   CFGBlockListTy Blocks;
 
   /// C++ 'try' statements are modeled with an indirect dispatch block.
   /// This is the collection of such blocks present in the CFG.
   std::vector<const CFGBlock *> TryDispatchBlocks;
 
   /// Collects DeclStmts synthesized for this CFG and maps each one back to its
   /// source DeclStmt.
-  llvm::DenseMap<const DeclStmt *, const DeclStmt *> SyntheticDeclStmts;
+  llvm::DenseMap<const Stmt *, const Stmt *> SyntheticStmts;
 };
 
 } // namespace clang
 
 //===----------------------------------------------------------------------===//
 // GraphTraits specializations for CFG basic block graphs (source-level CFGs)
 //===----------------------------------------------------------------------===//
 

lib/Analysis/CFG.cpp

private:
   CFGBlock *VisitCXXThrowExpr(CXXThrowExpr *T);
   CFGBlock *VisitCXXTryStmt(CXXTryStmt *S);
   CFGBlock *VisitDeclStmt(DeclStmt *DS);
   CFGBlock *VisitDeclSubExpr(DeclStmt *DS);
   CFGBlock *VisitDefaultStmt(DefaultStmt *D);
   CFGBlock *VisitDoStmt(DoStmt *D);
   CFGBlock *VisitExprWithCleanups(ExprWithCleanups *E, AddStmtChoice asc);
   CFGBlock *VisitForStmt(ForStmt *F);
+  CFGBlock *VisitGCCAsmStmt(GCCAsmStmt *GCCAsmS, AddStmtChoice asc);
   CFGBlock *VisitGotoStmt(GotoStmt *G);
   CFGBlock *VisitIfStmt(IfStmt *I);
   CFGBlock *VisitImplicitCastExpr(ImplicitCastExpr *E, AddStmtChoice asc);
   CFGBlock *VisitIndirectGotoStmt(IndirectGotoStmt *I);
   CFGBlock *VisitLabelStmt(LabelStmt *L);
   CFGBlock *VisitBlockExpr(BlockExpr *E, AddStmtChoice asc);
   CFGBlock *VisitLambdaExpr(LambdaExpr *E, AddStmtChoice asc);
   CFGBlock *VisitLogicalOperator(BinaryOperator *B);
private:
   CFGBlock *VisitUnaryOperator(UnaryOperator *U, AddStmtChoice asc);
   CFGBlock *VisitWhileStmt(WhileStmt *W);
 
   CFGBlock *Visit(Stmt *S, AddStmtChoice asc = AddStmtChoice::NotAlwaysAdd);
   CFGBlock *VisitStmt(Stmt *S, AddStmtChoice asc);
   CFGBlock *VisitChildren(Stmt *S);
   CFGBlock *VisitNoRecurse(Expr *E, AddStmtChoice asc);
 
+  GCCAsmStmt *getOrCreateGCCAsmStmtWithoutGnuExtensions(GCCAsmStmt *GCCAsmS);
+
+  template <typename StmtTy> void *allocateMemForStmt() {
+    // Get the alignment of the new Stmt, padding out to >=8 bytes.
+    unsigned Align = std::max(alignof(StmtTy), static_cast<size_t>(8));
+    // Allocate a new Stmt using the BumpPtrAllocator. It will get
+    // automatically freed with the CFG.
+    return cfg->getAllocator().Allocate(sizeof(StmtTy), Align);
+  }
+
   void maybeAddScopeBeginForVarDecl(CFGBlock *B, const VarDecl *VD,
                                     const Stmt *S) {
     if (ScopePos && (VD == ScopePos.getFirstVarInScope()))
       appendScopeBegin(B, VD, S);
   }
 
   /// When creating the CFG for temporary destructors, we want to mirror the
   /// branch structure of the corresponding constructor calls.
case Stmt::DefaultStmtClass:
       return VisitDefaultStmt(cast<DefaultStmt>(S));
 
     case Stmt::DoStmtClass:
       return VisitDoStmt(cast<DoStmt>(S));
 
     case Stmt::ForStmtClass:
       return VisitForStmt(cast<ForStmt>(S));
 
+    case Stmt::GCCAsmStmtClass:
+      return VisitGCCAsmStmt(cast<GCCAsmStmt>(S), asc);
+
     case Stmt::GotoStmtClass:
       return VisitGotoStmt(cast<GotoStmt>(S));
 
     case Stmt::IfStmtClass:
       return VisitIfStmt(cast<IfStmt>(S));
 
     case Stmt::ImplicitCastExprClass:
       return VisitImplicitCastExpr(cast<ImplicitCastExpr>(S), asc);
if (DS->isSingleDecl())
     return VisitDeclSubExpr(DS);
 
   CFGBlock *B = nullptr;
 
   // Build an individual DeclStmt for each decl.
   for (DeclStmt::reverse_decl_iterator I = DS->decl_rbegin(),
                                        E = DS->decl_rend();
        I != E; ++I) {
-    // Get the alignment of the new DeclStmt, padding out to >=8 bytes.
-    unsigned A = alignof(DeclStmt) < 8 ? 8 : alignof(DeclStmt);
-
-    // Allocate the DeclStmt using the BumpPtrAllocator.  It will get
-    // automatically freed with the CFG.
     DeclGroupRef DG(*I);
     Decl *D = *I;
-    void *Mem = cfg->getAllocator().Allocate(sizeof(DeclStmt), A);
+    void *Mem = allocateMemForStmt<DeclStmt>();
     DeclStmt *DSNew = new (Mem) DeclStmt(DG, D->getLocation(), GetEndLoc(D));
     cfg->addSyntheticDeclStmt(DSNew, DS);
 
     // Append the fake DeclStmt to block.
     B = VisitDeclSubExpr(DSNew);
   }
 
   return B;
for (LambdaExpr::capture_init_iterator it = E->capture_init_begin(),
     if (Expr *Init = *it) {
       CFGBlock *Tmp = Visit(Init);
       if (Tmp)
         LastBlock = Tmp;
     }
   }
   return LastBlock;
 }
-  
+GCCAsmStmt *
+CFGBuilder::getOrCreateGCCAsmStmtWithoutGnuExtensions(GCCAsmStmt *S) {
+  // GCC asm syntax allows using no-op-like casts as outputs. While GCC treats
+  // them as lvalues, clang builds an LValueToRValue cast.
+  // We are going to re-create the GCCAsmStmt if this happens.
+  if (std::find_if(S->begin_outputs(), S->end_outputs(), [this](const Expr *E) {
+        return E != E->IgnoreParenNoopCasts(*Context);
+      }) == S->end_outputs())
+    return S;
+
+  SmallVector<IdentifierInfo *, 4> Names;
+  for (unsigned I = 0, E = S->getNumOutputs(); I != E; I++)
+    Names.push_back(S->getOutputIdentifier(I));
+
+  for (unsigned I = 0, E = S->getNumInputs(); I != E; I++)
+    Names.push_back(S->getInputIdentifier(I));
+
+  SmallVector<StringLiteral *, 4> Clobbers;
+  for (unsigned I = 0, E = S->getNumClobbers(); I != E; I++)
+    Clobbers.push_back(S->getClobberStringLiteral(I));
+
+  SmallVector<StringLiteral *, 4> Constraints;
+  for (unsigned I = 0, E = S->getNumOutputs(); I != E; I++)
+    Constraints.push_back(S->getOutputConstraintLiteral(I));
+
+  for (unsigned I = 0, E = S->getNumInputs(); I != E; I++)
+    Constraints.push_back(S->getInputConstraintLiteral(I));
+
+  SmallVector<Expr *, 4> Exprs(S->getNumOutputs() + S->getNumInputs());
+  std::transform(
+      S->begin_outputs(), S->end_outputs(), Exprs.begin(),
+      [this](Expr *E) -> Expr * { return E->IgnoreParenNoopCasts(*Context); });
+
+  std::copy(S->begin_inputs(), S->end_inputs(),
+            Exprs.begin() + S->getNumOutputs());
+
+  void *Mem = allocateMemForStmt<GCCAsmStmt>();
+  GCCAsmStmt *CleanGCCAsmStmt = new (Mem) GCCAsmStmt(
+      *Context, S->getAsmLoc(), S->isSimple(), S->isVolatile(),
+      S->getNumOutputs(), S->getNumInputs(), Names.data(), Constraints.data(),
+      Exprs.data(), S->getAsmString(), S->getNumClobbers(), Clobbers.data(),
+      S->getRParenLoc());
+  cfg->addSyntheticStmt(CleanGCCAsmStmt, S);
+  return CleanGCCAsmStmt;
+}
+
+CFGBlock *CFGBuilder::VisitGCCAsmStmt(GCCAsmStmt *GCCAsmS, AddStmtChoice asc) {
+  GCCAsmS = getOrCreateGCCAsmStmtWithoutGnuExtensions(GCCAsmS);
+  return VisitStmt(GCCAsmS, asc);
+}
+
 CFGBlock *CFGBuilder::VisitGotoStmt(GotoStmt *G) {
   // Goto is a control-flow statement.  Thus we stop processing the current
   // block and create a new one.
 
   Block = createBlock(false);
   Block->setTerminator(G);
 
   // If we already know the mapping to the label block add the successor now.

lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
   // We have processed both the inputs and the outputs.  All of the outputs
   // should evaluate to Locs.  Nuke all of their values.
 
   // FIXME: Some day in the future it would be nice to allow a "plug-in"
   // which interprets the inline asm and stores proper results in the
   // outputs.
 
   ProgramStateRef state = Pred->getState();
+  const auto *LCtx = Pred->getLocationContext();
 
   for (const Expr *O : A->outputs()) {
-    SVal X = state->getSVal(O, Pred->getLocationContext());
+    SVal X = state->getSVal(O, LCtx);
     assert(!X.getAs<NonLoc>());  // Should be an Lval, or unknown, undef.
 
     if (Optional<Loc> LV = X.getAs<Loc>())
-      state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
+      state = state->bindLoc(*LV, UnknownVal(), LCtx);
   }
 
   Bldr.generateNode(A, Pred, state);
 }
 
 void ExprEngine::VisitMSAsmStmt(const MSAsmStmt *A, ExplodedNode *Pred,
                                 ExplodedNodeSet &Dst) {
   StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
   Bldr.generateNode(A, Pred, Pred->getState());

test/Analysis/asm.cpp

+// 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)global));  // don't crash on rvalue output operand
+  clang_analyzer_eval(global == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(ref == 1);    // expected-warning{{UNKNOWN}}
+}