[PATCH] D79625: [PowerPC] Extend .reloc directive on PowerPC

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:671
+                         MCDataFragment *&DF) {
+  if (Symbol.isVariable()) {
+    const MCExpr *SymbolExpr = Symbol.getVariableValue();
----------------
MaskRay wrote:
> The verification part does not have a close relation with the rest of PPC specific handling. I think we should move this part to a separate patch.
> 
> IIUC, without the verification part, the functionality still exists. It is just that we can miss some erroneous cases.
I'm not sure what you mean in this case.

I do need to have a separate case for the `Symbol.isVariable()` situation. Otherwise I get a `"Cannot get offset for a common/variable symbol"` error. 
Do you mean I should remove checking like this:
```
if(!SymbolExpr->evaluateAsRelocatable(OffsetVal, nullptr, nullptr))
return std::make_pair(false,
    std::string("symbol in .reloc offset is not "
          "relocatable"));
```



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp:390
+    case PPC::fixup_ppc_linker_opt:
+      Type = ELF::R_PPC64_PCREL_OPT;
+      break;
----------------
MaskRay wrote:
> Can we just use FirstLiteralRelocationKind+R_PPC64_PCREL_OPT and not define `fixup_ppc_linker_opt`?
Ok, I'll get rid of that the fixup name.
I'll update the patch and  you can see if that is what you are looking for.

I do have a question related to this though... In the future I will want to generate fixups of this type. Usually I would do that like this:
```
MCFixup::create(Offset, Expr, (MCFixupKind)PPC::fixup_ppc_linker_opt, Loc);
```
If I completely remove this naming would I be doing this instead?
```
MCFixup::create(Offset, Expr, FirstLiteralRelocationKind, Loc);
```
Is that the right way to do it?


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

https://reviews.llvm.org/D79625