[RISCV][MC] Fixed error: could not find corresponding %pcrel_hi

[apazos]

Summary

When parsing instructions, we need to provide the subtarget to detect when the subtarget changes due to options as they are parsed.
This affects the fragment the label will be placed in.

[apazos]

Lewis, is this what you had in mind, I only changed RISC-V parser path.

[lewis-revill]

In D58759#1413207, @apazos wrote:

Lewis, is this what you had in mind, I only changed RISC-V parser path.

Yes, something very similar, thank you. I just added an additional EmitLabel, with a default implementation which just called the original EmitLabel, and override it for MCELFStreamer. If this patch compiles for all targets though I don't see any problem with it.

[apazos]

Hi Lewis, yes it is possible to reduce the code changes. I will push an update.
The concern I have is that for pseudo instructions it was enough to change RISCVAsmParser.cpp to fix the problem. But for the expanded instructions I have to modify AsmParser.cpp.
So maybe instead of adding a new EmitLabel we change the current api.

[apazos]

Reduced code changes, added test case

[apazos]

Updated triple in test case

[efriedma]

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

[lewis-revill]

In D58759#1415688, @efriedma wrote:

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

I guess the difficulty there is that you wouldn't know which fragment the symbol offset (AUIPCSymbol->getOffset()) relates to. So you might end up finding a symbol with a matching offset in the current fragment even though you should be looking in the previous fragment, or vice-versa.

[lewis-revill]

Could you re-title the revision to start with [RISCV] and preferably [MC]? As it is, it may not be seen by all the relevant people.

[apazos]

Hi Eli,

The pair of instructions constraint is that the instruction (or instructions) with the LO12 relocation label points to a valid HI20 relocation which in turn points to the symbol:

At label: R_RISCV_PCREL_HI20 relocation entry ⟶ symbol
R_RISCV_PCREL_LO12_I relocation entry ⟶ label

See https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#relocations

Let us know if you have a suggestion on how to do this matching some other way.

[efriedma]

I guess the difficulty there is that you wouldn't know which fragment the symbol offset (AUIPCSymbol->getOffset()) relates to.

Either the label is in the same fragment as the fixup, so the current code works, or the label points to the end of a fragment and the fixup is at offset zero in the next fragment. Otherwise, they can't match because there's data in between the label and the fixup. (I guess it's a little more complicated if you consider the possibility of zero-size fragments, but that doesn't make it a lot more complicated.)

I'm concerned the approach in current patch can't be generalized to handle all cases, for example:

.option push
.option norelax
nop
2:
.option pop
auipc   a1, %pcrel_hi(another_symbol)
addi    a1, a1, %pcrel_lo(2b)

include/llvm/MC/MCStreamer.h

444

If we're going to do this, we should give this a better name... maybe something like "EmitLabelInNextFragment" or something like that, to indicate that it's actually doing something unusual.

[efriedma]

Posted https://reviews.llvm.org/D58943, which implements my proposal. Hopefully it makes sense.

[apazos]

Hi Eli, this current patch as it is fails for the test case you highlighted. Thanks for clarifying and pushing an alternative solution.

[apazos]

We are going with the alternative in https://reviews.llvm.org/D58943

include/llvm/MC/MCELFStreamer.h

void reset() override {
     MCObjectStreamer::reset();
   }
 
   /// \name MCStreamer Interface
   /// @{
 
   void InitSections(bool NoExecStack) override;
   void ChangeSection(MCSection *Section, const MCExpr *Subsection) override;
-  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                 const MCSubtargetInfo *STI = nullptr) override;
   void EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) override;
   void EmitAssemblerFlag(MCAssemblerFlag Flag) override;
   void EmitThumbFunc(MCSymbol *Func) override;
   void EmitWeakReference(MCSymbol *Alias, const MCSymbol *Symbol) override;
   bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
   void EmitSymbolDesc(MCSymbol *Symbol, unsigned DescValue) override;
   void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                         unsigned ByteAlignment) override;

include/llvm/MC/MCObjectStreamer.h

public:
   /// Create a dummy fragment to assign any pending labels.
   void flushPendingLabels() { flushPendingLabels(nullptr); }
 
   MCAssembler &getAssembler() { return *Assembler; }
   MCAssembler *getAssemblerPtr() override;
   /// \name MCStreamer Interface
   /// @{
 
-  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                 const MCSubtargetInfo *STI = nullptr) override;
   virtual void EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F);
   void EmitAssignment(MCSymbol *Symbol, const MCExpr *Value) override;
   void EmitValueImpl(const MCExpr *Value, unsigned Size,
                      SMLoc Loc = SMLoc()) override;
   void EmitULEB128Value(const MCExpr *Value) override;
   void EmitSLEB128Value(const MCExpr *Value) override;
   void EmitWeakReference(MCSymbol *Alias, const MCSymbol *Symbol) override;
   void ChangeSection(MCSection *Section, const MCExpr *Subsection) override;

include/llvm/MC/MCStreamer.h

public:
   /// This corresponds to an assembler statement such as:
   ///   foo:
   ///
   /// \param Symbol - The symbol to emit. A given symbol should only be
   /// emitted as a label once, and symbols emitted as a label should never be
   /// used in an assignment.
   // FIXME: These emission are non-const because we mutate the symbol to
   // add the section we're emitting it to later.
-  virtual void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc());
+  virtual void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                         const MCSubtargetInfo *STI = nullptr);
 
   virtual void EmitEHSymAttributes(const MCSymbol *Symbol, MCSymbol *EHSymbol);
 
   /// Note in the output the specified \p Flag.
   virtual void EmitAssemblerFlag(MCAssemblerFlag Flag);
 
   /// Emit the given list \p Options of strings as linker
   /// options into the output.
   virtual void EmitLinkerOptions(ArrayRef<std::string> Kind) {}
 
   /// Note in the output the specified region \p Kind.
   virtual void EmitDataRegion(MCDataRegionType Kind) {}

include/llvm/MC/MCWinCOFFStreamer.h

void reset() override {
     CurSymbol = nullptr;
     MCObjectStreamer::reset();
   }
 
   /// \name MCStreamer interface
   /// \{
 
   void InitSections(bool NoExecStack) override;
-  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                 const MCSubtargetInfo *STI = nullptr) override;
   void EmitAssemblerFlag(MCAssemblerFlag Flag) override;
   void EmitThumbFunc(MCSymbol *Func) override;
   bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
   void EmitSymbolDesc(MCSymbol *Symbol, unsigned DescValue) override;
   void BeginCOFFSymbolDef(MCSymbol const *Symbol) override;
   void EmitCOFFSymbolStorageClass(int StorageClass) override;
   void EmitCOFFSymbolType(int Type) override;
   void EndCOFFSymbolDef() override;

lib/MC/MCAsmStreamer.cpp

public:
   /// @{
 
   void ChangeSection(MCSection *Section, const MCExpr *Subsection) override;
 
   void emitELFSymverDirective(StringRef AliasName,
                               const MCSymbol *Aliasee) override;
 
   void EmitLOHDirective(MCLOHType Kind, const MCLOHArgs &Args) override;
-  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                 const MCSubtargetInfo *STI = nullptr) override;
 
   void EmitAssemblerFlag(MCAssemblerFlag Flag) override;
   void EmitLinkerOptions(ArrayRef<std::string> Options) override;
   void EmitDataRegion(MCDataRegionType Kind) override;
   void EmitVersionMin(MCVersionMinType Kind, unsigned Major, unsigned Minor,
                       unsigned Update, VersionTuple SDKVersion) override;
   void EmitBuildVersion(unsigned Platform, unsigned Major, unsigned Minor,
                         unsigned Update, VersionTuple SDKVersion) override;

 void MCAsmStreamer::emitELFSymverDirective(StringRef AliasName,
                                            const MCSymbol *Aliasee) {
   OS << ".symver ";
   Aliasee->print(OS, MAI);
   OS << ", " << AliasName;
   EmitEOL();
 }
 
-void MCAsmStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
+void MCAsmStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc,
+                              const MCSubtargetInfo *STI) {
   MCStreamer::EmitLabel(Symbol, Loc);
 
   Symbol->print(OS, MAI);
   OS << MAI->getLabelSuffix();
 
   EmitEOL();
 }
 

lib/MC/MCELFStreamer.cpp

void MCELFStreamer::InitSections(bool NoExecStack) {
   MCContext &Ctx = getContext();
   SwitchSection(Ctx.getObjectFileInfo()->getTextSection());
   EmitCodeAlignment(4);
 
   if (NoExecStack)
     SwitchSection(Ctx.getAsmInfo()->getNonexecutableStackSection(Ctx));
 }
 
-void MCELFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc) {
+void MCELFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc,
+                              const MCSubtargetInfo *STI) {
   auto *Symbol = cast<MCSymbolELF>(S);
-  MCObjectStreamer::EmitLabel(Symbol, Loc);
+  MCObjectStreamer::EmitLabel(Symbol, Loc, STI);
 
   const MCSectionELF &Section =
       static_cast<const MCSectionELF &>(*getCurrentSectionOnly());
   if (Section.getFlags() & ELF::SHF_TLS)
     Symbol->setType(ELF::STT_TLS);
 }
 
 void MCELFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc, MCFragment *F) {

lib/MC/MCMachOStreamer.cpp

void reset() override {
     HasSectionLabel.clear();
     MCObjectStreamer::reset();
   }
 
   /// @name MCStreamer Interface
   /// @{
 
   void ChangeSection(MCSection *Sect, const MCExpr *Subsect) override;
-  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                 const MCSubtargetInfo *STI = nullptr) override;
   void EmitAssignment(MCSymbol *Symbol, const MCExpr *Value) override;
   void EmitEHSymAttributes(const MCSymbol *Symbol, MCSymbol *EHSymbol) override;
   void EmitAssemblerFlag(MCAssemblerFlag Flag) override;
   void EmitLinkerOptions(ArrayRef<std::string> Options) override;
   void EmitDataRegion(MCDataRegionType Kind) override;
   void EmitVersionMin(MCVersionMinType Kind, unsigned Major, unsigned Minor,
                       unsigned Update, VersionTuple SDKVersion) override;
   void EmitBuildVersion(unsigned Platform, unsigned Major, unsigned Minor,
void MCMachOStreamer::EmitEHSymAttributes(const MCSymbol *Symbol,
   if (Symbol->isExternal())
     EmitSymbolAttribute(EHSymbol, MCSA_Global);
   if (cast<MCSymbolMachO>(Symbol)->isWeakDefinition())
     EmitSymbolAttribute(EHSymbol, MCSA_WeakDefinition);
   if (Symbol->isPrivateExtern())
     EmitSymbolAttribute(EHSymbol, MCSA_PrivateExtern);
 }
 
-void MCMachOStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
+void MCMachOStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc,
+                                const MCSubtargetInfo *STI) {
   // We have to create a new fragment if this is an atom defining symbol,
   // fragments cannot span atoms.
   if (getAssembler().isSymbolLinkerVisible(*Symbol))
     insert(new MCDataFragment());
 
   MCObjectStreamer::EmitLabel(Symbol, Loc);
 
   // This causes the reference type flag to be cleared. Darwin 'as' was "trying"

lib/MC/MCObjectStreamer.cpp

void MCObjectStreamer::EmitCFIStartProcImpl(MCDwarfFrameInfo &Frame) {
   EmitLabel(Frame.Begin);
 }
 
 void MCObjectStreamer::EmitCFIEndProcImpl(MCDwarfFrameInfo &Frame) {
   Frame.End = getContext().createTempSymbol();
   EmitLabel(Frame.End);
 }
 
-void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
+void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc,
+                                 const MCSubtargetInfo *STI) {
   MCStreamer::EmitLabel(Symbol, Loc);
 
   getAssembler().registerSymbol(*Symbol);
 
   // If there is a current fragment, mark the symbol as pointing into it.
   // Otherwise queue the label and set its fragment pointer when we emit the
   // next fragment.
   auto *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
-  if (F && !(getAssembler().isBundlingEnabled() &&
-             getAssembler().getRelaxAll())) {
+  if (F && CanReuseDataFragment(*F, getAssembler(), STI)) {
     Symbol->setFragment(F);
     Symbol->setOffset(F->getContents().size());
   } else {
     PendingLabels.push_back(Symbol);
   }
 }
 
 void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) {

lib/MC/MCParser/AsmParser.cpp

case AsmToken::Colon: {
     if (getTok().is(AsmToken::EndOfStatement)) {
       Lex();
     }
 
     getTargetParser().doBeforeLabelEmit(Sym);
 
     // Emit the label.
     if (!getTargetParser().isParsingInlineAsm())
-      Out.EmitLabel(Sym, IDLoc);
+      Out.EmitLabel(Sym, IDLoc, &getTargetParser().getSTI());
 
     // If we are generating dwarf for assembly source files then gather the
     // info to make a dwarf label entry for this label if needed.
     if (enabledGenDwarfForAssembly())
       MCGenDwarfLabelEntry::Make(Sym, &getStreamer(), getSourceManager(),
                                  IDLoc);
 
     getTargetParser().onLabelParsed(Sym);

lib/MC/MCStreamer.cpp

void MCStreamer::AssignFragment(MCSymbol *Symbol, MCFragment *Fragment) {
   assert(Fragment);
   Symbol->setFragment(Fragment);
 
   // As we emit symbols into a section, track the order so that they can
   // be sorted upon later. Zero is reserved to mean 'unemitted'.
   SymbolOrdering[Symbol] = 1 + SymbolOrdering.size();
 }
 
-void MCStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
+void MCStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc,
+                           const MCSubtargetInfo *STI) {
   Symbol->redefineIfPossible();
 
   if (!Symbol->isUndefined() || Symbol->isVariable())
     return getContext().reportError(Loc, "invalid symbol redefinition");
 
   assert(!Symbol->isVariable() && "Cannot emit a variable symbol!");
   assert(getCurrentSectionOnly() && "Cannot emit before setting section!");
   assert(!Symbol->getFragment() && "Unexpected fragment on symbol data!");

lib/MC/MCWinCOFFStreamer.cpp

void MCWinCOFFStreamer::InitSections(bool NoExecStack) {
   EmitCodeAlignment(4);
 
   SwitchSection(getContext().getObjectFileInfo()->getBSSSection());
   EmitCodeAlignment(4);
 
   SwitchSection(getContext().getObjectFileInfo()->getTextSection());
 }
 
-void MCWinCOFFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc) {
+void MCWinCOFFStreamer::EmitLabel(MCSymbol *S, SMLoc Loc,
+                                  const MCSubtargetInfo * STI) {
   auto *Symbol = cast<MCSymbolCOFF>(S);
   MCObjectStreamer::EmitLabel(Symbol, Loc);
 }
 
 void MCWinCOFFStreamer::EmitAssemblerFlag(MCAssemblerFlag Flag) {
   llvm_unreachable("not implemented");
 }
 

lib/Object/RecordStreamer.h

private:
   void markGlobal(const MCSymbol &Symbol, MCSymbolAttr Attribute);
   void markUsed(const MCSymbol &Symbol);
   void visitUsedSymbol(const MCSymbol &Sym) override;
 
 public:
   RecordStreamer(MCContext &Context, const Module &M);
 
   void EmitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI) override;
-  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc()) override;
+  void EmitLabel(MCSymbol *Symbol, SMLoc Loc = SMLoc(),
+                 const MCSubtargetInfo * STI = nullptr) override;
   void EmitAssignment(MCSymbol *Symbol, const MCExpr *Value) override;
   bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
   void EmitZerofill(MCSection *Section, MCSymbol *Symbol, uint64_t Size,
                     unsigned ByteAlignment, SMLoc Loc = SMLoc()) override;
   void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                         unsigned ByteAlignment) override;
 
   // Ignore COFF-specific directives; we do not need any information from them,

lib/Object/RecordStreamer.cpp

 
 RecordStreamer::const_iterator RecordStreamer::end() { return Symbols.end(); }
 
 void RecordStreamer::EmitInstruction(const MCInst &Inst,
                                      const MCSubtargetInfo &STI) {
   MCStreamer::EmitInstruction(Inst, STI);
 }
 
-void RecordStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
+void RecordStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc,
+                               const MCSubtargetInfo *STI) {
   MCStreamer::EmitLabel(Symbol);
   markDefined(*Symbol);
 }
 
 void RecordStreamer::EmitAssignment(MCSymbol *Symbol, const MCExpr *Value) {
   markDefined(*Symbol);
   MCStreamer::EmitAssignment(Symbol, Value);
 }

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

void RISCVAsmParser::emitAuipcInstPair(MCOperand DestReg, MCOperand TmpReg,
                                        MCStreamer &Out) {
   // A pair of instructions for PC-relative addressing; expands to
   //   TmpLabel: AUIPC TmpReg, VKHi(symbol)
   //             OP DestReg, TmpReg, %pcrel_lo(TmpLabel)
   MCContext &Ctx = getContext();
 
   MCSymbol *TmpLabel = Ctx.createTempSymbol(
       "pcrel_hi", /* AlwaysAddSuffix */ true, /* CanBeUnnamed */ false);
-  Out.EmitLabel(TmpLabel);
+  Out.EmitLabel(TmpLabel, IDLoc, &getSTI());
 
   const RISCVMCExpr *SymbolHi = RISCVMCExpr::create(Symbol, VKHi, Ctx);
   emitToStreamer(
       Out, MCInstBuilder(RISCV::AUIPC).addOperand(TmpReg).addExpr(SymbolHi));
 
   const MCExpr *RefToLinkTmpLabel =
       RISCVMCExpr::create(MCSymbolRefExpr::create(TmpLabel, Ctx),
                           RISCVMCExpr::VK_RISCV_PCREL_LO, Ctx);