[PATCH] D72027: [XCOFF][AIX] Support basic relocation type on AIX

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked 2 inline comments as done.
jasonliu added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:611
 
+  assert(sizeof(XCOFFRelocation32) == XCOFF::RelocationSerializationSize32);
   auto RelocationOrErr =
----------------
DiggerLin wrote:
> I do not think we need to add assert here, implement of XCOFFRelocation32 is packed alignment.
Yes, it is packed alignment now, the assertion here is to make sure it will always be packed alignment in the future. So I don't think adding assertion here is a bad thing. 


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:22
 class PPCXCOFFObjectWriter : public MCXCOFFObjectTargetWriter {
+  static constexpr uint8_t SignBitMask = 0x80;
+
----------------
DiggerLin wrote:
> jasonliu wrote:
> > hubert.reinterpretcast wrote:
> > > There's `XR_SIGN_INDICATOR_MASK` in the codebase already.
> > Unfortunately that's in llvm/Object. And we should have put it in llvm/BinaryFormat. 
> we maybe need to have NFC patch to put the XR_SIGN_INDICATOR_MASK, XR_FIXUP_INDICATOR_MASK,  XR_BIASED_LENGTH_MASK into XCOFF.h , and shared with XCOFFObjectFile.h
I agree we should have a NFC patch to clean it up. I think we could do that after this lands. 


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

https://reviews.llvm.org/D72027