[PATCH] D97117: [XCOFF] add C_FILE symbol at index 0

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:822
+  // n_numaux. No aux entry for now.
+  // FIXME: add source filename as the first auxiliary entry.
+  W.write<uint8_t>(0);
----------------
I think the FIXME needs to be where the `.file` is hardcoded. The issue is not merely that the source filename information is not provided; the information is being encoded like it is being provided (with `.file` being the source filename). The `AsmPrinter::doInitialization` code seems to be able to get the info needed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:908
 void XCOFFObjectWriter::assignAddressesAndIndices(const MCAsmLayout &Layout) {
-  // The first symbol table entry is for the file name. We are not emitting it
-  // yet, so start at index 0.
-  uint32_t SymbolTableIndex = 0;
+  // The first symbol table entry is for the file name.
+  uint32_t SymbolTableIndex = 1;
----------------
Suggest to make it easier for the reader to catch that the index is 0-based.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll:141
 ; SYMS-NEXT:       Index: [[#INDX+3]]
-; SYMS-NEXT:       ContainingCsectSymbolIndex: 2
+; SYMS-NEXT:       ContainingCsectSymbolIndex: 3
 ; SYMS-NEXT:       ParameterHashIndex: 0x0
----------------
At least for this file, I think a follow-up NFC patch to fix this to use `INDX` should be created. The fixed value here does not appear to be checked (in a similarly fixed manner) as the `Index` field value for a `Symbol`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97117