[Assembler] Improve error when unable to evaluate expression.

[sanwou01]

Summary

Add an SMLoc to MCExpr. Most code does not generate or consume the
SMLoc (yet). Use CHECK-DAG where necessary.

[sanwou01]

FYI, I do not have commit access.

[rengolin]

The change looks good, but it's a bit central to the MC layer, so I'll let Jim approve or add more people if necessary.

test/MC/AArch64/error-location-post-layout.s

4

Why CHECK-DAG? Can they really come in different order?

[grosbach]

This LGTM in general. I agree w/ Renato that CHECK-DAG is a bit unexpected. Can you elaborate on why that's better than plain CHECK?

I've long wanted better plumbing of location info through the asm stuff. This is a nice step in that direction. Thank you for working on it! Do you intend to continue on with threading this through the various places that create MCExprs in the AsmParser?

[sanwou01]

Removed uses of CHECK-DAG where they were unnecessary. Still required for the last two tests in ARM/error-location-post-layout.s because these /do/ come out in the "wrong" order.

@grosbach, I will be working on improving diagnostics throughout the assembler. So yes, I will be threading this through to the AsmParser whenever I come across an MCExpr with a default SMLoc. Seems reasonable to do this incrementally.

[grosbach]

Great! Incremental is definitely the way to go for this sort of thing. Thanks again.

include/llvm/MC/MCExpr.h

 //===- MCExpr.h - Assembly Level Expressions --------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCEXPR_H
 #define LLVM_MC_MCEXPR_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/SMLoc.h"
 
 namespace llvm {
 class MCAsmInfo;
 class MCAsmLayout;
 class MCAssembler;
 class MCContext;
 class MCFixup;
 class MCFragment;
enum ExprKind {
     Constant,  ///< Constant expressions.
     SymbolRef, ///< References to labels and assigned expressions.
     Unary,     ///< Unary expressions.
     Target     ///< Target specific expression.
   };
 
 private:
   ExprKind Kind;
+  SMLoc Loc;
 
   MCExpr(const MCExpr&) = delete;
   void operator=(const MCExpr&) = delete;
 
   bool evaluateAsAbsolute(int64_t &Res, const MCAssembler *Asm,
                           const MCAsmLayout *Layout,
                           const SectionAddrMap *Addrs) const;
 
   bool evaluateAsAbsolute(int64_t &Res, const MCAssembler *Asm,
                           const MCAsmLayout *Layout,
                           const SectionAddrMap *Addrs, bool InSet) const;
 
 protected:
-  explicit MCExpr(ExprKind Kind) : Kind(Kind) {}
+  explicit MCExpr(ExprKind Kind, SMLoc Loc) : Kind(Kind), Loc(Loc) {}
 
   bool evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
                                  const MCAsmLayout *Layout,
                                  const MCFixup *Fixup,
                                  const SectionAddrMap *Addrs, bool InSet) const;
 
 public:
   /// \name Accessors
   /// @{
 
   ExprKind getKind() const { return Kind; }
+  SMLoc getLoc() const { return Loc; }
 
   /// @}
   /// \name Utility Methods
   /// @{
 
   void print(raw_ostream &OS, const MCAsmInfo *MAI,
              bool InParens = false) const;
   void dump() const;
inline raw_ostream &operator<<(raw_ostream &OS, const MCExpr &E) {
   return OS;
 }
 
 //// \brief  Represent a constant integer expression.
 class MCConstantExpr : public MCExpr {
   int64_t Value;
 
   explicit MCConstantExpr(int64_t Value)
-      : MCExpr(MCExpr::Constant), Value(Value) {}
+      : MCExpr(MCExpr::Constant, SMLoc()), Value(Value) {}
 
 public:
   /// \name Construction
   /// @{
 
   static const MCConstantExpr *create(int64_t Value, MCContext &Ctx);
 
   /// @}
private:
 
   // FIXME: Remove this bit.
   const unsigned HasSubsectionsViaSymbols : 1;
 
   /// The symbol being referenced.
   const MCSymbol *Symbol;
 
   explicit MCSymbolRefExpr(const MCSymbol *Symbol, VariantKind Kind,
-                           const MCAsmInfo *MAI);
+                           const MCAsmInfo *MAI, SMLoc Loc = SMLoc());
 
 public:
   /// \name Construction
   /// @{
 
   static const MCSymbolRefExpr *create(const MCSymbol *Symbol, MCContext &Ctx) {
     return MCSymbolRefExpr::create(Symbol, VK_None, Ctx);
   }
 
   static const MCSymbolRefExpr *create(const MCSymbol *Symbol, VariantKind Kind,
-                                       MCContext &Ctx);
+                                       MCContext &Ctx, SMLoc Loc = SMLoc());
   static const MCSymbolRefExpr *create(StringRef Name, VariantKind Kind,
                                        MCContext &Ctx);
 
   /// @}
   /// \name Accessors
   /// @{
 
   const MCSymbol &getSymbol() const { return *Symbol; }
enum Opcode {
     Plus   ///< Unary plus.
   };
 
 private:
   Opcode Op;
   const MCExpr *Expr;
 
   MCUnaryExpr(Opcode Op, const MCExpr *Expr)
-      : MCExpr(MCExpr::Unary), Op(Op), Expr(Expr) {}
+      : MCExpr(MCExpr::Unary, SMLoc()), Op(Op), Expr(Expr) {}
 
 public:
   /// \name Construction
   /// @{
 
   static const MCUnaryExpr *create(Opcode Op, const MCExpr *Expr,
                                    MCContext &Ctx);
   static const MCUnaryExpr *createLNot(const MCExpr *Expr, MCContext &Ctx) {
enum Opcode {
     Sub,  ///< Subtraction.
     Xor   ///< Bitwise exclusive or.
   };
 
 private:
   Opcode Op;
   const MCExpr *LHS, *RHS;
 
-  MCBinaryExpr(Opcode Op, const MCExpr *LHS, const MCExpr *RHS)
-      : MCExpr(MCExpr::Binary), Op(Op), LHS(LHS), RHS(RHS) {}
+  MCBinaryExpr(Opcode Op, const MCExpr *LHS, const MCExpr *RHS,
+               SMLoc Loc = SMLoc())
+      : MCExpr(MCExpr::Binary, Loc), Op(Op), LHS(LHS), RHS(RHS) {}
 
 public:
   /// \name Construction
   /// @{
 
   static const MCBinaryExpr *create(Opcode Op, const MCExpr *LHS,
-                                    const MCExpr *RHS, MCContext &Ctx);
+                                    const MCExpr *RHS, MCContext &Ctx,
+                                    SMLoc Loc = SMLoc());
   static const MCBinaryExpr *createAdd(const MCExpr *LHS, const MCExpr *RHS,
                                        MCContext &Ctx) {
     return create(Add, LHS, RHS, Ctx);
   }
   static const MCBinaryExpr *createAnd(const MCExpr *LHS, const MCExpr *RHS,
                                        MCContext &Ctx) {
     return create(And, LHS, RHS, Ctx);
   }

 /// \brief This is an extension point for target-specific MCExpr subclasses to
 /// implement.
 ///
 /// NOTE: All subclasses are required to have trivial destructors because
 /// MCExprs are bump pointer allocated and not destructed.
 class MCTargetExpr : public MCExpr {
   virtual void anchor();
 protected:
-  MCTargetExpr() : MCExpr(Target) {}
+  MCTargetExpr() : MCExpr(Target, SMLoc()) {}
   virtual ~MCTargetExpr() {}
 public:
   virtual void printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const = 0;
   virtual bool evaluateAsRelocatableImpl(MCValue &Res,
                                          const MCAsmLayout *Layout,
                                          const MCFixup *Fixup) const = 0;
   virtual void visitUsedExpr(MCStreamer& Streamer) const = 0;
   virtual MCFragment *findAssociatedFragment() const = 0;

lib/MC/MCExpr.cpp

 LLVM_DUMP_METHOD void MCExpr::dump() const {
   dbgs() << *this;
   dbgs() << '\n';
 }
 
 /* *** */
 
 const MCBinaryExpr *MCBinaryExpr::create(Opcode Opc, const MCExpr *LHS,
-                                         const MCExpr *RHS, MCContext &Ctx) {
-  return new (Ctx) MCBinaryExpr(Opc, LHS, RHS);
+                                         const MCExpr *RHS, MCContext &Ctx,
+                                         SMLoc Loc) {
+  return new (Ctx) MCBinaryExpr(Opc, LHS, RHS, Loc);
 }
 
 const MCUnaryExpr *MCUnaryExpr::create(Opcode Opc, const MCExpr *Expr,
                                        MCContext &Ctx) {
   return new (Ctx) MCUnaryExpr(Opc, Expr);
 }
 
 const MCConstantExpr *MCConstantExpr::create(int64_t Value, MCContext &Ctx) {
   return new (Ctx) MCConstantExpr(Value);
 }
 
 /* *** */
 
 MCSymbolRefExpr::MCSymbolRefExpr(const MCSymbol *Symbol, VariantKind Kind,
-                                 const MCAsmInfo *MAI)
-    : MCExpr(MCExpr::SymbolRef), Kind(Kind),
+                                 const MCAsmInfo *MAI, SMLoc Loc)
+    : MCExpr(MCExpr::SymbolRef, Loc), Kind(Kind),
       UseParensForSymbolVariant(MAI->useParensForSymbolVariant()),
       HasSubsectionsViaSymbols(MAI->hasSubsectionsViaSymbols()),
       Symbol(Symbol) {
   assert(Symbol);
 }
 
 const MCSymbolRefExpr *MCSymbolRefExpr::create(const MCSymbol *Sym,
                                                VariantKind Kind,
-                                               MCContext &Ctx) {
-  return new (Ctx) MCSymbolRefExpr(Sym, Kind, Ctx.getAsmInfo());
+                                               MCContext &Ctx, SMLoc Loc) {
+  return new (Ctx) MCSymbolRefExpr(Sym, Kind, Ctx.getAsmInfo(), Loc);
 }
 
 const MCSymbolRefExpr *MCSymbolRefExpr::create(StringRef Name, VariantKind Kind,
                                                MCContext &Ctx) {
   return create(Ctx.getOrCreateSymbol(Name), Kind, Ctx);
 }
 
 StringRef MCSymbolRefExpr::getVariantKindName(VariantKind Kind) {

lib/MC/MCFragment.cpp

 const MCSymbol *MCAsmLayout::getBaseSymbol(const MCSymbol &Symbol) const {
   if (!Symbol.isVariable())
     return &Symbol;
 
   const MCExpr *Expr = Symbol.getVariableValue();
   MCValue Value;
   if (!Expr->evaluateAsValue(Value, *this)) {
     Assembler.getContext().reportError(
-        SMLoc(), "expression could not be evaluated");
+        Expr->getLoc(), "expression could not be evaluated");
     return nullptr;
   }
 
   const MCSymbolRefExpr *RefB = Value.getSymB();
   if (RefB) {
     Assembler.getContext().reportError(
-        SMLoc(), Twine("symbol '") + RefB->getSymbol().getName() +
+        Expr->getLoc(), Twine("symbol '") + RefB->getSymbol().getName() +
                      "' could not be evaluated in a subtraction expression");
     return nullptr;
   }
 
   const MCSymbolRefExpr *A = Value.getSymA();
   if (!A)
     return nullptr;
 
   const MCSymbol &ASym = A->getSymbol();
   const MCAssembler &Asm = getAssembler();
   if (ASym.isCommon()) {
-    // FIXME: we should probably add a SMLoc to MCExpr.
-    Asm.getContext().reportError(SMLoc(),
+    Asm.getContext().reportError(Expr->getLoc(),
                                  "Common symbol '" + ASym.getName() +
                                      "' cannot be used in assignment expr");
     return nullptr;
   }
 
   return &ASym;
 }
 

lib/MC/MCParser/AsmParser.cpp

if (Sym->isVariable() &&
       if (Variant)
         return Error(EndLoc, "unexpected modifier on variable reference");
 
       Res = Sym->getVariableValue(/*SetUsed*/ false);
       return false;
     }
 
     // Otherwise create a symbol ref.
-    Res = MCSymbolRefExpr::create(Sym, Variant, getContext());
+    Res = MCSymbolRefExpr::create(Sym, Variant, getContext(), FirstTokenLoc);
     return false;
   }
   case AsmToken::BigNum:
     return TokError("literal value out of range for directive");
   case AsmToken::Integer: {
     SMLoc Loc = getTok().getLoc();
     int64_t IntVal = getTok().getIntVal();
     Res = MCConstantExpr::create(IntVal, getContext());
unsigned AsmParser::getBinOpPrecedence(AsmToken::TokenKind K,
   return IsDarwin ? getDarwinBinOpPrecedence(K, Kind, ShouldUseLogicalShr)
                   : getGNUBinOpPrecedence(K, Kind, ShouldUseLogicalShr);
 }
 
 /// \brief Parse all binary operators with precedence >= 'Precedence'.
 /// Res contains the LHS of the expression on input.
 bool AsmParser::parseBinOpRHS(unsigned Precedence, const MCExpr *&Res,
                               SMLoc &EndLoc) {
+  SMLoc StartLoc = Lexer.getLoc();
   while (true) {
     MCBinaryExpr::Opcode Kind = MCBinaryExpr::Add;
     unsigned TokPrec = getBinOpPrecedence(Lexer.getKind(), Kind);
 
     // If the next token is lower precedence than we are allowed to eat, return
     // successfully with what we ate already.
     if (TokPrec < Precedence)
       return false;
 
     Lex();
 
     // Eat the next primary expression.
     const MCExpr *RHS;
     if (parsePrimaryExpr(RHS, EndLoc))
       return true;
 
     // If BinOp binds less tightly with RHS than the operator after RHS, let
     // the pending operator take RHS as its LHS.
     MCBinaryExpr::Opcode Dummy;
     unsigned NextTokPrec = getBinOpPrecedence(Lexer.getKind(), Dummy);
     if (TokPrec < NextTokPrec && parseBinOpRHS(TokPrec + 1, RHS, EndLoc))
       return true;
 
     // Merge LHS and RHS according to operator.
-    Res = MCBinaryExpr::create(Kind, Res, RHS, getContext());
+    Res = MCBinaryExpr::create(Kind, Res, RHS, getContext(), StartLoc);
   }
 }
 
 /// ParseStatement:
 ///   ::= EndOfStatement
 ///   ::= Label* Directive ...Operands... EndOfStatement
 ///   ::= Label* Identifier OperandList* EndOfStatement
 bool AsmParser::parseStatement(ParseStatementInfo &Info,

test/MC/AArch64/error-location-post-layout.s

 // RUN: not llvm-mc -triple aarch64--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
 
-// Note: These errors are not always emitted in the order in which the relevant
-// source appears, this file is carefully ordered so that that is the case.
-
-// CHECK: <unknown>:0: error: expression could not be evaluated
   .set v1, -undef
+// CHECK: <unknown>:0: error: expression could not be evaluated
 
   .comm common, 4
-// CHECK: <unknown>:0: error: Common symbol 'common' cannot be used in assignment expr
   .set v3, common
+// CHECK: 7:12: error: Common symbol 'common' cannot be used in assignment expr
 
-// CHECK: <unknown>:0: error: symbol 'undef' could not be evaluated in a subtraction expression
   .set v2, a-undef
+// CHECK: 10:13: error: symbol 'undef' could not be evaluated in a subtraction expression

test/MC/ARM/error-location-post-layout.s

 @ RUN: not llvm-mc -triple armv7a--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
 
-@ Note: These errors are not always emitted in the order in which the relevant
-@ source appears, this file is carefully ordered so that that is the case.
-
-@ CHECK: <unknown>:0: error: expression could not be evaluated
   .set v1, -undef
+@ CHECK: <unknown>:0: error: expression could not be evaluated
 
   .comm common, 4
-@ CHECK: <unknown>:0: error: Common symbol 'common' cannot be used in assignment expr
   .set v3, common
+@ CHECK: 7:12: error: Common symbol 'common' cannot be used in assignment expr
 
-@ CHECK: <unknown>:0: error: symbol 'undef' could not be evaluated in a subtraction expression
   .set v2, a-undef
+@ CHECK-DAG: 10:13: error: symbol 'undef' could not be evaluated in a subtraction expression
+
+  .equ STACK_START, (a + undef)
+@ CHECK-DAG: 13:24: error: expression could not be evaluated
 No newline at end of file