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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:704
+        MCFixup::create(SRE.getSymbol().getOffset() + OffsetVal.getConstant(),
+                        Expr, Kind, Loc));
+    return None;
----------------
stefanp wrote:
> MaskRay wrote:
> > stefanp wrote:
> > > 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;
> > > ```
> > GNU as somehow allows the following (it behaves as if .reloc is placed in `.data`)
> > 
> > ```
> > .text
> > .reloc .data, R_X86_64_NONE, 0
> > 
> > .data
> > .globl 
> > data:
> > ```
> > 
> > It needs thoughts how to handle/detect this. This probably can go to a separate change.
> I wouldn't know where to start with that `.data` reloc because I don't think we would be able to get the DF for it. I almost feel like we would have to make changes to `ELFObjectWriter::recordRelocation` and at least allow `MCFixup::create` to accept more than `uint32_t` as the offset. But that is a lot of work. Definitely a different patch.
> 
> Either way, I don't need the `.data` but I will need to cover the situation where the DF is different for the symbol. This is for code that will be coming from me later on. For this patch if you want to leave it as a TODO I'm ok with that. I can try to address it in the rework of D79625.
I'll leave it as a TODO. This did not work correctly before. The intention of the patch is just to add offset support, not thinking hard about how a foreign MCDataFragment works...

Do you have more review comments:) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83751