This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Avoid unset csect assert for functions defined after their use in TOC
ClosedPublic

Authored by daltenty on Dec 6 2019, 8:54 AM.

Details

Summary

If a function is defined after it appears in a TOC expression, we may
try to access an unset containing csect when returning a symbol for the
expression.

Diff Detail

Event Timeline

daltenty created this revision.Dec 6 2019, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2019, 8:54 AM
DiggerLin added inline comments.Dec 6 2019, 2:33 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1740

for global ,I checked the gcc generated Functions Description as HIDDEN.
for static function, I checked both gcc and xlc generated Functions Description as HIDDEN.

Xiangling_L added inline comments.Dec 8 2019, 1:45 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1740

For static function, Digger is right, function descriptor symbol should have internal storage type.

But for external linkage function, I think what Digger mentioned I checked the gcc generated Functions Description as HIDDEN should be interpreted as GCC generates two symbols for function descriptor, one is .csect foo[DS], the SD symbol has C_HIDEXT storage type, the other one is foo as a label within the csect which has C_EXT SC? And the label one plays the important role in GCC case to expose foo's function descriptor when the address of function is taken.

So in LLVM, since we use the same symbol to represent the foo in .csect foo[DS] and the foo as label, I think the storage class here should follow the linkage type of function symbol, like:

const XCOFF::StorageClass SC =
            TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);

 MCSectionXCOFF *FnDescSec = OutStreamer->getContext().getXCOFFSection(
      CurrentFnDescSym->getName(), XCOFF::XMC_DS, XCOFF::XTY_SD,
      SC, SectionKind::getData());
DiggerLin added inline comments.Dec 9 2019, 12:04 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1905

can we implement as
if (GOKind.isText()) {

if (!XSym->hasContainingCsect()) {
   .......
   XSym->setContainingCsect(Csect);
 }

return XSym->getContainingCsect()->getQualNameSymbol();
}

hubert.reinterpretcast added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1740

I agree with @Xiangling_L. The storage class should match that of the function symbol itself.

This patch might need splitting. At least the commit subject should encompass the scope more. Once the tests are added for weak external or internal linkage, the "drive-by" change will be sizable.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1902–1912

The binding of "initialized" is unclear here. Do you mean "defined" functions?

1910

Note regarding the linkage here as well.

daltenty marked 2 inline comments as done.Dec 10 2019, 10:06 AM
daltenty added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1740

I agree. I originally intended to use getStorageClassForGlobal, but for some reason the XCOFF object file format document declares that CSects of mapping class have XMC_DS have storage class value of C_EXT.

As noted this doesn't make much sense, so I will update this as described.

1905

I'm thinking we may need to refactor how we setup containing csects for GlobalObjects in general. We end up duplicating the logic for what type of csect to create for a particular type of global in multiple places, and its not clear who owns setting up the containing csect (i.e. is it the first place we need the csect, or emitGlobalVariable and friends?).

Perhaps getSymbol() could setup the containing CSect if it doesn't exist. Or maybe some type of getOrCreateContainingCsect(GlobalObject) function is more appropriate.

Anyway, I'd like to address this in a follow on refactor patch, since this one is already getting large in scope.

hubert.reinterpretcast requested changes to this revision.Dec 14 2019, 5:45 PM

The scope of the "drive-by" change is heading into redesign territory.

llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll
59 ↗(On Diff #232583)

This is not okay. There is already an external label definition with the same unqualified name. If we want to consider making the csect external, then we should stop creating the label. We will have to use a qualified name for the .globl in the assembly.

This revision now requires changes to proceed.Dec 14 2019, 5:45 PM

The scope of the "drive-by" change is heading into redesign territory.

I agree, I'm going to split the function descriptor change out and adopt the current mapping class of C_HIDEXT for this change.

daltenty updated this revision to Diff 234121.Dec 16 2019, 12:50 PM
  • Remove function descriptor storage changes. They can best be reviewed in their own patch.
daltenty marked 2 inline comments as done.Dec 16 2019, 12:53 PM
daltenty updated this revision to Diff 234123.Dec 16 2019, 12:53 PM
  • Update comment
Harbormaster completed remote builds in B42586: Diff 234123.

@daltenty, please update the commit message to indicate the corrected scope. This LGTM insofar as it fits the current handling of function descriptors for which we have a definition in the current object.

This revision is now accepted and ready to land.Dec 16 2019, 3:43 PM
daltenty edited the summary of this revision. (Show Details)Dec 17 2019, 7:45 AM
This revision was automatically updated to reflect the committed changes.