[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.

Thanks


================
Comment at: include/llvm/MC/MCObjectStreamer.h:127-128
@@ -126,2 +126,4 @@
   void EmitGPRel64Value(const MCExpr *Value) override;
+  bool EmitRelocDirective(const MCExpr &Offset, StringRef Name,
+                          const MCExpr &Expr, SMLoc Loc = SMLoc()) override;
   void EmitFill(uint64_t NumBytes, uint8_t FillValue) override;
----------------
majnemer wrote:
> Will there be any direct calls of this? If not, I'd just drop the default argument.
Probably not, I'll drop the default part. MIPS16 might use it at some point in the future but I can always bring it back later if it's required.

================
Comment at: include/llvm/MC/MCStreamer.h:692
@@ +691,3 @@
+                                  const MCExpr &Expr, SMLoc Loc = SMLoc()) {
+    return false;
+  }
----------------
majnemer wrote:
> Shouldn't this return `true` because nothing was emitted?
I think `false` makes the most sense. Returning true would cause all targets to silently accept and ignore any relocation name. By returning false, we emit an error message about the unknown/unsupported name.

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4528-4531
@@ +4527,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:
> Not all targets support an addend, this should be predicated on a hook.
This isn't the addend, it's the section offset the relocation is applied to. I believe you're thinking of the expression that's parsed on line 4547. If that's the case, I agree. I'll also make that expression optional like it should be.


http://reviews.llvm.org/D13659