This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] add C_FILE symbol at index 0
ClosedPublic

Authored by shchenz on Feb 20 2021, 3:50 AM.

Details

Summary

This is for XCOFF DWARF support.

Seems when DWARF debug is enable, symbol 0 has special usage for AIX binder. At least, symbol 0 can not be the .text section. Otherwise, we get some binding time error.

Add correct C_FILE symbol at index 0 here to make AIX binder work.

Currently, I add no auxiliary entry for C_FILE symbol because I can not find any use for it for AIX toolchains.

Diff Detail

Event Timeline

shchenz created this revision.Feb 20 2021, 3:50 AM
shchenz requested review of this revision.Feb 20 2021, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 3:50 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
718

The symbol name is .file when the auxiliary entry is present and is the source filename when not. Ideally, we put in the intended value; otherwise, this needs to be marked with a FIXME.

718

Please add comments indicating the field being written in a manner consistent with writeSymbolTableEntryForControlSection.

719

Add comment: The "symbol value" of a C_FILE symbol is its symbol table index.

721

Add comment: The n_type field of a C_FILE symbol encodes the source language and CPU version info; zero indicates no info.

shchenz updated this revision to Diff 325361.Feb 21 2021, 6:53 PM

1: add more comments

@hubert.reinterpretcast Thanks for the review, Hubert. Updated accordingly.

llvm/lib/MC/XCOFFObjectWriter.cpp
734

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.

812–813

Suggest to make it easier for the reader to catch that the index is 0-based.

llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
141

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.

shchenz updated this revision to Diff 325677.Feb 22 2021, 10:04 PM
shchenz marked an inline comment as done.

1: comments related fix

llvm/lib/MC/XCOFFObjectWriter.cpp
734

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?

llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
141

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?

LGTM with an additional comment wording change.

llvm/lib/MC/XCOFFObjectWriter.cpp
719–720

Clarify that .file is not the correct value when no auxiliary entries are present.

734

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.

llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
141

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.

This revision is now accepted and ready to land.Feb 23 2021, 8:12 AM
shchenz marked an inline comment as done.Feb 23 2021, 7:15 PM

Thanks for your review! @hubert.reinterpretcast

llvm/lib/MC/XCOFFObjectWriter.cpp
719–720

Updated in the commit patch.

734

Yeah, we should evaluate this when we try to fix the left FIXME, especially for the multiple C_FILE parts.

llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
141

Sure, we should improve the places where we use fixed value for Index, ContainingCsectSymbolIndex, symbol index in relocation entry and so on.

This revision was landed with ongoing or failed builds.Feb 23 2021, 7:26 PM
This revision was automatically updated to reflect the committed changes.
shchenz marked an inline comment as done.