[PATCH] D122287: [XCOFF] support writing sections, relocations and symbols for XCOFF64.

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:196
   uint64_t RelocationEntryOffset = 0;
+  StringRef SourceFileName = ".file";
 
----------------
DiggerLin wrote:
>  I do not think  we will change  ".file" .  change to const StringRef SourceFileName = ".file" ?
> If the file auxiliary entry is not used, the symbol name is the name of the source file. If the file auxiliary entry is used, then the symbol name should be .file, and the first file auxiliary entry (by convention) contains the source file name. 

Our current approach is to write a symbol without the auxiliary entry, with .file as the source file's name, but in fact we should write a symbol with the real source file's name. And we commented a todo for this.

```
// FIXME: add the real source file's name.
```
So I think the SourceFileName may be changed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:751
   } else {
     W.write<int32_t>(SymbolTableEntryCount);
     W.write<uint16_t>(0); // AuxHeaderSize. No optional header for an object
----------------
DiggerLin wrote:
> SymbolTableEntryCount+1 for ".file" symbol.
```
void XCOFFObjectWriter::assignAddressesAndIndices(const MCAsmLayout &Layout) {
  // The first symbol table entry (at index 0) is for the file name.
  uint32_t SymbolTableIndex = 1;
...
  SymbolTableEntryCount = SymbolTableIndex;
}
```
The ".file" symbol has been counted.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:808-809
   }
-  W.write<uint32_t>(Reloc.SymbolTableIndex);
+  // TODO SymbolTable for XCOFF64 is not yet supported.
+  W.write<uint32_t>(is64Bit() ? 0 : Reloc.SymbolTableIndex);
   W.write<uint8_t>(Reloc.SignAndSize);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I'd have thought implementing the symbol table before relocations makes more sense.
> agree with James
Thanks, since it's hard to separate the symbol table and relocation into 2 patches, I prefer to enable both of them in this patch.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:82
+    case MCSymbolRefExpr::VK_None:
+      return {XCOFF::RelocationType::R_TOC, 15};
+    case MCSymbolRefExpr::VK_PPC_L:
----------------
DiggerLin wrote:
> I am not sure, whether I understand correct or not ?
> 1. fixup_ppc_half16ds: A 14-bit fixup corresponding to lo16(_foo) with implied 2 zero bits for instrs like 'std'.
> 2. fixup_ppc_half16dq:  A 16-bit fixup corresponding to lo16(_foo) with implied 3 zero bits for instrs like 'lxv'.
> 3. fixup_ppc_half16 : A 16-bit fixup corresponding to lo16(_foo) or ha16(_foo) for instrs like 'li' or 'addis'.
> 
> in the https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__sua3i125jbau 
> r_rsize: 0x3F(6 bits)
> Specifies the bit length of the relocatable reference **minus one**. The current architecture allows for fields of up to 32 bits (XCOFF32) or 64 bits (XCOFF64) to be relocated.
> 
> so for the
> case PPC::fixup_ppc_half16ds: 
>     {XCOFF::RelocationType::R_TOC, EncodedSignednessIndicator | 13 } 
Thanks for your catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122287