[MC] Support .reloc sym+constant, *, *

[MaskRay]

Summary

For .reloc offset, *, *, currently offset can be a constant or symbol.
This patch makes it support any expression which can be folded to sym+constant.

[MaskRay]

Improve tests

[MaskRay]

Move comment to the base emitRelocDirective.

[stefanp]

Thank you for putting up a patch in response to your comment.
Once this goes in I'll rebase D79625 on top of it and that patch should become a lot smaller.

llvm/include/llvm/MC/MCStreamer.h

1022

I feel like you have changed the meaning of this default behaviour. Was that the intention?
Before we used to return true which indicated that the reloc could not be emitted. Now you return None meaning it succeeded.

llvm/lib/MC/MCObjectStreamer.cpp

688

I assume this is what you meant by simpler code. We already have a function to do this computation. :)

704

One of the issues I noticed with this situation is that the symbol may not be in the same data fragment as the .reloc directive. The code you have assumes that it is in the same DF.

I used the same DF as the symbol:

MCFragment *Fragment = Symbol->getSymbol().getFragment();
if (!Fragment || Fragment->getKind() != MCFragment::FT_Data)
  return false;

[stefanp]

llvm/test/MC/ELF/reloc-directive.s

24

One more comment. Since this is sym+const we should have a test along the lines of:

.globl a
	​a: ret
. reloc a+4, R_X86_64_NONE, foo

[MaskRay]

Improve tests

llvm/include/llvm/MC/MCStreamer.h

1022

This is intentional. parseDirectiveReloc tried doing things duplicated with emitRelocDirective (evaluate absolute). By moving logic to emitRelocDirective, we have to let emitRelocDirective handle diagnostics.

llvm/lib/MC/MCObjectStreamer.cpp

704

GNU as somehow allows the following (it behaves as if .reloc is placed in .data)

.text
.reloc .data, R_X86_64_NONE, 0

.data
.globl 
data:

It needs thoughts how to handle/detect this. This probably can go to a separate change.

[stefanp]

llvm/lib/MC/MCObjectStreamer.cpp

704

I wouldn't know where to start with that .data reloc because I don't think we would be able to get the DF for it. I almost feel like we would have to make changes to ELFObjectWriter::recordRelocation and at least allow MCFixup::create to accept more than uint32_t as the offset. But that is a lot of work. Definitely a different patch.

Either way, I don't need the .data but I will need to cover the situation where the DF is different for the symbol. This is for code that will be coming from me later on. For this patch if you want to leave it as a TODO I'm ok with that. I can try to address it in the rework of D79625.

[MaskRay]

llvm/lib/MC/MCObjectStreamer.cpp

704

I'll leave it as a TODO. This did not work correctly before. The intention of the patch is just to add offset support, not thinking hard about how a foreign MCDataFragment works...

Do you have more review comments:) ?

[MaskRay]

llvm/lib/MC/MCObjectStreamer.cpp

704

Actually I have added a FIXME :)

[stefanp]

Nope, no more comments.
LGTM

[MaskRay]

Merge reloc-directive-err.s into reloc-directive.s with --defsym=ERR=1

llvm/include/llvm/MC/MCObjectStreamer.h

public:
   void emitCVFileChecksumsDirective() override;
   void emitCVFileChecksumOffsetDirective(unsigned FileNo) override;
   void emitDTPRel32Value(const MCExpr *Value) override;
   void emitDTPRel64Value(const MCExpr *Value) override;
   void emitTPRel32Value(const MCExpr *Value) override;
   void emitTPRel64Value(const MCExpr *Value) override;
   void emitGPRel32Value(const MCExpr *Value) override;
   void emitGPRel64Value(const MCExpr *Value) override;
-  bool emitRelocDirective(const MCExpr &Offset, StringRef Name,
-                          const MCExpr *Expr, SMLoc Loc,
-                          const MCSubtargetInfo &STI) override;
+  Optional<std::pair<bool, std::string>>
+  emitRelocDirective(const MCExpr &Offset, StringRef Name, const MCExpr *Expr,
+                     SMLoc Loc, const MCSubtargetInfo &STI) override;
   using MCStreamer::emitFill;
   void emitFill(const MCExpr &NumBytes, uint64_t FillValue,
                 SMLoc Loc = SMLoc()) override;
   void emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
                 SMLoc Loc = SMLoc()) override;
   void emitFileDirective(StringRef Filename) override;
 
   void emitAddrsig() override;

llvm/include/llvm/MC/MCStreamer.h

public:
   /// section, but it may be any section containing code.
   MCSection *getAssociatedPDataSection(const MCSection *TextSec);
 
   /// Get the .xdata section used for the given section.
   MCSection *getAssociatedXDataSection(const MCSection *TextSec);
 
   virtual void emitSyntaxDirective();
 
-  /// Emit a .reloc directive.
-  /// Returns true if the relocation could not be emitted because Name is not
-  /// known.
-  virtual bool emitRelocDirective(const MCExpr &Offset, StringRef Name,
-                                  const MCExpr *Expr, SMLoc Loc,
-                                  const MCSubtargetInfo &STI) {
-    return true;
+  /// Record a relocation described by the .reloc directive. Return None if
+  /// succeeded. Otherwise, return a pair (Name is invalid, error message).
+  virtual Optional<std::pair<bool, std::string>>
+  emitRelocDirective(const MCExpr &Offset, StringRef Name, const MCExpr *Expr,
+                     SMLoc Loc, const MCSubtargetInfo &STI) {
+    return None;
   }
 
   virtual void emitAddrsig() {}
   virtual void emitAddrsigSym(const MCSymbol *Sym) {}
 
   /// Emit the given \p Instruction into the current section.
   virtual void emitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI);
 

llvm/lib/MC/MCAsmStreamer.cpp

void emitCGProfileEntry(const MCSymbolRefExpr *From,
                           const MCSymbolRefExpr *To, uint64_t Count) override;
 
   void emitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI) override;
 
   void emitBundleAlignMode(unsigned AlignPow2) override;
   void emitBundleLock(bool AlignToEnd) override;
   void emitBundleUnlock() override;
 
-  bool emitRelocDirective(const MCExpr &Offset, StringRef Name,
-                          const MCExpr *Expr, SMLoc Loc,
-                          const MCSubtargetInfo &STI) override;
+  Optional<std::pair<bool, std::string>>
+  emitRelocDirective(const MCExpr &Offset, StringRef Name, const MCExpr *Expr,
+                     SMLoc Loc, const MCSubtargetInfo &STI) override;
 
   void emitAddrsig() override;
   void emitAddrsigSym(const MCSymbol *Sym) override;
 
   /// If this file is backed by an assembly streamer, this dumps the specified
   /// string in the output .s file. This capability is indicated by the
   /// hasRawTextSupport() predicate.
   void emitRawTextImpl(StringRef String) override;
void MCAsmStreamer::emitBundleLock(bool AlignToEnd) {
   EmitEOL();
 }
 
 void MCAsmStreamer::emitBundleUnlock() {
   OS << "\t.bundle_unlock";
   EmitEOL();
 }
 
-bool MCAsmStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
-                                       const MCExpr *Expr, SMLoc,
-                                       const MCSubtargetInfo &STI) {
+Optional<std::pair<bool, std::string>>
+MCAsmStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
   OS << "\t.reloc ";
   Offset.print(OS, MAI);
   OS << ", " << Name;
   if (Expr) {
     OS << ", ";
     Expr->print(OS, MAI);
   }
   EmitEOL();
-  return false;
+  return None;
 }
 
 void MCAsmStreamer::emitAddrsig() {
   OS << "\t.addrsig";
   EmitEOL();
 }
 
 void MCAsmStreamer::emitAddrsigSym(const MCSymbol *Sym) {

llvm/lib/MC/MCObjectStreamer.cpp

 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCCodeView.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCSymbol.h"
+#include "llvm/MC/MCValue.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SourceMgr.h"
 using namespace llvm;
 
 MCObjectStreamer::MCObjectStreamer(MCContext &Context,
                                    std::unique_ptr<MCAsmBackend> TAB,
                                    std::unique_ptr<MCObjectWriter> OW,
                                    std::unique_ptr<MCCodeEmitter> Emitter)
void MCObjectStreamer::emitGPRel64Value(const MCExpr *Value) {
   MCDataFragment *DF = getOrCreateDataFragment();
   flushPendingLabels(DF, DF->getContents().size());
 
   DF->getFixups().push_back(
       MCFixup::create(DF->getContents().size(), Value, FK_GPRel_4));
   DF->getContents().resize(DF->getContents().size() + 8, 0);
 }
 
-bool MCObjectStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
-                                          const MCExpr *Expr, SMLoc Loc,
-                                          const MCSubtargetInfo &STI) {
+Optional<std::pair<bool, std::string>>
+MCObjectStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
   Optional<MCFixupKind> MaybeKind = Assembler->getBackend().getFixupKind(Name);
   if (!MaybeKind.hasValue())
-    return true;
+    return std::make_pair(true, std::string("unknown relocation name"));
 
   MCFixupKind Kind = *MaybeKind;
 
   if (Expr == nullptr)
     Expr =
         MCSymbolRefExpr::create(getContext().createTempSymbol(), getContext());
 
   MCDataFragment *DF = getOrCreateDataFragment(&STI);
   flushPendingLabels(DF, DF->getContents().size());
 
-  int64_t OffsetValue;
-  if (Offset.evaluateAsAbsolute(OffsetValue)) {
-    if (OffsetValue < 0)
-      llvm_unreachable(".reloc offset is negative");
-    DF->getFixups().push_back(MCFixup::create(OffsetValue, Expr, Kind, Loc));
-    return false;
+  MCValue OffsetVal;
+  if (!Offset.evaluateAsRelocatable(OffsetVal, nullptr, nullptr))
+    return std::make_pair(false,
+                          std::string(".reloc offset is not relocatable"));
+  if (OffsetVal.isAbsolute()) {
+    if (OffsetVal.getConstant() < 0)
+      return std::make_pair(false, std::string(".reloc offset is negative"));
+    DF->getFixups().push_back(
+        MCFixup::create(OffsetVal.getConstant(), Expr, Kind, Loc));
+    return None;
   }
+  if (OffsetVal.getSymB())
+    return std::make_pair(false,
+                          std::string(".reloc offset is not representable"));
 
-  if (Offset.getKind() != llvm::MCExpr::SymbolRef)
-    llvm_unreachable(".reloc offset is not absolute nor a label");
-
-  const MCSymbolRefExpr &SRE = cast<MCSymbolRefExpr>(Offset);
+  const MCSymbolRefExpr &SRE = cast<MCSymbolRefExpr>(*OffsetVal.getSymA());
   if (SRE.getSymbol().isDefined()) {
-    DF->getFixups().push_back(MCFixup::create(SRE.getSymbol().getOffset(),
-                                              Expr, Kind, Loc));
-    return false;
+    // FIXME SRE.getSymbol() may not be relative to DF.
+    DF->getFixups().push_back(
+        MCFixup::create(SRE.getSymbol().getOffset() + OffsetVal.getConstant(),
+    return None;
   }
 
   PendingFixups.emplace_back(&SRE.getSymbol(), DF,
-                                         MCFixup::create(-1, Expr, Kind, Loc));
-  return false;
+  return None;
 }
 
 void MCObjectStreamer::emitFill(const MCExpr &NumBytes, uint64_t FillValue,
                                 SMLoc Loc) {
   MCDataFragment *DF = getOrCreateDataFragment();
   flushPendingLabels(DF, DF->getContents().size());
 
   assert(getCurrentSectionOnly() && "need a section");

llvm/lib/MC/MCParser/AsmParser.cpp

bool AsmParser::parseDirectiveAscii(StringRef IDVal, bool ZeroTerminated) {
   return false;
 }
 
 /// parseDirectiveReloc
 ///  ::= .reloc expression , identifier [ , expression ]
 bool AsmParser::parseDirectiveReloc(SMLoc DirectiveLoc) {
   const MCExpr *Offset;
   const MCExpr *Expr = nullptr;
-  int64_t OffsetValue;
   SMLoc OffsetLoc = Lexer.getTok().getLoc();
 
   if (parseExpression(Offset))
     return true;
-
-  if ((Offset->evaluateAsAbsolute(OffsetValue,
-                                  getStreamer().getAssemblerPtr()) &&
-       check(OffsetValue < 0, OffsetLoc, "expression is negative")) ||
-      (check(Offset->getKind() != llvm::MCExpr::Constant &&
-             Offset->getKind() != llvm::MCExpr::SymbolRef,
-             OffsetLoc, "expected non-negative number or a label")) ||
-      (parseToken(AsmToken::Comma, "expected comma") ||
-       check(getTok().isNot(AsmToken::Identifier), "expected relocation name")))
+  if (parseToken(AsmToken::Comma, "expected comma") ||
+      check(getTok().isNot(AsmToken::Identifier), "expected relocation name"))
     return true;
 
   SMLoc NameLoc = Lexer.getTok().getLoc();
   StringRef Name = Lexer.getTok().getIdentifier();
   Lex();
 
   if (Lexer.is(AsmToken::Comma)) {
     Lex();
     SMLoc ExprLoc = Lexer.getLoc();
     if (parseExpression(Expr))
       return true;
 
     MCValue Value;
     if (!Expr->evaluateAsRelocatable(Value, nullptr, nullptr))
       return Error(ExprLoc, "expression must be relocatable");
   }
 
   if (parseToken(AsmToken::EndOfStatement,
                  "unexpected token in .reloc directive"))
       return true;
 
   const MCTargetAsmParser &MCT = getTargetParser();
   const MCSubtargetInfo &STI = MCT.getSTI();
-  if (getStreamer().emitRelocDirective(*Offset, Name, Expr, DirectiveLoc, STI))
-    return Error(NameLoc, "unknown relocation name");
+  if (Optional<std::pair<bool, std::string>> Err =
+          getStreamer().emitRelocDirective(*Offset, Name, Expr, DirectiveLoc,
+                                           STI))
+    return Error(Err->first ? NameLoc : OffsetLoc, Err->second);
 
   return false;
 }
 
 /// parseDirectiveValue
 ///  ::= (.byte | .short | ... ) [ expression (, expression)* ]
 bool AsmParser::parseDirectiveValue(StringRef IDVal, unsigned Size) {
   auto parseOp = [&]() -> bool {

llvm/test/MC/ELF/reloc-directive-err.s

+# RUN: not llvm-mc -filetype=obj -triple=x86_64 %s -o /dev/null 2>&1 | FileCheck %s
+
+.globl a, b
+a: ret
+b: ret
+x: ret
+y: ret
+
+# CHECK: {{.*}}.s:[[#@LINE+1]]:10: error: expected comma
+.reloc 0 R_X86_64_NONE, a
+
+# CHECK: {{.*}}.s:[[#@LINE+1]]:8: error: .reloc offset is negative
+.reloc -1, R_X86_64_NONE, a
+# CHECK: {{.*}}.s:[[#@LINE+1]]:8: error: .reloc offset is not relocatable
+.reloc 2*., R_X86_64_NONE, a
+# CHECK: {{.*}}.s:[[#@LINE+1]]:8: error: .reloc offset is not relocatable
+.reloc a+a, R_X86_64_NONE, a
+## GNU as accepts a-a but rejects b-a.
+# CHECK: {{.*}}.s:[[#@LINE+1]]:8: error: .reloc offset is not representable
+.reloc a-a, R_X86_64_NONE, a
+## TODO GNU as accepts x-x and y-x.
+# CHECK: {{.*}}.s:[[#@LINE+1]]:8: error: .reloc offset is not representable
+.reloc x-x, R_X86_64_NONE, a
+
+# CHECK: {{.*}}.s:[[#@LINE+1]]:8: error: directional label undefined
+.reloc 1f, R_X86_64_NONE, a
+
+.data
+.globl data
+data:

llvm/test/MC/ELF/reloc-directive.s

+## Target specific relocation support is tested in MC/$target/*reloc-directive*.s
+# RUN: llvm-mc -triple=x86_64 %s | FileCheck %s --check-prefix=ASM
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s
+
+# ASM:      .Ltmp0:
+# ASM-NEXT:  .reloc (.Ltmp0+3)-2, R_X86_64_NONE, foo
+# ASM-NEXT: .Ltmp1:
+# ASM-NEXT:  .reloc .Ltmp1-1, R_X86_64_NONE, foo
+# ASM-NEXT: .Ltmp2:
+# ASM-NEXT:  .reloc 2+.Ltmp2, R_X86_64_NONE, foo
+# ASM-NEXT:  .reloc (1+foo)+3, R_X86_64_NONE, data+1
+
+# CHECK:      0x2 R_X86_64_NONE foo 0x0
+# CHECK-NEXT: 0x0 R_X86_64_NONE foo 0x0
+# CHECK-NEXT: 0x3 R_X86_64_NONE foo 0x0
+# CHECK-NEXT: 0x4 R_X86_64_NONE data 0x1
+
+.text
+.globl foo
+foo:
+  ret
+  .reloc .+3-2, R_X86_64_NONE, foo
+  .reloc .-1, R_X86_64_NONE, foo
+  .reloc 2+., R_X86_64_NONE, foo
+  .reloc 1+foo+3, R_X86_64_NONE, data+1
+
+.data
+.globl data
+data:
+  .long 0

llvm/test/MC/Mips/reloc-directive-bad.s

 # RUN: not llvm-mc -triple mips-unknown-linux < %s -show-encoding \
 # RUN:     -target-abi=o32 2>&1 | FileCheck %s
   .text
 foo:
-  .reloc foo+4, R_MIPS_32, .text    # CHECK: :[[@LINE]]:9: error: expected non-negative number or a label
-  .reloc foo+foo, R_MIPS_32, .text  # CHECK: :[[@LINE]]:9: error: expected non-negative number or a label
   .reloc 0, R_MIPS_32, .text+.text  # CHECK: :[[@LINE]]:23: error: expression must be relocatable
-  .reloc 0 R_MIPS_32, .text         # CHECK: :[[@LINE]]:11: error: expected comma
   .reloc 0, 0, R_MIPS_32, .text     # CHECK: :[[@LINE]]:12: error: expected relocation name
-  .reloc -1, R_MIPS_32, .text       # CHECK: :[[@LINE]]:9: error: expression is negative
-  .reloc 1b, R_MIPS_32, .text       # CHECK: :[[@LINE]]:9: error: directional label undefined
-  .reloc 1f, R_MIPS_32, .text       # CHECK: :[[@LINE]]:9: error: directional label undefined
   nop