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

Mon Jan 6 20:33:15 PST 2020

hubert.reinterpretcast added inline comments.

================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:75
+  for (auto &Fixup : Fixups) {
+    Fixup.setOffset(Fixup.getOffset() + DF->getContents().size());
+    DF->getFixups().push_back(Fixup);
----------------
Does the reference returned by `DF->getContents()` change between loop iterations? For that matter, does the value returned by `size()`?

================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:76
+    Fixup.setOffset(Fixup.getOffset() + DF->getContents().size());
+    DF->getFixups().push_back(Fixup);
+  }
----------------
Does the reference returned by `DF->getFixups()` change between loop iterations?

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:51
+// however we only use 10 byte spaces when writing it out.
+constexpr unsigned SizeOfXCOFFRelocation32 = 10;
 constexpr unsigned DefaultSectionAlign = 4;
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > DiggerLin wrote:
> > > can we use sizeof(XCOFFRelocation) when using SizeOfXCOFFRelocation32 ?
> > No, as I'm trying to express in the comments above, the sizeof(XCOFFRelocation) is 12, but actual size is 10. 
> I think we can drop "XCOFF"; this is a .cpp file that is XCOFF-specific.
> 
> Suggestion: `RelocationSerializationSize32`.
I believe that `sizeof(llvm::object::XCOFFRelocation32)` is 10 (and we should not hard-code `10` here).

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:58

+struct XCOFFRelocation {
+  uint32_t SymbolTableIndex;
----------------
I just want to make sure we have a good reason (e.g., a sizable performance benefit) for having this in-memory version instead of reusing the `llvm::object::XCOFFRelocation32` serialization version.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:163

+  // Maps the MC Section representation to its corrresponding ControlSection
+  // wrapper. Needed for finding the ControlSection to insert an MCSymbol into
----------------
s/MC Section/MCSection/;

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:163

+  // Maps the MC Section representation to its corrresponding ControlSection
+  // wrapper. Needed for finding the ControlSection to insert an MCSymbol into
----------------
hubert.reinterpretcast wrote:
> s/MC Section/MCSection/;
s/corrresponding/corresponding/;

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:166
+  // from its containing MCSectionXCOFF.
+  DenseMap<MCSectionXCOFF const *, ControlSection *> SectionMap;
+
----------------
The east const here does not agree with line 196, which uses west const.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:168
+
+  // Maps the MCSymbol representation to its corrresponding symbol table index.
+  // Needed for relocation.
----------------
s/corrresponding/corresponding/;

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:170
+  // Needed for relocation.
+  DenseMap<MCSymbol const *, uint32_t> SymbolIndexMap;
+
----------------
Fix the east const here too.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:257
 void XCOFFObjectWriter::reset() {
-  UndefinedCsects.clear();
+  // Clear mappings we created.
+  SymbolIndexMap.clear();
----------------
Insert "the" before "mappings".

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:608
+    W.write<uint32_t>(Sec->FileOffsetToRelocations);
+    // Line number pointer. Not supported yet.
     W.write<uint32_t>(0);
----------------
Blank line before the comment please.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:612
+    W.write<uint16_t>(Sec->RelocationCount);
+    // Line number counts. Not supported yet.
     W.write<uint16_t>(0);
----------------
Blank line before the comment please.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:629
+  for (const auto *Section : Sections) {
+    // Nothing to write for this Section.
+    if (Section->Index == Section::UninitializedIndex)
----------------
Please move the comment into the `if`.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:676
+  for (auto *Section : Sections) {
+    // Nothing to record for this Section.
+    if (Section->Index == Section::UninitializedIndex)
----------------
Please move the comment into the `if`.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:689
+
+  // Calculate file offset to relocation entries.
+  uint32_t RawPointer = RelocationEntryOffset;
----------------
Add "the" before "file offset" and "relocation entries".

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:696
+    Sec->FileOffsetToRelocations = RawPointer;
+    RawPointer += Sec->RelocationCount * SizeOfXCOFFRelocation32;
+  }
----------------
Both the multiplication and the addition can overflow here.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:699
+
+  // TODO What to align the SymbolTable too?
+  // TODO Error check that the number of symbol table entries fits in 32-bits
----------------
It seems there is no special alignment requirement for the symbol table. Alignment as low as 2-byte has been observed.

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:22
 class PPCXCOFFObjectWriter : public MCXCOFFObjectTargetWriter {
+  static constexpr uint8_t SignBitMask = 0x80;
+
----------------
There's `XR_SIGN_INDICATOR_MASK` in the codebase already.

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:24
+
+protected:
+  uint8_t getRelocType(const MCValue &Taraget,
----------------
Is there a reason here to override public member functions of a public base class as protected?

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:48
+                          : Target.getSymA()->getKind();
+  unsigned Type;
+  switch ((unsigned)Fixup.getKind()) {
----------------
Can we just return directly instead of assigning to a variable first?

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:71
+
+uint8_t PPCXCOFFObjectWriter::getRelocSignAndSize(const MCFixup &Fixup,
+                                                  bool IsPCRel) const {
----------------
There is a chance that merging this with the previous function would improve the logic and reduce parallel maintenance. In particular, the "magic numbers" are easier to swallow if they are paired with the XCOFF relocation type.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll:125
+; DIS-NEXT:       14: 60 00 00 00                   nop
+; DIS-NEXT:       18: 80 82 00 00                   lwz 4, 0(2)
+; DIS-NEXT:       1c: 80 84 00 00                   lwz 4, 0(4)
----------------
It would be better if there was a case where the TOC entry is at a non-zero offset from the TOC base.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll:140
+; DIS-NEXT:       40: 00 00 00 00                   <unknown>
+; DIS:      00000044 globalA:
+; DIS-NEXT:       44: 00 00 00 34                   <unknown>
----------------
@DiggerLin, I think this makes a good case for printing the storage mapping class for other-than-label-definition symbols.

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

https://reviews.llvm.org/D72027