This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] write the real source file name in C_FILE symbol.
ClosedPublic

Authored by Esme on May 29 2022, 6:38 PM.

Diff Detail

Event Timeline

Esme created this revision.May 29 2022, 6:38 PM
Herald added a project: Restricted Project. · View Herald Transcript
Esme requested review of this revision.May 29 2022, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2022, 6:38 PM
shchenz added inline comments.May 29 2022, 9:43 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
498

What if we are compiling a .s file and there are more than one .file in the file?

Esme updated this revision to Diff 435418.Jun 8 2022, 9:35 PM

Considered the scenario of multiple source files.

MaskRay added inline comments.Jun 8 2022, 9:37 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
980

llvm coding standard uses pre-increments.

Esme updated this revision to Diff 435420.Jun 8 2022, 9:44 PM

Addressed comments and added a test for this patch.

jhenderson added inline comments.Jun 13 2022, 1:17 AM
llvm/test/MC/PowerPC/aix-symtbl-order.s
4 ↗(On Diff #435420)

Do you mean "when support is added to llvm-mc"? "when llvm-mc is supported" implies the application isn't yet supported, but you are actively using it in this test.

Esme updated this revision to Diff 437443.Jun 15 2022, 9:58 PM

Addressed James's comments.

No further comments from me at this time, but I'm not knowledgeable enough about this area to be able to LGTM it.

Esme updated this revision to Diff 438228.Jun 19 2022, 6:45 PM

The AIX assembler doesn't really support multiple .file statements and its output symbol table starts with all the .file symbols.
This update drops the symbol order logic from the previous version in favor of a behavior consistent with the AIX assembly.

shchenz added inline comments.Jun 19 2022, 6:48 PM
llvm/test/MC/PowerPC/aix-symtbl-order.s
7 ↗(On Diff #437443)

We are over design here? Like what we discussed offline, AIX assembler does not have the functionality to guarantee that the local/global symbols are associated with their precedent .file directive. I think it is better for us to do as AIX assembler does, just generate the C_FILE symbols for each .file and don't do any symbol scope related handling.

8 ↗(On Diff #437443)

Nit: for function entry, we need to make it like .foo.

oops, my last comment is for previous change, don't aware the patch is already updated when I posted that comment.

llvm/lib/MC/XCOFFObjectWriter.cpp
498

Seems all the .file symbols are all changed to <stdin>? Can we add a new case for this branch?

861

Nits: Write C_FILE symbols?

964–965

C_FILE?

Esme updated this revision to Diff 438320.Jun 20 2022, 3:09 AM

Addressed @shchenz's comments. Thanks!

shchenz accepted this revision as: shchenz.Jun 20 2022, 3:30 AM

LGTM with nits. Thanks for fixing this.

llvm/test/MC/PowerPC/aix-file-symbols-empty.s
6

nit: var1 and var2 are both function entries by default. We need to name them as .var1 and .var2.

llvm/test/MC/PowerPC/aix-file-symbols.s
10

Ditto.

This revision is now accepted and ready to land.Jun 20 2022, 3:30 AM
Esme updated this revision to Diff 438952.Jun 22 2022, 2:37 AM

Addressed comments.

This revision was landed with ongoing or failed builds.Jun 22 2022, 3:26 AM
This revision was automatically updated to reflect the committed changes.