Page MenuHomePhabricator

[AIX] emit .extern and .weak directive linkage
ClosedPublic

Authored by DiggerLin on Mar 27 2020, 8:47 AM.

Details

Summary
  1. emit .extern and .weak directive linkage

the c source code for test cases aix-extern.ll , aix-weak.ll , aix-extern-weak.ll as
bash> cat aix_extern.c

extern int bar_extern(int* ip);
extern int bar_ref();
extern int b_e;

void foo(){ }

int (*bar_p)() = bar_ref;

int main() {
   bar_extern(&b_e);
   foo();
   bar_p();
   bar_ref();
}

bash>cat aix_weak.c

__attribute__ ((weak)) void foo_weak(int *p){
  (*p)++;
}

__attribute__ ((weak)) void foo_ref_weak(){
}

void (*foo_weak_p)() = foo_ref_weak;
int __attribute__ ((weak)) b;

int main() {
   foo_weak_p();
   foo_weak(&b);
   foo_ref_weak();
}

bash>cat aix_extern_weak.c

extern __attribute__ ((weak)) void foo_ext_weak(int* );
extern __attribute__ ((weak)) void foo_ext_weak_ref(void);
extern __attribute__ ((weak)) int b;

void (*foo_ext_weak_p)() = foo_ext_weak_ref;
void (*foo_ext_weak_p_nocall)() = foo_ext_weak_ref;

int main() {
   foo_ext_weak_p();
   foo_ext_weak(&b);
}

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin marked 2 inline comments as done.Apr 8 2020, 10:39 AM

address comment and rebased the patch on D77080 [NFC][XCOFF][AIX] Refactor get/setContainingCsect

jasonliu added inline comments.Apr 8 2020, 10:59 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1531

Yes, but foo[PR]->hasRepresentedCsectSet() will always return true, because whenever we created a qualname will always have csect set. How will we know if foo() function is called directly then?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
397–399

I have my doubts that CommonLinkage should produce .weak for XCOFF. It is working right because we don't actually get here from PPCAIXAsmPrinter::emitGlobalVariable for that case. I'm not sure what the best way would be to add an assertion here though.

435

Similarly, this is okay without extra checks because non-XCOFF cases should never get here. A check would be nice but I'm not sure how to formulate it in this context.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
12

I would prefer to test the function call and the take-address-of-function cases separately here.

14

There should be testing for extern_weak variables as well.

DiggerLin marked 2 inline comments as done.Apr 9 2020, 6:33 AM
DiggerLin added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1531

we only deal with extern function(we do not deal with definition function) here, for extern function, it not always has MCSectionXCOFF, it only create the extern function be called directly in the llvm/lib/CodeGen/MachineModuleInfo.cpp line 108~116, unless we delete the code later. if the code is not changed. for extern function foo , the .foo[PR] -> hasRepresentedCsectSet() when directly call. otherwise false.

jasonliu added inline comments.Apr 9 2020, 7:45 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1531

Okk. I think I got what you mean. If .foo[PR] is created then it means there is a direct call. Otherwise, we will not have a .foo[PR], and of course that newly created .foo[PR] will not have a RepresentedCsect.
Thanks.

jasonliu added inline comments.Apr 9 2020, 8:08 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1512

Based on our discussion, this assertion could be removed.
And please add a comment saying
// If there is a direct call to this extern function, we need to emit linkage for its function entry point symbol.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
12

I think this is to test when the function is both taken address, and get called directly, will we emit linkage for both function entry point and function descriptor symbol? The original implementation only emit one of them.
Maybe it makes sense to have a separate test case like this to test the linkage emit in all circumstance:

void foo_called();
void foo_refed();
void foo_called_refed();

void (*foo_refed_p)() = foo_refed;
void (*foo_called_refed_p)() = foo_called_refed;

void bar(){
  foo_called();
  foo_called_refed();
}
jasonliu added inline comments.Apr 9 2020, 8:10 AM
llvm/test/CodeGen/PowerPC/aix-extern.ll
15

nit: Function Attrs and #0 could be removed

DiggerLin edited the summary of this revision. (Show Details)Apr 14 2020, 7:20 AM
DiggerLin updated this revision to Diff 257319.Apr 14 2020, 7:25 AM
DiggerLin marked 2 inline comments as done.

address comment and add a new test case

DiggerLin updated this revision to Diff 257473.Apr 14 2020, 1:28 PM
DiggerLin edited the summary of this revision. (Show Details)

address comment

jasonliu added inline comments.Apr 15 2020, 7:56 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
435

Maybe an assert on isOSBinFormatXCOFF is better?
If not, could we remove the else and llvm_unreachable to let it fall through? Will it have warnings?
Or we could just only remove else here.

1506

This block of code and D78045 will have conflict. One of us will need to rebase.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1618–1619

This comment does not apply any more.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
11

Do we care to check the 64 bit object generation error here and for other test cases that are applicable?

30

Could we align the assembly output in aix-extern-weak.ll, aix-extern.ll, aix-weak.ll?

49

If the plan is not emit function entry point if it's not necessary, let's CHECK-NOT that.

llvm/test/CodeGen/PowerPC/aix-extern.ll
15

This comment is not addressed.

DiggerLin marked 8 inline comments as done.Apr 15 2020, 1:23 PM
DiggerLin added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
435

yes. remove else , and let it fall through , there is a warning.

1506

the one who land later will rebase.

DiggerLin updated this revision to Diff 257827.Apr 15 2020, 1:40 PM
DiggerLin marked 2 inline comments as done.

address comment

jasonliu accepted this revision.Apr 16 2020, 12:04 PM

LGTM. Please wait a day or two to see if @hubert.reinterpretcast have further comments.

This revision is now accepted and ready to land.Apr 16 2020, 12:04 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
439

We can fall through using an annotation to suppress the warning:

LLVM_FALLTHROUGH;
1506

My understanding is that this would need a semantic reconciliation. I'd like to see the rebase of this patch before approving. Also, this would be another place to look into for the follow-up mentioned in https://reviews.llvm.org/D78045?id=257331#inline-714634 @jasonliu.

1515

A comment here to explain that we are emitting linkage for the function descriptor would help.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2136

Swap the order of the two LinkOnce* cases to match their definition order.

2141–2142

Since AvailableExternallyLinkage is the only value left, just replace default with that and change the report_fatal_error to say AvailableExternallyLinkage for its first instance of "linkage".

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1619–1620

This should probably be isDeclarationForLinker. It seems we need a larger followup for AvailableExternallyLinkage that would involve checking our calls to isDeclaration:

define void @_Z1gv() {
entry:
  call void @_Z1fIiEvv()
  ret void
}

; Function Attrs: inlinehint nounwind
define available_externally void @_Z1fIiEvv() #0 {
entry:
  ret void
}

attributes #0 = { inlinehint nounwind }
DiggerLin marked 7 inline comments as done.Apr 17 2020, 1:26 PM
DiggerLin added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1506

rebase the patch on the D78045

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1619–1620
bool isDeclarationForLinker() const {
  if (hasAvailableExternallyLinkage())
    return true;
 
  return isDeclaration();
}

since we do not deal with AvailableExternallyLinkage in AsmPrinter::emitLinkage()

if change to isDeclarationForLinker here , it will hit a report_fatal_error.

DiggerLin updated this revision to Diff 258409.Apr 17 2020, 1:29 PM
DiggerLin marked 2 inline comments as done.

rebase the patch on the D78045 and address comment.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2142–2144

It seems my editing instruction got confused for the text. This is what I meant:

"Unhandled AvailableExternallyLinkage when mapping linkage to StorageClass"
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1619–1620

What happens for the case above? We need to generate MCSA_Extern for the reference. The definition should not appear in the output.

DiggerLin updated this revision to Diff 258432.EditedApr 17 2020, 2:54 PM

handle getSymbol returning a function descriptor symbol after rebase on the D78045

DiggerLin updated this revision to Diff 258442.Apr 17 2020, 3:51 PM

I think I address all the comments. thanks hubert.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1506

Add a comment that this gives us the function descriptor symbol.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2147

Replicate the

llvm_unreachable("Unknown linkage type!");

here before falling off the end of a function that does not return void.

DiggerLin updated this revision to Diff 258732.Apr 20 2020, 7:28 AM
DiggerLin marked 2 inline comments as done.
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
10

No need for two consecutive spaces here.

46

This has a prefix that is not checked by the RUN lines. Note also that it would only prevent the appearance of the subject line until the next positive match. Running FileCheck a second time on the same output may be necessary to check that it does not appear anywhere in an active manner. The passive method is to count on the block of COMMON-NEXT to exclude it.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
175

This should be relative to Index.

215

Same comment.

llvm/test/CodeGen/PowerPC/aix-extern.ll
10

Same comments for this test.

llvm/test/CodeGen/PowerPC/aix-weak.ll
10

Same comments for this test.

DiggerLin marked 2 inline comments as done.Apr 21 2020, 10:54 AM
DiggerLin added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2147

thanks

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
46

thanks for let me know

DiggerLin marked an inline comment as done.

address comment

LGTM with minor comment.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
9

Please fix the two consecutive spaces.

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 8:17 AM
DiggerLin updated this revision to Diff 261004.Apr 29 2020, 1:17 PM
DiggerLin marked an inline comment as done.

take out the functionality of "remove -u from clang"

DiggerLin edited the summary of this revision. (Show Details)Apr 29 2020, 1:17 PM

LGTM with minor comment.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2143

The beginning of report_fatal_error messages should not be capitalized in the style of a sentence.

This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
397–399

Note, there is a -Wimplicit-fallthrough issue in -DLLVM_ENABLE_ASSERTIONS=off builds which has been fixed by 31db4dbbbebd135c837014033a548eec15664ea8