[MC][X86] Allow assembler variable assignment to register name.

[niravd]

Summary

Allow extended parsing of variable assembler assignment syntax and modify X86 to permit
VAR = register assignment. As we emit these as .set directives when possible, we inline
such expressions in output assembly.

Fixes PR37425.

[rnk]

This breaks the invariant that an MCSymbol is always a relocatable symbol. I wonder if the better way to do this would be to pretend what we actually saw was a macro definition when the RHS of an assignment doesn't parse as an MCExpr, which is basically a language for relocatable expressions.

I'm not familiar enough with gas syntax to know what the equivalent directives would be. I see the #define suggestion in the bug, but is there a gas-only way to express that? We seem to support macros with arguments, but is there a way to express argument-less macros?

[niravd]

In D47545#1116590, @rnk wrote:

This breaks the invariant that an MCSymbol is always a relocatable symbol. I wonder if the better way to do this would be to pretend what we actually saw was a macro definition when the RHS of an assignment doesn't parse as an MCExpr, which is basically a language for relocatable expressions.

That's part of why these symbols are inlined and assignments are elided in the assembly output. I agree it'd better to other variable layers, but neither the C++ processor or assembler macro work. For the former case because of order of evaluation compared to assembler functions; for the latter because assembler macro handling assumes instantiated macros are complete assembly statements.

[nickdesaulniers]

llvm/lib/Target/X86/MCTargetDesc/X86MCExpr.h

15 ↗

(On Diff #149166)

wrong guard?

[niravd]

Address Nick's comments and restrict this to change to V = E statements

[rnk]

llvm/lib/MC/MCParser/AsmParser.cpp

1114–1121 ↗

(On Diff #149196)

Given that we already have logic like this, I'm OK with this. I guess gnu as treats = assignments much more textually than we do.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

Does this generalize to more than just registers? Could it include things like foo = $42, or foo = 0x40(%rcx) as some kind of alias for an accessor? We might want to allow these things so that we don't have to do this fire drill again the next time a Linux developer writes some creative GNU as.

[nickdesaulniers]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

or tries to do this with another ISA, like arm64. I guess I'm curious if this is needs to be implemented in the other parsers as well? We only see the issue on x86 in the kernel currently, but I'd think this kind of functionality would be ISA independent?

[void]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

Something like foo = 0x40(%rcx) should be an illegal expression according to their own documentation....but then again so should using a register as a primary symbol. :-/

[niravd]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

Extending to generic instruction operands seems relatively straightforward, but given that it's illegal and there's really only a few cases we need to support, I'd prefer to keep this as simple as possible and deal with it as it comes along.

Hopefully, once we have clang consistently building the linux kernel with the integrated assembler (which I believe is now just this) additional things we need to workaround in this we should get quick push back and that'll be enough social pressure to prevent new violations needing compiler support vs. a code rewrite.

[niravd]

Since we've only found 2 cases needing this extension and none requiring us to accept a larger set of expressions, I think we should defer any more work until we find such a case in the wild.

Reid, can I get an LGTM?

[rnk]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

I'm still curious to know what gas actually supports in practice before we commit to limiting ourselves to things that look like registers. If gas really treats this as a textual macro, that's probably the way we should go.

They may document that foo = 0x40(%rcx) is illegal, but documentation has been known to be wrong.

[void]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

In an ideal world, we could do that because it would be documented. But as you mentioned their documentation seems faulty. In order to know exactly what gas supports, we would need to unravel its code. That's a fairly complex task, which would take a lot of time and may not be useful.

In this case, it used to be an acceptable practice to allow registers in assembly macros, but it was uncommon. It's possible that its support in gas was unknown to the documentation writers.

IMO, we should go with their documentation and change only when we detect a deviation from it.

[niravd]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

From testing, it's not just textual replacement. It looks like we always interpret assigned variables either as registers or memory references.

foo = <Register> -> valid.

foo = ($<immediate expression>)
foo = $<Immediate expression>
foo = <immediate expression>

Both valid. Always interpreted as memory access i.e. xorl $eax, foo translates to xorl $eax, (<immediate>)

foo = 40(%rcx)
foo = 40 + (%rcx)
foo = 40 + %rcx

All invalid.

That means, the only things we're missing from gas is the parsing of memory references off of immediate (4 + $100), but this isn't parsed currently in straight line code, e.g., "xor %eax, ($40)".

[rnk]

lgtm

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

@void I think my perspective is informed by doing lots of MS compatibility work and triaging user complaints about incompatibility. I lean more towards doing all the experiments we can to try to understand the principle of what gas (or GCC, or MSVC) is really trying to do. It usually saves time in the long run if we can discover the underlying principle and implement it if it is not too far from our existing model. Knowing what's a bridge too far is tough, though.

@niravd Thanks for doing the experiments! The examples convince me that gas users aren't likely to rely on this extension for much more than register names. ($imm) is just not that useful, and is easy to rewrite as an absolute symbol definition. Actually, isn't that just an absolute symbol? Do we not already handle that? Whatever, it's not relevant to this change.

[void]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2197 ↗

(On Diff #149196)

@rnk For the record, I don't disagree with you. And I certainly would like to have full compatibility. One thing we're doing is trying to compile the Linux kernel with the integrated assembler. They have a lot of assembly that does wonky thing. I think it will be a good test for Clang. However, we do need to progress past this bug to find more incompatibilities...It's a bit of a chicken-'n-egg thing, unfortunately. :-(

llvm/trunk/include/llvm/MC/MCExpr.h

protected:
   MCTargetExpr() : MCExpr(Target, SMLoc()) {}
   virtual ~MCTargetExpr() = default;
 
 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;
+  // This should be set when assigned expressions are not valid ".set"
+  // expressions, e.g. registers, and must be inlined.
+  virtual bool inlineAssignedExpr() const { return false; }
   virtual void visitUsedExpr(MCStreamer& Streamer) const = 0;
   virtual MCFragment *findAssociatedFragment() const = 0;
 
   virtual void fixELFSymbolsInTLSFixups(MCAssembler &) const = 0;
 
   static bool classof(const MCExpr *E) {
     return E->getKind() == MCExpr::Target;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCEXPR_H

llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h

bool printPendingErrors() {
     bool rv = !PendingErrors.empty();
     for (auto Err : PendingErrors) {
       printError(Err.Loc, Twine(Err.Msg), Err.Range);
     }
     PendingErrors.clear();
     return rv;
   }
 
+  void clearPendingErrors() { PendingErrors.clear(); }
+
   bool addErrorSuffix(const Twine &Suffix);
 
   /// Get the next AsmToken in the stream, possibly handling file
   /// inclusion first.
   virtual const AsmToken &Lex() = 0;
 
   /// Get the current AsmToken from the stream.
   const AsmToken &getTok() const;

llvm/trunk/include/llvm/MC/MCParser/MCAsmParserUtils.h

 namespace MCParserUtils {
 
 /// Parse a value expression and return whether it can be assigned to a symbol
 /// with the given name.
 ///
 /// On success, returns false and sets the Symbol and Value output parameters.
 bool parseAssignmentExpression(StringRef Name, bool allow_redef,
                                MCAsmParser &Parser, MCSymbol *&Symbol,
-                               const MCExpr *&Value);
+                               const MCExpr *&Value, bool AllowExtendedExpr = false);
 
 } // namespace MCParserUtils
 
 } // namespace llvm
 
 #endif // LLVM_MC_MCPARSER_MCASMPARSERUTILS_H

llvm/trunk/include/llvm/MC/MCParser/MCTargetAsmParser.h

public:
   void setParsingInlineAsm (bool Value) { ParsingInlineAsm = Value; }
 
   MCTargetOptions getTargetOptions() const { return MCOptions; }
 
   void setSemaCallback(MCAsmParserSemaCallback *Callback) {
     SemaCallback = Callback;
   }
 
+  // Target-specific parsing of assembler-level variable assignment.
+  virtual bool parseAssignmentExpression(const MCExpr *&Res, SMLoc &EndLoc) {
+    return getParser().parseExpression(Res, EndLoc);
+  }
+
   virtual bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc,
                              SMLoc &EndLoc) = 0;
 
   /// Sets frame register corresponding to the current MachineFunction.
   virtual void SetFrameRegister(unsigned RegNo) {}
 
   /// ParseInstruction - Parse one assembly instruction.
   ///

llvm/trunk/lib/MC/MCAsmStreamer.cpp

void MCAsmStreamer::EmitThumbFunc(MCSymbol *Func) {
   if (MAI->hasSubsectionsViaSymbols()) {
     OS << '\t';
     Func->print(OS, MAI);
   }
   EmitEOL();
 }
 
 void MCAsmStreamer::EmitAssignment(MCSymbol *Symbol, const MCExpr *Value) {
+  // Do not emit a .set on inlined target assignments.
+  bool EmitSet = true;
+  if (auto *E = dyn_cast<MCTargetExpr>(Value))
+    if (E->inlineAssignedExpr())
+      EmitSet = false;
+  if (EmitSet) {
     OS << ".set ";
     Symbol->print(OS, MAI);
     OS << ", ";
     Value->print(OS, MAI);
 
     EmitEOL();
+  }
 
   MCStreamer::EmitAssignment(Symbol, Value);
 }
 
 void MCAsmStreamer::EmitWeakReference(MCSymbol *Alias, const MCSymbol *Symbol) {
   OS << ".weakref ";
   Alias->print(OS, MAI);
   OS << ", ";

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

private:
   /// will be either the EndOfStatement or EOF.
   StringRef parseStringToEndOfStatement() override;
 
   /// Parse until the end of a statement or a comma is encountered,
   /// return the contents from the current token up to the end or comma.
   StringRef parseStringToComma();
 
   bool parseAssignment(StringRef Name, bool allow_redef,
-                       bool NoDeadStrip = false);
+                       bool NoDeadStrip = false, bool AllowExtendedExpr = false);
 
   unsigned getBinOpPrecedence(AsmToken::TokenKind K,
                               MCBinaryExpr::Opcode &Kind);
 
   bool parseBinOpRHS(unsigned Precedence, const MCExpr *&Res, SMLoc &EndLoc);
   bool parseParenExpr(const MCExpr *&Res, SMLoc &EndLoc);
   bool parseBracketExpr(const MCExpr *&Res, SMLoc &EndLoc);
 
if (!Split.second.empty()) {
                      "invalid variant '" + Split.second + "'");
       }
     }
 
     MCSymbol *Sym = getContext().getOrCreateSymbol(SymbolName);
 
     // If this is an absolute variable reference, substitute it now to preserve
     // semantics in the face of reassignment.
-    if (Sym->isVariable() &&
-        isa<MCConstantExpr>(Sym->getVariableValue(/*SetUsed*/ false))) {
+    if (Sym->isVariable()) {
+      auto V = Sym->getVariableValue(/*SetUsed*/ false);
+      bool DoInline = isa<MCConstantExpr>(V);
+      if (auto TV = dyn_cast<MCTargetExpr>(V))
+        DoInline = TV->inlineAssignedExpr();
+      if (DoInline) {
         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(), FirstTokenLoc);
     return false;
   }
   case AsmToken::BigNum:
     return TokError("literal value out of range for directive");
   case AsmToken::Integer: {
bool AsmParser::parseStatement(ParseStatementInfo &Info,
   }
 
   case AsmToken::Equal:
     if (!getTargetParser().equalIsAsmAssignment())
       break;
     // identifier '=' ... -> assignment statement
     Lex();
 
-    return parseAssignment(IDVal, true);
+    return parseAssignment(IDVal, true, /*NoDeadStrip*/ false, /*AllowExtendedExpr*/true);
 
   default: // Normal instruction or directive.
     break;
   }
 
   // If macros are enabled, check to see if this is a macro instantiation.
   if (areMacrosEnabled())
     if (const MCAsmMacro *M = getContext().lookupMacro(IDVal)) {
void AsmParser::handleMacroExit() {
   Lex();
 
   // Pop the instantiation entry.
   delete ActiveMacros.back();
   ActiveMacros.pop_back();
 }
 
 bool AsmParser::parseAssignment(StringRef Name, bool allow_redef,
-                                bool NoDeadStrip) {
+                                bool NoDeadStrip, bool AllowExtendedExpr) {
   MCSymbol *Sym;
   const MCExpr *Value;
   if (MCParserUtils::parseAssignmentExpression(Name, allow_redef, *this, Sym,
-                                               Value))
+                                               Value, AllowExtendedExpr))
     return true;
 
   if (!Sym) {
     // In the case where we parse an expression starting with a '.', we will
     // not generate an error, nor will we create a symbol.  In this case we
     // should just return out.
     return false;
   }
return isSymbolUsedInExpression(
         Sym, static_cast<const MCUnaryExpr *>(Value)->getSubExpr());
   }
 
   llvm_unreachable("Unknown expr kind!");
 }
 
 bool parseAssignmentExpression(StringRef Name, bool allow_redef,
                                MCAsmParser &Parser, MCSymbol *&Sym,
-                               const MCExpr *&Value) {
+                               const MCExpr *&Value, bool AllowExtendedExpr) {
 
   // FIXME: Use better location, we should use proper tokens.
   SMLoc EqualLoc = Parser.getTok().getLoc();
-
-  if (Parser.parseExpression(Value)) {
+  SMLoc EndLoc;
+  if (AllowExtendedExpr) {
+    if (Parser.getTargetParser().parseAssignmentExpression(Value, EndLoc)) {
       return Parser.TokError("missing expression");
     }
+  } else if (Parser.parseExpression(Value, EndLoc))
+      return Parser.TokError("missing expression");
 
   // Note: we don't count b as used in "a = b". This is to allow
   // a = b
   // b = c
 
   if (Parser.parseToken(AsmToken::EndOfStatement))
     return true;
 

llvm/trunk/lib/MC/MCParser/MCAsmParser.cpp

bool MCAsmParser::check(bool P, SMLoc Loc, const Twine &Msg) {
   return false;
 }
 
 bool MCAsmParser::TokError(const Twine &Msg, SMRange Range) {
   return Error(getLexer().getLoc(), Msg, Range);
 }
 
 bool MCAsmParser::Error(SMLoc L, const Twine &Msg, SMRange Range) {
-  HadError = true;
 
   MCPendingError PErr;
   PErr.Loc = L;
   Msg.toVector(PErr.Msg);
   PErr.Range = Range;
   PendingErrors.push_back(PErr);
 
   // If we threw this parsing error after a lexing error, this should

llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp

 //===-- X86AsmParser.cpp - Parse X86 assembly to MCInst instructions ------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "InstPrinter/X86IntelInstPrinter.h"
 #include "MCTargetDesc/X86BaseInfo.h"
+#include "MCTargetDesc/X86MCExpr.h"
 #include "MCTargetDesc/X86TargetStreamer.h"
 #include "X86AsmInstrumentation.h"
 #include "X86AsmParserCommon.h"
 #include "X86Operand.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
X86AsmParser(const MCSubtargetInfo &sti, MCAsmParser &Parser,
     Instrumentation.reset(
         CreateX86AsmInstrumentation(Options, Parser.getContext(), STI));
   }
 
   bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc, SMLoc &EndLoc) override;
 
   void SetFrameRegister(unsigned RegNo) override;
 
+  bool parseAssignmentExpression(const MCExpr *&Res, SMLoc &EndLoc) override;
+
   bool ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
                         SMLoc NameLoc, OperandVector &Operands) override;
 
   bool ParseDirective(AsmToken DirectiveID) override;
 };
 } // end anonymous namespace
 
 /// @name Auto-generated Match Functions
std::unique_ptr<X86Operand> X86AsmParser::ParseMemOperand(unsigned SegReg,
   // We have to disambiguate a parenthesized expression "(4+5)" from the start
   // of a memory operand with a missing displacement "(%ebx)" or "(,%eax)".  The
   // only way to do this without lookahead is to eat the '(' and see what is
   // after it.
   const MCExpr *Disp = MCConstantExpr::create(0, getParser().getContext());
   if (getLexer().isNot(AsmToken::LParen)) {
     SMLoc ExprEnd;
     if (getParser().parseExpression(Disp, ExprEnd)) return nullptr;
+    // Disp may be a variable, handle register values.
+    if (auto *RE = dyn_cast<X86MCExpr>(Disp))
+      return X86Operand::CreateReg(RE->getRegNo(), MemStart, ExprEnd);
 
     // After parsing the base expression we could either have a parenthesized
     // memory address or not.  If not, return now.  If so, eat the (.
     if (getLexer().isNot(AsmToken::LParen)) {
       // Unless we have a segment register, treat this as an immediate.
       if (SegReg == 0)
         return X86Operand::CreateMem(getPointerWidth(), Disp, MemStart, ExprEnd);
       return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, 0, 0, 1,
std::unique_ptr<X86Operand> X86AsmParser::ParseMemOperand(unsigned SegReg,
   }
 
   if (SegReg || BaseReg || IndexReg)
     return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
                                  IndexReg, Scale, MemStart, MemEnd);
   return X86Operand::CreateMem(getPointerWidth(), Disp, MemStart, MemEnd);
 }
 
+// Parse either a standard expression or a register.
+bool X86AsmParser::parseAssignmentExpression(const MCExpr *&Res,
+                                             SMLoc &EndLoc) {
+  MCAsmParser &Parser = getParser();
+  if (Parser.parseExpression(Res, EndLoc)) {
+    SMLoc StartLoc = Parser.getTok().getLoc();
+    // Normal Expression parse fails, check if it could be a register.
+    unsigned RegNo;
+    if (Parser.getTargetParser().ParseRegister(RegNo, StartLoc, EndLoc))
+      return true;
+    // Clear previous parse error and return correct expression.
+    Parser.clearPendingErrors();
+    Res = X86MCExpr::create(RegNo, Parser.getContext());
+    return false;
+  }
+
+  return false;
+}
+
 bool X86AsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
                                     SMLoc NameLoc, OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   InstInfo = &Info;
   StringRef PatchedName = Name;
 
   if ((Name.equals("jmp") || Name.equals("jc") || Name.equals("jz")) &&
       isParsingIntelSyntax() && isParsingInlineAsm()) {

llvm/trunk/lib/Target/X86/MCTargetDesc/X86MCExpr.h

+//=--- X86MCExpr.h - X86 specific MC expression classes ---*- C++ -*-=//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file describes X86-specific MCExprs, i.e, registers used for
+// extended variable assignments.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_X86_MCTARGETDESC_X86MCEXPR_H
+#define LLVM_LIB_TARGET_X86_MCTARGETDESC_X86MCEXPR_H
+
+#include "InstPrinter/X86ATTInstPrinter.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCExpr.h"
+#include "llvm/Support/ErrorHandling.h"
+
+namespace llvm {
+
+class X86MCExpr : public MCTargetExpr {
+
+private:
+  const int64_t RegNo; // All
+
+  explicit X86MCExpr(int64_t R) : RegNo(R) {}
+
+public:
+  /// @name Construction
+  /// @{
+
+  static const X86MCExpr *create(int64_t RegNo, MCContext &Ctx) {
+    return new (Ctx) X86MCExpr(RegNo);
+  }
+
+  /// @}
+  /// @name Accessors
+  /// @{
+
+  /// getSubExpr - Get the child of this expression.
+  int64_t getRegNo() const { return RegNo; }
+
+  /// @}
+
+  void printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const override {
+    if (MAI->getAssemblerDialect() == 0)
+      OS << '%';
+    OS << X86ATTInstPrinter::getRegisterName(RegNo);
+  }
+
+  bool evaluateAsRelocatableImpl(MCValue &Res, const MCAsmLayout *Layout,
+                                 const MCFixup *Fixup) const override {
+    return false;
+  }
+  // Register values should be inlined as they are not valid .set expressions.
+  bool inlineAssignedExpr() const override { return true; }
+  void visitUsedExpr(MCStreamer &Streamer) const override{};
+  MCFragment *findAssociatedFragment() const override { return nullptr; }
+
+  // There are no TLS X86MCExprs at the moment.
+  void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const override {}
+
+  static bool classof(const MCExpr *E) {
+    return E->getKind() == MCExpr::Target;
+  }
+};
+
+} // end namespace llvm
+
+#endif

llvm/trunk/test/MC/X86/pr37425.s

+// RUN: llvm-mc -triple x86_64-unknown-unknown -defsym=ERR=0 %s -o -      | FileCheck %s
+// RUN: not llvm-mc -triple x86_64-unknown-unknown -defsym=ERR=1 %s -o - 2>&1 | FileCheck --check-prefix=ERR %s
+  
+// CHECK-NOT: .set var_xdata
+var_xdata = %rcx
+
+// CHECK: xorq %rcx, %rcx
+xorq var_xdata, var_xdata
+
+.if (ERR==1)
+// ERR: [[@LINE+2]]:15: error: unknown token in expression in '.set' directive
+// ERR: [[@LINE+1]]:15: error: missing expression in '.set' directive
+.set err_var, %rcx
+.endif  
+
+