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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked an inline comment as done.
shchenz added a comment.

Thanks for your review! @hubert.reinterpretcast



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:808-809
+  //
+  // n_name.
+  // FIXME: add source filename as the first auxiliary entry.
+  writeSymbolName(".file");
----------------
hubert.reinterpretcast wrote:
> Clarify that `.file` is not the correct value when no auxiliary entries are present.
Updated in the commit patch.


================
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:
> shchenz wrote:
> > 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?
> I'm fine with leaving the FIXME at this time, but I am a bit concerned that the assembly path and the object path might end up differing on its idea of what the source filenames are for the purposes of C_FILE symbols.
Yeah, we should evaluate this when we try to fix the left FIXME, especially for the multiple C_FILE parts.


================
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:
> shchenz wrote:
> > 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?
> I'm fine with deferring a bit. If there's a next time, we really should fix it separately instead of updating the fixed value again.
Sure, we should improve the places where we use fixed value for `Index`, `ContainingCsectSymbolIndex`, symbol index in relocation entry and so on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97117