[PATCH] D138982: [XCOFF] adjust the Fixedvalue for R_RBR relocations.

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:653
+        SectionMap[SymASec]->Address - BRInstrAddress + Target.getConstant();
   }
 
----------------
Esme wrote:
> Esme wrote:
> > shchenz wrote:
> > > For the other `else`(relocation types), maybe we should add an assertion here to avoid any other unhandled relocation type that may introduce in `PPCXCOFFObjectWriter::getRelocTypeAndSignSize()` in future. The assertion will help us address the root cause early like the one for `XCOFF::RelocationType::R_RBR`.
> > Good point!
> Oh, it's incorrect to add the assertion/unreachable here, because what we are doing in the function is just to adjust the fixedvalue for some particular cases, and the fixedvalue for general cases are already calculated before this function.
Thanks. My initial thought was like:
```
else {
  assert(Type == {list all other relocation types in `PPCXCOFFObjectWriter::getRelocTypeAndSignSize()` that we know are OK with current FixedValue handling})
}
```

But this assertion may report false alarm if we add a new relocation type in `PPCXCOFFObjectWriter::getRelocTypeAndSignSize()` in future but that relocation type does not need special handling for the FixedValue.

If we know what relocation types need special handling for FixedValue and assert here that all of them are handled or known to be unhandled for now, that would be the most reasonable solution. But I guess it must need non trivial effort to understand each relocation type. So let's just back to previous version for now.

Thanks again for the try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138982