[PATCH] D13659: Implement .reloc (constant offset only) with support for R_MIPS_NONE and R_MIPS_32.

[Daniel Sanders via llvm-commits]

dsanders added a comment.

In http://reviews.llvm.org/D13659#276919, @majnemer wrote:

> What happens when you do:
>
>   .text
>   .long .text+2
>   .reloc 0, R_MIPS_32, .text+3


I get:

  Disassembly of section .text:
  
  00000000 <.text>:
     0:	00000005 	lsa	zero,zero,zero,0x1
  			0: R_MIPS_32	.text
  			0: R_MIPS_32	.text
  	...

The two addends are summed and stored in the section while two R_MIPS_32 relocs for .text are emitted.


================
Comment at: lib/MC/MCParser/AsmParser.cpp:368
@@ -367,3 +367,3 @@
     DK_ERR, DK_ERROR, DK_WARNING,
-    DK_END
+    DK_END, DK_RELOC
   };
----------------
majnemer wrote:
> It'd be nicer if this could be closer to `DK_VALUE`.  I'd just leave it on it's own line.
Ok

================
Comment at: lib/MC/MCParser/AsmParser.cpp:488-489
@@ -487,1 +487,4 @@
 
+  // ".reloc"
+  bool parseDirectiveReloc(SMLoc DirectiveLoc);
+
----------------
majnemer wrote:
> Could this be moved near the handler for `.value` ?
Ok

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4611-4614
@@ +4610,6 @@
+
+  // We can only deal with constant expressions at the moment.
+  int64_t OffsetValue;
+  if (!Offset->evaluateAsAbsolute(OffsetValue))
+    return Error(OffsetLoc, "expression is not a constant value");
+
----------------
majnemer wrote:
> dsanders wrote:
> > Do you know any cases where the addend isn't supported? The GAS documentation says that ELF REL targets such as 386 don't support addends but I'm finding they work. Similarly, addends are also working for MIPS O32 which is also a REL target.
> NT targets use REL relocations and they work with mingw assemblers.  Perhaps the comment is out of date.
Seems like it. Shall we leave this patch as always accepting addends and follow-up later if we find a case that doesn't work?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsFixupKinds.h:27
@@ +26,3 @@
+    // Branch fixups resulting in R_MIPS_NONE.
+    fixup_Mips_None = FirstTargetFixupKind,
+
----------------
majnemer wrote:
> Would it be more consistent to name this `fixup_Mips_NONE` ?
I agree. There's a couple inconsistencies below but there's no need to add more


http://reviews.llvm.org/D13659