[PATCH] D83751: [MC] Support .reloc sym+constant, *, *

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Thank you for putting up a patch in response to your comment. 
Once this goes in I'll rebase D79625 <https://reviews.llvm.org/D79625> on top of it and that patch should become a lot smaller.



================
Comment at: llvm/include/llvm/MC/MCStreamer.h:1022
+                     SMLoc Loc, const MCSubtargetInfo &STI) {
+    return None;
   }
----------------
I feel like you have changed the meaning of this default behaviour.  Was that the intention?
Before we used to `return true` which indicated that the reloc could not be emitted. Now you return `None` meaning it succeeded. 


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:688
+    return std::make_pair(false,
+                          std::string(".reloc offset is not relocatable"));
+  if (OffsetVal.isAbsolute()) {
----------------
I assume this is what you meant by simpler code. We already have a function to do this computation. :)


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:704
+        MCFixup::create(SRE.getSymbol().getOffset() + OffsetVal.getConstant(),
+                        Expr, Kind, Loc));
+    return None;
----------------
One of the issues I noticed with this situation is that the symbol may not be in the same data fragment as the `.reloc` directive. The code you have assumes that it is in the same DF.

I used the same DF as the symbol:
```
  MCFragment *Fragment = Symbol->getSymbol().getFragment();
  if (!Fragment || Fragment->getKind() != MCFragment::FT_Data)
    return false;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83751/new/

https://reviews.llvm.org/D83751