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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:1022
+                     SMLoc Loc, const MCSubtargetInfo &STI) {
+    return None;
   }
----------------
stefanp wrote:
> 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. 
This is intentional. parseDirectiveReloc tried doing things duplicated with emitRelocDirective (evaluate absolute). By moving logic to emitRelocDirective, we have to let emitRelocDirective handle diagnostics.


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:704
+        MCFixup::create(SRE.getSymbol().getOffset() + OffsetVal.getConstant(),
+                        Expr, Kind, Loc));
+    return None;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83751