This is an archive of the discontinued LLVM Phabricator instance.

Use empty symbol name for XCOFF text csect
ClosedPublic

Authored by stephenpeckham on Jul 10 2023, 8:29 AM.

Details

Summary

When generating XCOFF, the compiler generates a csect with an internal name. Each function results in a label within the csect. This patch replaces the internal name ".text" with an empty string "". This avoids adding special code to handle a function text() in the source file, and works better with some XCOFF tools that are confused when the csect and the first function have the same address.

Diff Detail

Event Timeline

stephenpeckham created this revision.Jul 10 2023, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 8:29 AM
stephenpeckham requested review of this revision.Jul 10 2023, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 8:29 AM
lkail removed a reviewer: stephenpeckham.
llvm/lib/MC/MCObjectFileInfo.cpp
923–924

Comment wording suggestion.

llvm/test/CodeGen/PowerPC/aix-pgo-function-sections.ll
123 ↗(On Diff #538658)

It is somewhat unfortunate that the default tool outputs for empty names is especially bad, but I don't want to hold up this patch on changing that.

llvm/test/CodeGen/PowerPC/aix-text.ll
3

Since this is a new test, I think we should use --symbol-description (same for other case below).

41

I think it should be possible to remove this (and the directive referring to this). Please try to remove the other metadata and attributes (and their uses) as well.

llvm/test/CodeGen/PowerPC/aix-text.ll
3

@stephenpeckham, while the case where text() is defined needs function-sections to exhibit the failure, there is another case where text() is an external reference that does not need function-sections.

I suggest adding that additional test as well.

hubert.reinterpretcast edited the summary of this revision. (Show Details)Jul 11 2023, 5:23 PM
stephenpeckham marked 4 inline comments as done.Jul 12 2023, 12:12 PM
stephenpeckham added inline comments.
llvm/test/CodeGen/PowerPC/aix-pgo-function-sections.ll
123 ↗(On Diff #538658)

A change to llvm-objdump that prints a visible string for empty names could be done in another patch.

llvm/test/CodeGen/PowerPC/aix-pgo-function-sections.ll
123 ↗(On Diff #538658)

Sounds good. Such a future patch should probably update all of the cases changed here too (although most of them will continue to pass even without such a future update).

llvm/test/CodeGen/PowerPC/aix-text.ll
3

This test (where text is defined as a function) needs function-sections to exhibit the problem.
A separate test (different source), where text is an undefined function being referenced, exhibits the problem with or without function-sections. In other words, a different test file is needed to capture the additional case.

32–33

I expect we will see these named with the storage mapping class?

36–37

Same comment.

stephenpeckham marked 3 inline comments as done.
This revision is now accepted and ready to land.Jul 13 2023, 8:17 AM
daltenty accepted this revision.Jul 14 2023, 9:54 AM
daltenty added a subscriber: daltenty.

LGTM with some minor nits

llvm/test/CodeGen/PowerPC/aix-text-ref.ll
7–9

nit: you can remove all this metadata, since it doesn't affect the test. The defaults will be supplied by your llc invocation.

21–25

nit: ditto the earlier comment, you can strip all this metadata out to simplify the test

llvm/test/CodeGen/PowerPC/aix-text.ll
11

nit: ditto the metadata comments about the other test file.

This revision was landed with ongoing or failed builds.Jul 15 2023, 1:14 PM
This revision was automatically updated to reflect the committed changes.