[X86][MS-InlineAsm] Extended support for variables / identifiers on memory / immediate expressions

[coby]

Summary

Allow the proper recognition of Enum values and global variables inside ms inline-asm memory / immediate expressions, as they require some additional overhead and treated incorrect if doesn't early recognized.
supersedes D33277, D35775

[RKSimon]

Tests?

lib/Sema/SemaStmtAsm.cpp

71

This all looks like a clang-format NFC change - just commit it?

617

(style) Split these instead of an if-elseif chain

[coby]

addressed Simon's comments

[rnk]

lib/Sema/SemaStmtAsm.cpp

617

LLVM suggests early returns: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

I don't particularly care here, up to you. I personally like to abuse the ability of C++ to return void to write things like this and skip braces, but maybe *I'm* too clever for my own good:

if (T->isFunctionType() || T->isDependentType())
  return Info.setLabel(Res);
if (Res->isRValue()) {
  if (isa<clang::EnumType>(T) && Res->EvaluateAsRValue(Eval, Context))
    return Info.setEnum(Eval.Val.getInt().getSExtValue());
  return Info.setLabel(Res);
}

620

This conditional expression is a bit too cute, I'd rather use a regular if.

[coby]

lib/Sema/SemaStmtAsm.cpp

617

brilliant. was holding myself from emitting such returns, as i had the impression such kind of things are considered taboo. hate redundant bracs. will happily address that.
Anything else?

620

ah.. can't blame me for trying :)

[coby]

addressed @rnk 's suggestions:
cuteness out
c++ mischief in

[rnk]

lgtm, thanks!

include/clang/Parse/Parser.h

ExprResult ParseConstantExpressionInExprEvalContext(
       TypeCastState isTypeCast = NotTypeCast);
   ExprResult ParseConstantExpression(TypeCastState isTypeCast = NotTypeCast);
   ExprResult ParseConstraintExpression();
   // Expr that doesn't include commas.
   ExprResult ParseAssignmentExpression(TypeCastState isTypeCast = NotTypeCast);
 
   ExprResult ParseMSAsmIdentifier(llvm::SmallVectorImpl<Token> &LineToks,
                                   unsigned &NumLineToksConsumed,
-                                  void *Info,
                                   bool IsUnevaluated);
 
 private:
   ExprResult ParseExpressionWithLeadingAt(SourceLocation AtLoc);
 
   ExprResult ParseExpressionWithLeadingExtension(SourceLocation ExtLoc);
 
   ExprResult ParseRHSOfBinaryExpression(ExprResult LHS,

include/clang/Sema/Sema.h

public:
 
   StmtResult ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                              bool IsVolatile, unsigned NumOutputs,
                              unsigned NumInputs, IdentifierInfo **Names,
                              MultiExprArg Constraints, MultiExprArg Exprs,
                              Expr *AsmString, MultiExprArg Clobbers,
                              SourceLocation RParenLoc);
 
+  void FillInlineAsmIdentifierInfo(Expr *Res,
+                                   llvm::InlineAsmIdentifierInfo &Info);
   ExprResult LookupInlineAsmIdentifier(CXXScopeSpec &SS,
                                        SourceLocation TemplateKWLoc,
                                        UnqualifiedId &Id,
-                                       llvm::InlineAsmIdentifierInfo &Info,
                                        bool IsUnevaluatedContext);
   bool LookupInlineAsmField(StringRef Base, StringRef Member,
                             unsigned &Offset, SourceLocation AsmLoc);
   ExprResult LookupInlineAsmVarDeclField(Expr *RefExpr, StringRef Member,
-                                         llvm::InlineAsmIdentifierInfo &Info,
                                          SourceLocation AsmLoc);
   StmtResult ActOnMSAsmStmt(SourceLocation AsmLoc, SourceLocation LBraceLoc,
                             ArrayRef<Token> AsmToks,
                             StringRef AsmString,
                             unsigned NumOutputs, unsigned NumInputs,
                             ArrayRef<StringRef> Constraints,
                             ArrayRef<StringRef> Clobbers,
                             ArrayRef<Expr*> Exprs,

lib/Parse/ParseStmtAsm.cpp

 public:
   ClangAsmParserCallback(Parser &P, SourceLocation Loc, StringRef AsmString,
                          ArrayRef<Token> Toks, ArrayRef<unsigned> Offsets)
       : TheParser(P), AsmLoc(Loc), AsmString(AsmString), AsmToks(Toks),
         AsmTokOffsets(Offsets) {
     assert(AsmToks.size() == AsmTokOffsets.size());
   }
 
-  void *LookupInlineAsmIdentifier(StringRef &LineBuf,
-                                  llvm::InlineAsmIdentifierInfo &Info,
-                                  bool IsUnevaluatedContext) override {
+  void LookupInlineAsmIdentifier(StringRef &LineBuf,
     // Collect the desired tokens.
     SmallVector<Token, 16> LineToks;
     const Token *FirstOrigToken = nullptr;
     findTokensForString(LineBuf, LineToks, FirstOrigToken);
 
     unsigned NumConsumedToks;
     ExprResult Result = TheParser.ParseMSAsmIdentifier(
-        LineToks, NumConsumedToks, &Info, IsUnevaluatedContext);
+        LineToks, NumConsumedToks, IsUnevaluatedContext);
 
     // If we consumed the entire line, tell MC that.
     // Also do this if we consumed nothing as a way of reporting failure.
     if (NumConsumedToks == 0 || NumConsumedToks == LineToks.size()) {
       // By not modifying LineBuf, we're implicitly consuming it all.
 
       // Otherwise, consume up to the original tokens.
     } else {
       assert(FirstOrigToken && "not using original tokens?");
 
       // Since we're using original tokens, apply that offset.
       assert(FirstOrigToken[NumConsumedToks].getLocation() ==
              LineToks[NumConsumedToks].getLocation());
       unsigned FirstIndex = FirstOrigToken - AsmToks.begin();
       unsigned LastIndex = FirstIndex + NumConsumedToks - 1;
 
       // The total length we've consumed is the relative offset
       // of the last token we consumed plus its length.
       unsigned TotalOffset =
           (AsmTokOffsets[LastIndex] + AsmToks[LastIndex].getLength() -
            AsmTokOffsets[FirstIndex]);
       LineBuf = LineBuf.substr(0, TotalOffset);
     }
 
-    // Initialize the "decl" with the lookup result.
-    Info.OpDecl = static_cast<void *>(Result.get());
-    return Info.OpDecl;
+    // Initialize Info with the lookup result.
+    if (!Result.isUsable())
+      return;
+    TheParser.getActions().FillInlineAsmIdentifierInfo(Result.get(), Info);
   }
 
   StringRef LookupInlineAsmLabel(StringRef Identifier, llvm::SourceMgr &LSM,
                                  llvm::SMLoc Location,
                                  bool Create) override {
     SourceLocation Loc = translateLocation(LSM, Location);
     LabelDecl *Label =
       TheParser.getActions().GetOrCreateMSAsmLabel(Identifier, Loc, Create);
void handleDiagnostic(const llvm::SMDiagnostic &D) {
     const llvm::SourceMgr &LSM = *D.getSourceMgr();
     SourceLocation Loc = translateLocation(LSM, D.getLoc());
     TheParser.Diag(Loc, diag::err_inline_ms_asm_parsing) << D.getMessage();
   }
 };
 }
 
 /// Parse an identifier in an MS-style inline assembly block.
-///
-/// \param CastInfo - a void* so that we don't have to teach Parser.h
-///   about the actual type.
 ExprResult Parser::ParseMSAsmIdentifier(llvm::SmallVectorImpl<Token> &LineToks,
                                         unsigned &NumLineToksConsumed,
-                                        void *CastInfo,
                                         bool IsUnevaluatedContext) {
-  llvm::InlineAsmIdentifierInfo &Info =
-      *(llvm::InlineAsmIdentifierInfo *)CastInfo;
-
   // Push a fake token on the end so that we don't overrun the token
   // stream.  We use ';' because it expression-parsing should never
   // overrun it.
   const tok::TokenKind EndOfStream = tok::semi;
   Token EndOfStreamTok;
   EndOfStreamTok.startToken();
   EndOfStreamTok.setKind(EndOfStream);
   LineToks.push_back(EndOfStreamTok);
ExprResult Parser::ParseMSAsmIdentifier(llvm::SmallVectorImpl<Token> &LineToks,
   } else {
     Invalid = ParseUnqualifiedId(SS,
                                  /*EnteringContext=*/false,
                                  /*AllowDestructorName=*/false,
                                  /*AllowConstructorName=*/false,
                                  /*AllowDeductionGuide=*/false,
                                  /*ObjectType=*/nullptr, TemplateKWLoc, Id);
     // Perform the lookup.
-    Result = Actions.LookupInlineAsmIdentifier(SS, TemplateKWLoc, Id, Info,
+    Result = Actions.LookupInlineAsmIdentifier(SS, TemplateKWLoc, Id,
                                                IsUnevaluatedContext);
   }
   // While the next two tokens are 'period' 'identifier', repeatedly parse it as
   // a field access. We have to avoid consuming assembler directives that look
   // like '.' 'else'.
   while (Result.isUsable() && Tok.is(tok::period)) {
     Token IdTok = PP.LookAhead(0);
     if (IdTok.isNot(tok::identifier))
       break;
     ConsumeToken(); // Consume the period.
     IdentifierInfo *Id = Tok.getIdentifierInfo();
     ConsumeToken(); // Consume the identifier.
     Result = Actions.LookupInlineAsmVarDeclField(Result.get(), Id->getName(),
-                                                 Info, Tok.getLocation());
+                                                 Tok.getLocation());
   }
 
   // Figure out how many tokens we are into LineToks.
   unsigned LineIndex = 0;
   if (Tok.is(EndOfStream)) {
     LineIndex = LineToks.size() - 2;
   } else {
     while (LineToks[LineIndex].getLocation() != Tok.getLocation()) {

lib/Sema/SemaStmtAsm.cpp

if (E->isLValue())
     return false;  // Cool, this is an lvalue.
 
   // Okay, this is not an lvalue, but perhaps it is the result of a cast that we
   // are supposed to allow.
   const Expr *E2 = E->IgnoreParenNoopCasts(S.Context);
   if (E != E2 && E2->isLValue()) {
     if (!S.getLangOpts().HeinousExtensions)
       S.Diag(E2->getLocStart(), diag::err_invalid_asm_cast_lvalue)
           << E->getSourceRange();
     else
       S.Diag(E2->getLocStart(), diag::warn_invalid_asm_cast_lvalue)
           << E->getSourceRange();
     // Accept, even if we emitted an error diagnostic.
     return false;
   }
 
   // None of the above, just randomly invalid non-lvalue.
   return true;
 }
 
 /// isOperandMentioned - Return true if the specified operand # is mentioned
 /// anywhere in the decomposed asm string.
-static bool isOperandMentioned(unsigned OpNo,
-                         ArrayRef<GCCAsmStmt::AsmStringPiece> AsmStrPieces) {
+static bool
+isOperandMentioned(unsigned OpNo,
   for (unsigned p = 0, e = AsmStrPieces.size(); p != e; ++p) {
     const GCCAsmStmt::AsmStringPiece &Piece = AsmStrPieces[p];
-    if (!Piece.isOperand()) continue;
+    if (!Piece.isOperand())
+      continue;
 
     // If this is a reference to the input and if the input was the smaller
     // one, then we have to reject this asm.
     if (Piece.getOperandNo() == OpNo)
       return true;
   }
   return false;
 }
SourceLocation ConstraintLoc =
       getClobberConflictLocation(Exprs, Constraints, Clobbers, NumClobbers,
                                  Context.getTargetInfo(), Context);
   if (ConstraintLoc.isValid())
     return Diag(ConstraintLoc, diag::error_inoutput_conflict_with_clobber);
   
   return NS;
 }
 
-static void fillInlineAsmTypeInfo(const ASTContext &Context, QualType T,
+void Sema::FillInlineAsmIdentifierInfo(Expr *Res,
                                        llvm::InlineAsmIdentifierInfo &Info) {
-  // Compute the type size (and array length if applicable?).
-  Info.Type = Info.Size = Context.getTypeSizeInChars(T).getQuantity();
-  if (T->isArrayType()) {
-    const ArrayType *ATy = Context.getAsArrayType(T);
-    Info.Type = Context.getTypeSizeInChars(ATy->getElementType()).getQuantity();
-    Info.Length = Info.Size / Info.Type;
-  }
+  QualType T = Res->getType();
+  Expr::EvalResult Eval;
+  if (T->isFunctionType() || T->isDependentType()) {
+    Info.setLabel(Res);
+    return;
+  } else if (Res->isRValue()) {
+    bool Enum = isa<clang::EnumType>(T) && Res->EvaluateAsRValue(Eval, Context);
+    Enum ? Info.setEnum(Eval.Val.getInt().getSExtValue()) : Info.setLabel(Res);
+    return;
+  }
+  unsigned Size = Context.getTypeSizeInChars(T).getQuantity();
+  unsigned Type = Size;
+  if (const auto *ATy = Context.getAsArrayType(T))
+    Type = Context.getTypeSizeInChars(ATy->getElementType()).getQuantity();
+  bool IsGlobalLV = false;
+  if (Res->EvaluateAsLValue(Eval, Context))
+    IsGlobalLV = Eval.isGlobalLValue();
+  Info.setVar(Res, IsGlobalLV, Size, Type);
 }
 
 ExprResult Sema::LookupInlineAsmIdentifier(CXXScopeSpec &SS,
                                            SourceLocation TemplateKWLoc,
                                            UnqualifiedId &Id,
-                                           llvm::InlineAsmIdentifierInfo &Info,
                                            bool IsUnevaluatedContext) {
-  Info.clear();
 
   if (IsUnevaluatedContext)
     PushExpressionEvaluationContext(
         ExpressionEvaluationContext::UnevaluatedAbstract,
         ReuseLambdaContextDecl);
 
   ExprResult Result = ActOnIdExpression(getCurScope(), SS, TemplateKWLoc, Id,
                                         /*trailing lparen*/ false,
if (T->isFunctionType()) {
     return Result;
   }
 
   // Otherwise, it needs to be a complete type.
   if (RequireCompleteExprType(Result.get(), diag::err_asm_incomplete_type)) {
     return ExprError();
   }
 
-  fillInlineAsmTypeInfo(Context, T, Info);
-
-  // We can work with the expression as long as it's not an r-value.
-  if (!Result.get()->isRValue())
-    Info.IsVarDecl = true;
-
   return Result;
 }
 
 bool Sema::LookupInlineAsmField(StringRef Base, StringRef Member,
                                 unsigned &Offset, SourceLocation AsmLoc) {
   Offset = 0;
   SmallVector<StringRef, 2> Members;
   Member.split(Members, ".");
for (StringRef NextMember : Members) {
     Offset += (unsigned)Result.getQuantity();
   }
 
   return false;
 }
 
 ExprResult
 Sema::LookupInlineAsmVarDeclField(Expr *E, StringRef Member,
-                                  llvm::InlineAsmIdentifierInfo &Info,
                                   SourceLocation AsmLoc) {
-  Info.clear();
 
   QualType T = E->getType();
   if (T->isDependentType()) {
     DeclarationNameInfo NameInfo;
     NameInfo.setLoc(AsmLoc);
     NameInfo.setName(&Context.Idents.get(Member));
     return CXXDependentScopeMemberExpr::Create(
         Context, E, T, /*IsArrow=*/false, AsmLoc, NestedNameSpecifierLoc(),
if (!FD)
     FD = dyn_cast<IndirectFieldDecl>(FieldResult.getFoundDecl());
   if (!FD)
     return ExprResult();
 
   // Make an Expr to thread through OpDecl.
   ExprResult Result = BuildMemberReferenceExpr(
       E, E->getType(), AsmLoc, /*IsArrow=*/false, CXXScopeSpec(),
       SourceLocation(), nullptr, FieldResult, nullptr, nullptr);
-  if (Result.isInvalid())
-    return Result;
-  Info.OpDecl = Result.get();
-
-  fillInlineAsmTypeInfo(Context, Result.get()->getType(), Info);
-
-  // Fields are "variables" as far as inline assembly is concerned.
-  Info.IsVarDecl = true;
 
   return Result;
 }
 
 StmtResult Sema::ActOnMSAsmStmt(SourceLocation AsmLoc, SourceLocation LBraceLoc,
                                 ArrayRef<Token> AsmToks,
                                 StringRef AsmString,
                                 unsigned NumOutputs, unsigned NumInputs,