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

[Daniel Sanders via llvm-commits]

dsanders added inline comments.

================
Comment at: lib/MC/MCObjectStreamer.cpp:447-448
@@ +446,4 @@
+  if (Assembler->getBackend().getFixupKind(Name, Kind)) {
+    if (Expr == nullptr)
+      Expr = MCSymbolRefExpr::create(getContext().createTempSymbol(), getContext());
+    DF->getFixups().push_back(MCFixup::create(OffsetValue, Expr, Kind, Loc));
----------------
majnemer wrote:
> I don't believe you have a test case which exercises this case.  I'm a little confused as to why we need to create a symbol here. GNU as doesn't seem to do this:
>   $ cat t.s
>   .text
>   .long 0
>   .reloc .text, R_X86_64_32
>   $ as t.s -o t.o
>   $ objdump -r t.o
>   t.o:     file format elf64-x86-64
> 
>   RELOCATION RECORDS FOR [.text]:
>   OFFSET           TYPE              VALUE                                                                                                                                                   
>   0000000000000000 R_X86_64_32       *ABS*
It's the:
  .reloc 12, R_MIPS_NONE # ASM: .reloc 12, R_MIPS_NONE{{$}}

The temporary symbol arose from experimentation. Leaving Expr as nullptr leads to a crash and using '0' causes the reloc to be omitted in the output. Using a temporary symbol like this causes both 'objdump -dr' and 'llvm-readobj -sections -section-data -r'* output to match between the integrated assembler and gas.

*llvm-readobj does show differences compared to gas but they aren't related to the reloc or the text section.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4603
@@ -4596,1 +4602,3 @@
 
+bool AsmParser::parseDirectiveReloc(SMLoc DirectiveLoc) {
+  const MCExpr *Offset;
----------------
majnemer wrote:
> It might be nice to have a comment here describing the syntax using pseudo-EBNF.  We do this for some directives like `parseDirectiveRept` and it makes it a little easier to see the "big idea".
I noticed this when I moved the implementation near parseDirectiveValue. I've already added one in my working copy.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4628-4629
@@ +4627,4 @@
+    Lexer.Lex();
+    if (parseExpression(Expr))
+      return true;
+  }
----------------
majnemer wrote:
> I think we should `evaluateAsRelocatable` the `Expr` to sanity check the expression. Stuff like `.text + .text` should fail with a nice message.
Ok, that makes sense.


http://reviews.llvm.org/D13659