[RISCV] Add diff relocation support for RISC-V

[simoncook]

Summary

For RISC-V it is desirable to have relaxation happen in the linker once addresses are known, and as such the size between two instructions/byte sequences in a section could change.

For most assembler expressions, this is fine, as the absolute address results in the expression being converted to a fixup, and finally relocations. However, for expressions such as .quad .L2-.L1, the assembler folds this down to a constant once fragments are laid out, under the assumption that the difference can no longer change, although in the case of linker relaxation the differences can change at link time, so the constant is incorrect. One place where this commonly appears is in debug information, where the size of a function expression is in a form similar to the above.

This patch extends the assembler to allow an AsmBackend to declare that it does not want the assembler to fold down this expression, and instead generate a pair of relocations that allow the linker to carry out the calculation. In this case, the expression is not folded, but when it comes to emitting a fixup, the generic FK_Data_* fixups are converted into a pair, one for the addition half, one for the subtraction, and this is passed to the relocation generating methods as usual. I have named these FK_Data_Add_* and FK_Data_Sub_* to indicate which half these are for.

For RISC-V, which supports this via e.g. the R_RISCV_ADD64, R_RISCV_SUB64 pair of relocations, these are also set to always emit relocations relative to local symbols rather than section offsets. This is to deal with the fact that if relocations were calculated on e.g. .text+8 and .text+4, the result 12 would be stored rather than 4 as both addends are added in the linker.

[apazos]

include/llvm/MC/MCFixup.h

48 ↗

(On Diff #140678)

Comments need to be updated:
FK_Data_Add_8 -> A eight byte..
FK_Data_Sub_1 -> A one-byte..

lib/MC/MCAssembler.cpp

686 ↗

(On Diff #140678)

Comment update:
one for the add, and one for the addition, -> one for the add, and one for the sub,

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

39 ↗

(On Diff #140678)

Should this setting be conditional on the relax flag?
The relax flag is being introduced in https://reviews.llvm.org/D44888

[asb]

Thanks for this Simon.

Perhaps requiresDiffExpressionRelocations or similar might be better than supportsEmittingDiffExpressions? As Ana says, you might want to stick with the current behaviour when -mno-relax is set.

Just to make sure I understand the scope of this patch properly:

• As far as you currently understand, does this cover all cases where constants are emitted where relocations are needed in the presence of linker relaxation. i.e. are there other known cases where you have planned follow-ups?
• Does anyone know of other backends that could be affected by this issue? Linker relaxation seems to used somewhat sparingly as far as I can see, with targets like ARM preferring the less invasive 'range thunks'.

lib/MC/MCAssembler.cpp

684 ↗

(On Diff #140678)

different -> difference

[asb]

From a quick look in binutils, it seems micromips could be tripped up by similar issues.

[mgrang]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

40 ↗

(On Diff #140678)

Remove empty line.

[simoncook]

I've updated the patch to respond to comments, and deal with the couple of test failures I was seeing.

The only one I think may need some thoughts on is the changes I made to hilo-constaddr.s: Using %lo on an expression won't work in this case, gas doesn't accept it as input, and in the relaxation case I don't believe there are RISC-V relocations that could represent such an expression. I've split out the test case into two files, one for the working case, and one for the input I would consider invalid.

Does this seem reasonable, I think we might want to reject this all the time, and not just when linker relaxation is enabled. Otherwise I think it could cause confusion that some expressions are accepted by the assembler only when relaxation is on. (For reference, this is the change in RISCVMCExpr.cpp) Any thoughts on this?

Alex, as far as I'm aware, this covers all cases where constants are emitted from what I've seen so far.

Ana, we can tie this to linker relaxation, looking at D44886 which I think adds the MC part of conditional relaxation, my change in the AsmBackend can be easily swapped from return true to querying the SubTargetInfo for return enableLinkerRelax() when that lands.

[simoncook]

The previous diff had one change missing from my original patch, that is now restored.

[simoncook]

Take part of test I removed as it now produces and error, and turn it into an explicit test for an error message.

[asb]

I think this patch and its tests would be best if based on D44886, so it can be clear which behaviour is conditional on linker relaxation being enabled.

I'd be happy for hilo-constaddr-invalid.s to always be rejected. It never worked in gas, and we can always support it for builds without linker relaxation in the future. Perhaps add a note to the test file to document that we're unconditionally rejecting for now for consistency, but that it would be easy in the future to support when linker relaxation is disabled.

lib/MC/MCAssembler.cpp

685 ↗

(On Diff #141512)

indiciated -> indicated

[simoncook]

I've rebased the change on top of D44886 to indicate what is conditional based on linker relaxation.

I haven't made hilo-constaddr-invalid.s rejected unconditionally. I took a look at how to do this, and it seems it's more difficult to do that (considering fixup types on parent expressions when evaluating subexpressions) than it may be worth it, than to have this accepted based on the linker flag. If the latter is the behavior we want in the end, we already have that and I propose leaving it this way around.

[asb]

test/MC/RISCV/hilo-constaddr.s

3 ↗

(On Diff #142607)

We shouldn't lose the CHECK-REL check from this file, and probably would want to keep the check for the difference of two labels.

[asb]

CC @sdardis - micromips seems to use aggressive linker relaxation in binutils ld meaning this may have some current or future relevance to the Mips LLVM backend.

[simoncook]

I've readded the reloc check line to hilo-constaddr.s and renamed hilo-constaddr-invalid.s to hilo-constaddr-expr.s since it is no longer checking something that is always invalid.

[simoncook]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

42 ↗

(On Diff #143061)

I've been doing some additional thinking on this, and I don't know if having this depend on the relax feature bit might introduce a problem going into the future.

Looking at D45864, the way .option rvc and .option norvc are implemented is they add and remove feature bits. If in the future we support .option norelax through the same approach we can end up with a situation where we end up with correct DWARF depending on which of these two is used last.

If we want to do this unconditionally, then we need to track whether feature relax was ever enabled, possibly via a second feature bit that is set by .option relax but not cleared by .option norelax and have return true if either is set. Does this seem like a good approach?

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

42 ↗

(On Diff #143061)

I could see that argument. I could certainly see a counter-argument that says that if the user uses .option relax and .option norelax in the same section then they're doing something wrong and if things break they get to keep both pieces.

Maybe an alternative is to warn if relax/norelax is switched within the same section?

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

42 ↗

(On Diff #143061)

The array value in the following case will always generate a fixup instead of absolute value after the commit.

commit 4d344626681a31cd8993f97eceb68419bfcc4c54
Author: Nirav Dave <[e-mail]>
AuthorDate: Mon Apr 30 19:22:40 2018 +0000
Commit: Nirav Dave <[e-mail]>
CommitDate: Mon Apr 30 19:22:40 2018 +0000
[MC] Change AsmParser to leverage Assembler during evaluation

int foo (int a)
{
  static const short ar[] = { &&l1 - &&l1, &&l2 - &&l1 };
  void *p = &&l1 + ar[a];
  goto *p;
 l1:
  return 1;
 l2:
  return 2;
}

We probably have two options.

1. requiresDiffExpressionRelocations() always return true to emit ADD16 and SUB16 relocations.
2. Fix up the above commit and then create a new patch to force this case to emit a relocation type when requiresDiffExpressionRelocations return true.

Any thoughts?

[simoncook]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

42 ↗

(On Diff #143061)

I'm not quite sure what you mean. Compiling this test case I see ADD16 and SUB16 relocations in the table when relaxation is enabled, which I would expect. I did notice clang crashes assembling this case with relaxation disabled, I've submitted a fix to this as D46746, was it this crash you were referring to/the fixup being applied there is fine?

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

42 ↗

(On Diff #143061)

Hi Simon. Yes, clang crashes with relaxation disabled in this case. I thought we could let requiresDiffExpressionRelocations always return true to fix this case. But I think https://reviews.llvm.org/D46746 would be a better fixed. Thanks. Another observation is that before the "[MC] Change AsmParser to leverage Assembler" commit,
The case will insert absolute value instead of generating FK_DATA_2 fixup. So the case didn't fail. Anyway, we could apply https://reviews.llvm.org/D46746 on current trunk to pass this case.

[asb]

Thanks Simon, I think this is in pretty good shape. I've added a few comments on code style and expanding the tests. The only part of the patch I'm unsure about is the reference to InSet, so if you can clarify I'd really appreciate it.

include/llvm/MC/MCFixup.h

101 ↗

(On Diff #143061)

autobrief is enabled in the LLVM Doxygen config so \brief isn't needed in any of these newly added functions.

142–165 ↗

(On Diff #143061)

LLVM coding style is not to indent case labels (see getKindForSize).

lib/MC/MCAssembler.cpp

699–701 ↗

(On Diff #143061)

This comment applies to both the if and else branches it should be brought up to just below if (!IsResolved) { and possibly given a minor rephrasing to mention the requiresDiffExpressionRelocation case.

702 ↗

(On Diff #143061)

Given that the if() has a brace, the else should probably have one too. [Note: personal preference - can't see guidance one way or another in the LLVM coding guidelines).

lib/MC/MCExpr.cpp

574 ↗

(On Diff #143061)

Why is InSet needed in this if statement? That's a genuine question rather than a suggestion it's wrong - the meaning of InSet is unclear.

test/MC/RISCV/fixups-expr.s

5 ↗

(On Diff #143061)

Could you please make this test more exhaustive:

1. Add riscv64 check lines
2. Check R_RISCV_{ADD,SUB}{8,16,32} as well

test/MC/RISCV/hilo-constaddr-expr.s

1 ↗

(On Diff #143061)

Could you add a RUN line for when relax is disabled, and adapt the CHECK lines that were previously there when this was in hilo-constaddr.s for this example?

[asb]

test/MC/RISCV/fixups-expr.s

5 ↗

(On Diff #143061)

I committed rL332574 which adds support for the same data directive names used by binutils, so you should be able to use .word etc

[simoncook]

Rebased on top of tree, and applied changes as per Alex's review.

Regarding InSet, I've tried to put a comment that explains its usage, but I'm not sure if it explains why it's there too well (or whether this is really should have a FIXME: The use of InSet = SymbolSize is a hack, similiar to evaluateAsAbsolute (line 461) ).

As far as I can tell, InSet is used as a flag to determine whether to allow expressions to evaluate as an absolute across sections (as this is otherwise an error), and the ELF object emitter re-uses this when emitting the size of a symbol in the symbol table (this seems the most reliable way of identifying an explicit "IsSymbolSize" through the expression evaluator).

The way our linker relaxtion works is that the size in the symbol table is a constant which represents the current symbol size, and the linker adjusts this before emitting its final binary based on how much relaxation occurs. Without this change we would emit a symbol size of zero, and a pair of relocations in the .symtab section, which I don't think the linker understands, or is able to support easily (if at all). Using this approach generates the binary we would expect.

[asb]

Thanks Simon. A few minor nitpicks, but this looks good to me.

You forgot to include full context in the updated patch version.

include/llvm/MC/MCFixup.h

112 ↗

(On Diff #148007)

This \brief still needs removing

lib/MC/MCAssembler.cpp

739–740 ↗

(On Diff #148007)

Should be on a single line } else {

lib/MC/MCExpr.cpp

583 ↗

(On Diff #148007)

reloations -> relocations

Nitpick: end with full stop

[rogfer01]

I don't like resurrecting old reviews. This is just a warning in advance of a future issue we will have with the RISC-V backend when exception handling is enabled (it is not yet upstream).

DwarfDebug is using AsmPrinter::EmitLabelDifferenceAsULEB128 which ends calling MCObjectStreamer::emitAbsoluteSymbolDiffAsULEB128 which calls MCObjectStreamer::absoluteSymbolDiff. That last function checks MCAsmBackend::requiresDiffExpressionRelocations to tell if it can simplify the label difference.

It happens that the function AsmPrinter::EmitLabelDifferenceAsULEB128 is also used by EHStreamer.

When relaxation is enabled, we end with an invalid .eh_frame section. GNU ld rejects those and so do tools like llvm-objdump --dwarf=frames.

After some analysis, I think the reason why the .eh_frame is rejected is because, some of the label differences must be resolved in the final object. At least for the Length field (I'm using this reference: https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-PDA/LSB-PDA/ehframechpt.html) but maybe other values currently emitted as label differences by EHStreamer too (I'm not well versed in this topic).

Now, the RISC-V backend returns true for MCAsmBackend::requiresDiffExpressionRelocations when relaxation is enabled. This means no label difference is folded in .eh_frame. Instead we end with a field Length that is just emitted as zeroes and should be patched up by a relocation (emitted in .rela.eh_frame), but the code that looks at the .eh_frame section does not seem to care with relocations in that field.

@simoncook @asb perhaps you were already aware of this issue?

Thank you!

[jrtc27]

The problem is that Length is the length of the CIE, i.e. the data structure itself; this means it isn't subject to relaxations and so the difference expression can be folded. I don't see much merit in teaching all the tools out there to check relocations for Length when we can instead just emit the "right" thing. I think the fix is to make sure we only return true from requiresDiffExpressionRelocations if a target is in a code section? MCSection::UseCodeAlign will tell you this (or you can use MCSectionELF::getFlags() & ELF::SHF_EXECINSTR to be less general), though perhaps it should be renamed to isCodeSection to match isVirtualSection.

[asb]

Thanks for the report Roger. I agree with @jrtc27's analysis, it makes sense to limit requiresDiffExpressionRelocations to returning true only for code sections.

[rogfer01]

James, Alex: Thanks a lot for the comments. I'll look into what I can do here.

[luismarques]

In D45181#1470356, @rogfer01 wrote:

James, Alex: Thanks a lot for the comments. I'll look into what I can do here.

This issue also affects the debug information in .debug_info. Relocations (R_RISCV_ADD32 and R_RISCV_SUB32) are emitted for DWARF's compilation unit header Length field. While binutils accepts that, LLVM does not (and it's not trivial to fix that because DWARFContext currently assumes that only one relocation would exist per offset, while these label differences require two), which means that tools like llvm-objdump and llvm-dwarfdump don't correctly parse the debug information. Returning false in requiresDiffExpressionRelocations does not suffice to solve that issue because an absolute R_RISCV_32 relocation is still emitted.

@rogfer01 Are you still looking into this? My efforts to fix the problem with the debug information would also touch this. How do you want to proceed?

[rogfer01]

In D45181#1488084, @luismarques wrote:

@rogfer01 Are you still looking into this? My efforts to fix the problem with the debug information would also touch this. How do you want to proceed?

I made some changes in my downstream so things link successfully but I'm still seeing issues with exceptions at runtime. So nothing very useful at the moment for upstream, feel free to make changes here.

[HsiangKai]

In D45181#1489015, @rogfer01 wrote:

In D45181#1488084, @luismarques wrote:

@rogfer01 Are you still looking into this? My efforts to fix the problem with the debug information would also touch this. How do you want to proceed?

I made some changes in my downstream so things link successfully but I'm still seeing issues with exceptions at runtime. So nothing very useful at the moment for upstream, feel free to make changes here.

I added a flag to MCExpr to decide to resolve the symbol difference or not. If the flag is set, the symbol difference will be resolved even when relaxation is enabled. I upstream the idea to D61584. If it is acceptable to add flags to MCExpr, I think it is a solution for the problem. Any suggestions?

By the way, I also found another problem when dealing with exception handling table. The "call site encoding" is uleb128 by default in LLVM. However, LLVM does not support variable length encoding in symbol difference relocations in current implementation. So, when relaxation is enabled, I think it is simpler to use udata4 as "call site encoding".

[HsiangKai]

In D45181#1470189, @jrtc27 wrote:

The problem is that Length is the length of the CIE, i.e. the data structure itself; this means it isn't subject to relaxations and so the difference expression can be folded. I don't see much merit in teaching all the tools out there to check relocations for Length when we can instead just emit the "right" thing. I think the fix is to make sure we only return true from requiresDiffExpressionRelocations if a target is in a code section? MCSection::UseCodeAlign will tell you this (or you can use MCSectionELF::getFlags() & ELF::SHF_EXECINSTR to be less general), though perhaps it should be renamed to isCodeSection to match isVirtualSection.

Use SHF_EXECINSTR flag may be not enough. Several debug information sections also need relocations. For example, although it has no need to generate relocations for Length in FDE, it needs relocations for Address Range in FDE. These two attributes are located in the same section and the section has no SHF_EXECINSTR flag.

[luismarques]

In D45181#1488084, @luismarques wrote:

Use SHF_EXECINSTR flag may be not enough. Several debug information sections also need relocations. For example, although it has no need to generate relocations for Length in FDE, it needs relocations for Address Range in FDE. These two attributes are located in the same section and the section has no SHF_EXECINSTR flag.

I think the point is to check for the SHF_EXECINSTR flag *not* in the section where the value is going to be generated but for the sections of the referenced symbols. If the symbols are in a code section then their addresses can change due to the relaxations. Since relaxations only occur in code sections (right?) then this isn't an issue for symbols in non-code sections, and therefore you can evaluate the fixup and avoid emitting a relocation.

llvm/trunk/include/llvm/MC/MCAsmBackend.h

public:
   /// Apply the \p Value for given \p Fixup into the provided data fragment, at
   /// the offset specified by the fixup and following the fixup kind as
   /// appropriate. Errors (such as an out of range fixup value) should be
   /// reported via \p Ctx.
   virtual void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                           const MCValue &Target, MutableArrayRef<char> Data,
                           uint64_t Value, bool IsResolved) const = 0;
 
+  /// Check whether the given target requires emitting differences of two
+  /// symbols as a set of relocations.
+  virtual bool requiresDiffExpressionRelocations() const { return false; }
+
   /// @}
 
   /// \name Target Relaxation Interfaces
   /// @{
 
   /// Check whether the given instruction may need relaxation.
   ///
   /// \param Inst - The instruction to test.

llvm/trunk/include/llvm/MC/MCFixup.h

enum MCFixupKind {
   FK_DTPRel_4,   ///< A four-byte dtp relative fixup.
   FK_DTPRel_8,   ///< A eight-byte dtp relative fixup.
   FK_TPRel_4,    ///< A four-byte tp relative fixup.
   FK_TPRel_8,    ///< A eight-byte tp relative fixup.
   FK_SecRel_1,   ///< A one-byte section relative fixup.
   FK_SecRel_2,   ///< A two-byte section relative fixup.
   FK_SecRel_4,   ///< A four-byte section relative fixup.
   FK_SecRel_8,   ///< A eight-byte section relative fixup.
+  FK_Data_Add_1, ///< A one-byte add fixup.
+  FK_Data_Add_2, ///< A two-byte add fixup.
+  FK_Data_Add_4, ///< A four-byte add fixup.
+  FK_Data_Add_8, ///< A eight-byte add fixup.
+  FK_Data_Sub_1, ///< A one-byte sub fixup.
+  FK_Data_Sub_2, ///< A two-byte sub fixup.
+  FK_Data_Sub_4, ///< A four-byte sub fixup.
+  FK_Data_Sub_8, ///< A eight-byte sub fixup.
 
   FirstTargetFixupKind = 128,
 
   // Limit range of target fixups, in case we want to pack more efficiently
   // later.
   MaxTargetFixupKind = (1 << 8)
 };
 
static MCFixup create(uint32_t Offset, const MCExpr *Value,
     MCFixup FI;
     FI.Value = Value;
     FI.Offset = Offset;
     FI.Kind = unsigned(Kind);
     FI.Loc = Loc;
     return FI;
   }
 
+  /// Return a fixup corresponding to the add half of a add/sub fixup pair for
+  /// the given Fixup.
+  static MCFixup createAddFor(const MCFixup &Fixup) {
+    MCFixup FI;
+    FI.Value = Fixup.getValue();
+    FI.Offset = Fixup.getOffset();
+    FI.Kind = (unsigned)getAddKindForKind(Fixup.getKind());
+    FI.Loc = Fixup.getLoc();
+    return FI;
+  }
+
+  /// Return a fixup corresponding to the sub half of a add/sub fixup pair for
+  /// the given Fixup.
+  static MCFixup createSubFor(const MCFixup &Fixup) {
+    MCFixup FI;
+    FI.Value = Fixup.getValue();
+    FI.Offset = Fixup.getOffset();
+    FI.Kind = (unsigned)getSubKindForKind(Fixup.getKind());
+    FI.Loc = Fixup.getLoc();
+    return FI;
+  }
+
   MCFixupKind getKind() const { return MCFixupKind(Kind); }
 
   uint32_t getOffset() const { return Offset; }
   void setOffset(uint32_t Value) { Offset = Value; }
 
   const MCExpr *getValue() const { return Value; }
 
   /// Return the generic fixup kind for a value with the given size. It
   /// is an error to pass an unsupported size.
   static MCFixupKind getKindForSize(unsigned Size, bool isPCRel) {
     switch (Size) {
     default: llvm_unreachable("Invalid generic fixup size!");
     case 1: return isPCRel ? FK_PCRel_1 : FK_Data_1;
     case 2: return isPCRel ? FK_PCRel_2 : FK_Data_2;
     case 4: return isPCRel ? FK_PCRel_4 : FK_Data_4;
     case 8: return isPCRel ? FK_PCRel_8 : FK_Data_8;
     }
   }
 
+  /// Return the generic fixup kind for an addition with a given size. It
+  /// is an error to pass an unsupported size.
+  static MCFixupKind getAddKindForKind(unsigned Kind) {
+    switch (Kind) {
+    default: llvm_unreachable("Unknown type to convert!");
+    case FK_Data_1: return FK_Data_Add_1;
+    case FK_Data_2: return FK_Data_Add_2;
+    case FK_Data_4: return FK_Data_Add_4;
+    case FK_Data_8: return FK_Data_Add_8;
+    }
+  }
+
+  /// Return the generic fixup kind for an subtraction with a given size. It
+  /// is an error to pass an unsupported size.
+  static MCFixupKind getSubKindForKind(unsigned Kind) {
+    switch (Kind) {
+    default: llvm_unreachable("Unknown type to convert!");
+    case FK_Data_1: return FK_Data_Sub_1;
+    case FK_Data_2: return FK_Data_Sub_2;
+    case FK_Data_4: return FK_Data_Sub_4;
+    case FK_Data_8: return FK_Data_Sub_8;
+    }
+  }
+
   SMLoc getLoc() const { return Loc; }
 };
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/MC/MCAsmBackend.cpp

static const MCFixupKindInfo Builtins[] = {
       {"FK_GPRel_8", 0, 64, 0},
       {"FK_DTPRel_4", 0, 32, 0},
       {"FK_DTPRel_8", 0, 64, 0},
       {"FK_TPRel_4", 0, 32, 0},
       {"FK_TPRel_8", 0, 64, 0},
       {"FK_SecRel_1", 0, 8, 0},
       {"FK_SecRel_2", 0, 16, 0},
       {"FK_SecRel_4", 0, 32, 0},
-      {"FK_SecRel_8", 0, 64, 0}};
+      {"FK_SecRel_8", 0, 64, 0},
+      {"FK_Data_Add_1", 0, 8, 0},
+      {"FK_Data_Add_2", 0, 16, 0},
+      {"FK_Data_Add_4", 0, 32, 0},
+      {"FK_Data_Add_8", 0, 64, 0},
+      {"FK_Data_Sub_1", 0, 8, 0},
+      {"FK_Data_Sub_2", 0, 16, 0},
+      {"FK_Data_Sub_4", 0, 32, 0},
+      {"FK_Data_Sub_8", 0, 64, 0}};
 
   assert((size_t)Kind <= array_lengthof(Builtins) && "Unknown fixup kind");
   return Builtins[Kind];
 }
 
 bool MCAsmBackend::fixupNeedsRelaxationAdvanced(
     const MCFixup &Fixup, bool Resolved, uint64_t Value,
     const MCRelaxableFragment *DF, const MCAsmLayout &Layout,

llvm/trunk/lib/MC/MCAssembler.cpp

MCAssembler::handleFixup(const MCAsmLayout &Layout, MCFragment &F,
   uint64_t FixedValue;
   bool WasForced;
   bool IsResolved = evaluateFixup(Layout, Fixup, &F, Target, FixedValue,
                                   WasForced);
   if (!IsResolved) {
     // The fixup was unresolved, we need a relocation. Inform the object
     // writer of the relocation, and give it an opportunity to adjust the
     // fixup value if need be.
-    getWriter().recordRelocation(*this, Layout, &F, Fixup, Target, FixedValue);
+    if (Target.getSymA() && Target.getSymB() &&
+        getBackend().requiresDiffExpressionRelocations()) {
+      // The fixup represents the difference between two symbols, which the
+      // backend has indicated must be resolved at link time. Split up the fixup
+      // into two relocations, one for the add, and one for the sub, and emit
+      // both of these. The constant will be associated with the add half of the
+      // expression.
+      MCFixup FixupAdd = MCFixup::createAddFor(Fixup);
+      MCValue TargetAdd =
+          MCValue::get(Target.getSymA(), nullptr, Target.getConstant());
+      getWriter().recordRelocation(*this, Layout, &F, FixupAdd, TargetAdd,
+                                   FixedValue);
+      MCFixup FixupSub = MCFixup::createSubFor(Fixup);
+      MCValue TargetSub = MCValue::get(Target.getSymB());
+      getWriter().recordRelocation(*this, Layout, &F, FixupSub, TargetSub,
+                                   FixedValue);
+    } else {
+      getWriter().recordRelocation(*this, Layout, &F, Fixup, Target,
+                                   FixedValue);
+    }
   }
   return std::make_tuple(Target, FixedValue, IsResolved);
 }
 
 void MCAssembler::layout(MCAsmLayout &Layout) {
   assert(getBackendPtr() && "Expected assembler backend");
   DEBUG_WITH_TYPE("mc-dump", {
       errs() << "assembler backend - pre-layout\n--\n";

llvm/trunk/lib/MC/MCExpr.cpp

 //===- MCExpr.cpp - Assembly Level Expression Implementation --------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/MC/MCExpr.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCAsmLayout.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/Casting.h"
EvaluateSymbolicAdd(const MCAssembler *Asm, const MCAsmLayout *Layout,
   int64_t LHS_Cst = LHS.getConstant();
 
   // Fold the result constant immediately.
   int64_t Result_Cst = LHS_Cst + RHS_Cst;
 
   assert((!Layout || Asm) &&
          "Must have an assembler object if layout is given!");
 
-  // If we have a layout, we can fold resolved differences.
-  if (Asm) {
+  // If we have a layout, we can fold resolved differences. Do not do this if
+  // the backend requires this to be emitted as individual relocations, unless
+  // the InSet flag is set to get the current difference anyway (used for
+  // example to calculate symbol sizes).
+  if (Asm &&
+      (InSet || !Asm->getBackend().requiresDiffExpressionRelocations())) {
     // First, fold out any differences which are fully resolved. By
     // reassociating terms in
     //   Result = (LHS_A - LHS_B + LHS_Cst) + (RHS_A - RHS_B + RHS_Cst).
     // we have the four possible differences:
     //   (LHS_A - LHS_B),
     //   (LHS_A - RHS_B),
     //   (RHS_A - LHS_B),
     //   (RHS_A - RHS_B).

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

class RISCVAsmBackend : public MCAsmBackend {
   bool Is64Bit;
 
 public:
   RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit)
       : MCAsmBackend(support::little), STI(STI), OSABI(OSABI),
         Is64Bit(Is64Bit) {}
   ~RISCVAsmBackend() override {}
 
+  // Generate diff expression relocations if the relax feature is enabled,
+  // otherwise it is safe for the assembler to calculate these internally.
+  bool requiresDiffExpressionRelocations() const override {
+    return STI.getFeatureBits()[RISCV::FeatureRelax];
+  }
   void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved) const override;
 
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override;
 
   // If linker relaxation is enabled, always emit relocations even if the fixup

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp

unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
   // Determine the type of the relocation
   switch ((unsigned)Fixup.getKind()) {
   default:
     llvm_unreachable("invalid fixup kind!");
   case FK_Data_4:
     return ELF::R_RISCV_32;
   case FK_Data_8:
     return ELF::R_RISCV_64;
+  case FK_Data_Add_1:
+    return ELF::R_RISCV_ADD8;
+  case FK_Data_Add_2:
+    return ELF::R_RISCV_ADD16;
+  case FK_Data_Add_4:
+    return ELF::R_RISCV_ADD32;
+  case FK_Data_Add_8:
+    return ELF::R_RISCV_ADD64;
+  case FK_Data_Sub_1:
+    return ELF::R_RISCV_SUB8;
+  case FK_Data_Sub_2:
+    return ELF::R_RISCV_SUB16;
+  case FK_Data_Sub_4:
+    return ELF::R_RISCV_SUB32;
+  case FK_Data_Sub_8:
+    return ELF::R_RISCV_SUB64;
   case RISCV::fixup_riscv_hi20:
     return ELF::R_RISCV_HI20;
   case RISCV::fixup_riscv_lo12_i:
     return ELF::R_RISCV_LO12_I;
   case RISCV::fixup_riscv_lo12_s:
     return ELF::R_RISCV_LO12_S;
   case RISCV::fixup_riscv_pcrel_hi20:
     return ELF::R_RISCV_PCREL_HI20;

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

void RISCVMCExpr::printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const {
   Expr->print(OS, MAI);
   if (HasVariant)
     OS << ')';
 }
 
 bool RISCVMCExpr::evaluateAsRelocatableImpl(MCValue &Res,
                                             const MCAsmLayout *Layout,
                                             const MCFixup *Fixup) const {
-  return getSubExpr()->evaluateAsRelocatable(Res, Layout, Fixup);
+  if (!getSubExpr()->evaluateAsRelocatable(Res, Layout, Fixup))
+    return false;
+
+  // Some custom fixup types are not valid with symbol difference expressions
+  if (Res.getSymA() && Res.getSymB()) {
+    switch (getKind()) {
+    default:
+      return true;
+    case VK_RISCV_LO:
+    case VK_RISCV_HI:
+    case VK_RISCV_PCREL_LO:
+    case VK_RISCV_PCREL_HI:
+      return false;
+    }
+  }
+
+  return true;
 }
 
 void RISCVMCExpr::visitUsedExpr(MCStreamer &Streamer) const {
   Streamer.visitUsedExpr(*getSubExpr());
 }
 
 RISCVMCExpr::VariantKind RISCVMCExpr::getVariantKindForName(StringRef name) {
   return StringSwitch<RISCVMCExpr::VariantKind>(name)

llvm/trunk/test/MC/RISCV/fixups-expr.s

+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+relax %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX %s
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=-relax %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX %s
+
+# Check that subtraction expressions are emitted as two relocations
+# only when relaxation is enabled
+
+.globl G1
+.globl G2
+.L1:
+G1:
+addi a0, a0, 0
+.L2:
+G2:
+
+.data
+.dword .L2-.L1
+.dword G2-G1
+.word .L2-.L1
+.word G2-G1
+.half .L2-.L1
+.half G2-G1
+.byte .L2-.L1
+.byte G2-G1
+# RELAX: 0x0 R_RISCV_ADD64 .L2 0x0
+# RELAX: 0x0 R_RISCV_SUB64 .L1 0x0
+# RELAX: 0x8 R_RISCV_ADD64 G2 0x0
+# RELAX: 0x8 R_RISCV_SUB64 G1 0x0
+# RELAX: 0x10 R_RISCV_ADD32 .L2 0x0
+# RELAX: 0x10 R_RISCV_SUB32 .L1 0x0
+# RELAX: 0x14 R_RISCV_ADD32 G2 0x0
+# RELAX: 0x14 R_RISCV_SUB32 G1 0x0
+# RELAX: 0x18 R_RISCV_ADD16 .L2 0x0
+# RELAX: 0x18 R_RISCV_SUB16 .L1 0x0
+# RELAX: 0x1A R_RISCV_ADD16 G2 0x0
+# RELAX: 0x1A R_RISCV_SUB16 G1 0x0
+# RELAX: 0x1C R_RISCV_ADD8 .L2 0x0
+# RELAX: 0x1C R_RISCV_SUB8 .L1 0x0
+# RELAX: 0x1D R_RISCV_ADD8 G2 0x0
+# RELAX: 0x1D R_RISCV_SUB8 G1 0x0
+# NORELAX-NOT: R_RISCV

llvm/trunk/test/MC/RISCV/hilo-constaddr-expr.s

+# RUN: not llvm-mc -filetype=obj -triple=riscv32 -mattr=+relax %s 2>&1 \
+# RUN:     | FileCheck %s -check-prefix=CHECK-RELAX
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s 2>&1 \
+# RUN:     | llvm-objdump -d - | FileCheck %s -check-prefix=CHECK-INSTR
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s 2>&1 \
+# RUN:     | llvm-objdump -r - | FileCheck %s -check-prefix=CHECK-REL
+
+# Check the assembler rejects hi and lo expressions with constant expressions
+# involving labels when diff expressions are emitted as relocation pairs.
+# Test case derived from test/MC/Mips/hilo-addressing.s
+
+tmp1:
+  # Emit zeros so that difference between tmp1 and tmp3 is 0x30124 bytes.
+  .fill 0x30124-8
+tmp2:
+# CHECK-RELAX: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
+  lui t0, %hi(tmp3-tmp1)
+# CHECK-RELAX: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
+  lw ra, %lo(tmp3-tmp1)(t0)
+# CHECK-INSTR: lui t0, 48
+# CHECK-INSTR: lw ra, 292(t0)
+
+tmp3:
+# CHECK-RELAX: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
+  lui t1, %hi(tmp2-tmp3)
+# CHECK-RELAX: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
+  lw sp, %lo(tmp2-tmp3)(t1)
+# CHECK-INSTR: lui t1, 0
+# CHECK-INSTR: lw sp, -8(t1)
+
+# CHECK-REL-NOT: R_RISCV
 No newline at end of file

llvm/trunk/test/MC/RISCV/hilo-constaddr.s

 # RUN: llvm-mc -filetype=obj -triple=riscv32 %s \
 # RUN:  | llvm-objdump -d - | FileCheck %s -check-prefix=CHECK-INSTR
 
 # RUN: llvm-mc -filetype=obj -triple=riscv32 %s \
 # RUN:  | llvm-readobj -r | FileCheck %s -check-prefix=CHECK-REL
 
-# Check the assembler can handle hi and lo expressions with a constant 
-# address, and constant expressions involving labels. Test case derived from 
-# test/MC/Mips/hilo-addressing.s
+# Check the assembler can handle hi and lo expressions with a constant
+# address. Test case derived from test/MC/Mips/hilo-addressing.s
 
 # Check that 1 is added to the high 20 bits if bit 11 of the low part is 1.
 .equ addr, 0xdeadbeef
   lui t0, %hi(addr)
   lw ra, %lo(addr)(t0)
 # CHECK-INSTR: lui t0, 912092
 # CHECK-INSTR: lw ra, -273(t0)
 
-# Check that assembler can handle %hi(label1 - label2) and %lo(label1 - label2)
-# expressions.
-
-tmp1:
-  # Emit zeros so that difference between tmp1 and tmp3 is 0x30124 bytes.
-  .fill 0x30124-8
-tmp2:
-  lui t0, %hi(tmp3-tmp1)
-  lw ra, %lo(tmp3-tmp1)(t0)
-# CHECK-INSTR: lui t0, 48
-# CHECK-INSTR: lw ra, 292(t0)
-
-tmp3:
-  lui t1, %hi(tmp2-tmp3)
-  lw sp, %lo(tmp2-tmp3)(t1)
-# CHECK-INSTR: lui t1, 0
-# CHECK-INSTR: lw sp, -8(t1)
-
-# Check that a relocation isn't emitted for %hi(label1 - label2) and
-# %lo(label1 - label2) expressions.
-
 # CHECK-REL-NOT: R_RISCV