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

Mon Feb 22 22:04:51 PST 2021

shchenz 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);
----------------
hubert.reinterpretcast wrote:
> 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.
We can get filename info from `MCAssembler`, but there is not an easy way to do so for now. There may be more than one 1 file in one object, so `getFileNames` returns a vector of all the declared files in this object and we also need to associate the symbols with the files. It is better to do this in another patch. Let's keep using FIXME for now?

================
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
----------------
hubert.reinterpretcast wrote:
> 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`.
yeah, this should be done surely. The fixed value makes `INDX` have no meaning at all. Do you agree to defer this a little bit, for example after the XCOFF DWARF support is committed, since this issue exists before this patch?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97117