This is an archive of the discontinued LLVM Phabricator instance.

[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

DiggerLin created this revision.Mar 27 2020, 8:47 AM
DiggerLin edited the summary of this revision. (Show Details)Mar 27 2020, 8:52 AM
DiggerLin edited the summary of this revision. (Show Details)Mar 27 2020, 8:53 AM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin updated this revision to Diff 253281.Mar 27 2020, 7:31 PM

clang reformat

DiggerLin updated this revision to Diff 253579.Mar 30 2020, 6:28 AM

delete -u from clang test case aix-as.c

DiggerLin marked an inline comment as done.Mar 30 2020, 6:29 AM
DiggerLin added inline comments.
llvm/include/llvm/MC/MCDirectives.h
47

@hubert.reinterpretcast , @jasonliu , do we need to create a NFC patch for the clang format problem of the above first ?

llvm/include/llvm/MC/MCDirectives.h
47

I think it would help; yes. Please drop one at your earliest convenience and then rebase this patch on top of it.

jasonliu edited the summary of this revision. (Show Details)Mar 30 2020, 1:23 PM
jasonliu added inline comments.Mar 31 2020, 10:05 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1512

This query asked if the target supports .extern. However, .extern is not the only directive that could get emitted here. We could also emit .weak.

1518

I'm surprised we do not enter here for foo_ext_weak(). The result of that is we need to do something in lowerConstant(), which I would want to avoid if possible.
Should we look into why isDeclarationForLinker() returns false for foo_ext_weak() here?
My guess is we returned false for !F->isMaterializable(). And I'm not sure if foo_ext_weak is materializable is the correct answer here.

jasonliu added inline comments.Mar 31 2020, 10:11 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1518

Sorry, ignore the above comment. I think we did entered here for emitLinkage for foo_ext_weak. But the hasContainingCsect query blocked the emission there.

jasonliu added inline comments.Mar 31 2020, 10:40 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1577

I hope we can find a better solution here. IMO, we don't even want to override this function in this patch.
GVSym should be the right one from what caller passed in. It's caller's responsibility to pass in the right GVSym.
When caller calls emitLinkage, we should emitLinkage. It's weird when emitLinkage is called, but none is emitted (when hasContainingCsect() returns false).

jasonliu added inline comments.Mar 31 2020, 10:58 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1603

If it's possible, I would like to not emitLinkage here. I don't think it's the right place. Could we emitLinkage for the symbols needed in AsmPrinter::doFinalization instead?

DiggerLin marked 3 inline comments as done.Mar 31 2020, 2:24 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1577

In the function

 bool AsmPrinter::doFinalization(Module &M) {
  // Emit remaining GOT equivalent globals.
  emitGlobalGOTEquivs();

  // Emit visibility info for declarations
  for (const Function &F : M) {
  ....
}

there is function name .llvm.stackprotector in the M. the function is declared . it go to emitLinkage();

we need
if (XCOFFSym->hasContainingCsect()) {

}

to exclude emit linkage for function .llvm.stackprotector

I think over it , do you have any good suggestion to distinguish .llvm.stackprotector from other extern declared function.

DiggerLin updated this revision to Diff 254252.Apr 1 2020, 10:45 AM
DiggerLin marked 3 inline comments as done.

address comment

This comment was removed by jasonliu.
jasonliu added inline comments.Apr 2 2020, 7:21 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1491

Comment should change to something similar to:
Emit linkage(XCOFF) and visibility info for declarations.

1530

What if we have

void foo();
void (*foo_ptr)() = foo;
int bar() {
  foo();
}

We would need both .extern .foo and .extern foo[DS].
Also, please have a similar case into the lit test.

llvm/lib/MC/MCXCOFFStreamer.cpp
38

Please remove llvm:: for MCSA_Extern and MCSA_Weak to make the style consistent.

48

Maybe we should just move Symbol->setExternal(true); outside of the switch, as it is set for every attribute that we are going to emit.

llvm/lib/MC/XCOFFObjectWriter.cpp
355–356

line 353 to 365 did not align properly.

356

We should continue here if the rest of the logic does not matter.

357

A new line after '}'.

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

Nit: Please remove #0, #1 from the test case.

56

A proper space alignment would make the expected result more readable.

jasonliu added inline comments.Apr 2 2020, 8:03 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
409

Could we verify if these Linkage should also always emit .weak?
We do have one test case that tests LinkOnceODRLinkage in llvm/test/CodeGen/PowerPC/aix-LinkOnceODRLinkage.ll, but no other linkages here is tested and confirmed the behavior is right for AIX.
Also LinkOnceODRLinkage.ll test needs to get update as well, since we will emit .weak _Z3fooIiEvT_[DS] now.

DiggerLin edited the summary of this revision. (Show Details)Apr 2 2020, 12:38 PM
DiggerLin edited the summary of this revision. (Show Details)Apr 2 2020, 12:53 PM
DiggerLin edited the summary of this revision. (Show Details)Apr 3 2020, 8:25 AM
DiggerLin updated this revision to Diff 254846.Apr 3 2020, 9:49 AM
DiggerLin marked 7 inline comments as done.
  1. address comment
  2. add two new test cases.
  3. split a bit test case into three small test case.
DiggerLin added inline comments.Apr 3 2020, 10:41 AM
llvm/lib/MC/MCXCOFFStreamer.cpp
48

I think there is Symbol->setExternal(false ) in other switch cases later .

llvm/lib/MC/XCOFFObjectWriter.cpp
356

thanks.

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

Thanks for splitting the test case. I think it helps a lot for reading and maintainability.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1510

This line is not used before XCOFF specialized code, we should move it down.

1515

If I understand correctly, we do not want to touch how we interact with visibility in this patch.
If that's the case, we don't want to continue here and in line 1512 since the early continue will change the code flow for visibility.
Suggestion:
if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
and remove continue in line 1512.

1518

nit: I don't think it's a good idea here to assign back to Name here, as Name will also get used in emitVisibility below and we want to keep it as it is. Let's create a new MCSymbol* here.

1520
  1. We need to rebase here, as it is called hasRepresentedCsectSet() instead of hasContainingCsect() now.
  2. I'm slightly worried here to rely on hasRepresentedCsectSet() to check if a linkage should be emitted. This is based on the assumption that we will not ever change our implementation for .bl foo to .bl foo[PR]. But in https://reviews.llvm.org/D77080#inline-706207, we discussed about this possibility. So this assumption might not be true in the future. However, I'm not sure if there is another way to check if this function have been called directly.

So if there is another way to check, we should pursue the alternative instead. If there is not, then we need to add an assert here, like assert(Name->getName().equals(cast<MCSymbolXCOFF>(Name)->getUnqualifiedName()) to make sure we don't get a qualname here.

Have a comment here and tell people what we are doing here.
For example,
// If there is a direct call to external function, then we need to emit linkage for its function entry point.

1523

Comment need to mention what we are trying to do here:
// If address is taken from an extern function, we need to emit linkage for its function descriptor symbol.

1524

nit: for -> For,
and we don't need space between as and :

1530

nit: We don't need to assign it back to Name.
emitLinkage(&F, Csect->getQualNameSymbol());

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

Do we also want to test WeakAnyLinkage?

DiggerLin marked 16 inline comments as done.Apr 8 2020, 10:31 AM
DiggerLin added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1520

when we implement .bl foo to .bl foo[PR]
the SymbolName will change from .bl[SMC] and check the .bl[SMC]->hasRepresentedCsectSet()

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

WeakAnyLinkage should be weak
It has been tested in aix-weak.ll

define weak void @foo_weak(i32* %p) {
entry:

%p.addr = alloca i32*, align 4
store i32* %p, i32** %p.addr, align 4
%0 = load i32*, i32** %p.addr, align 4
%1 = load i32, i32* %0, align 4
%inc = add nsw i32 %1, 1
store i32 %inc, i32* %0, align 4
ret void

}

DiggerLin updated this revision to Diff 256067.Apr 8 2020, 10:39 AM
DiggerLin marked 2 inline comments as done.

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
1520

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
403–405

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.

441

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
1520

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
1520

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
1502

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
441

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.

1496

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

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1641

This comment does not apply any more.

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

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

29

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

48

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
441

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

1496

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
445

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

LLVM_FALLTHROUGH;
1496

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.

1505

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

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2014

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

2021

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
1642–1643

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
1496

rebase the patch on the D78045

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1642–1643
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
2023

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
1642–1643

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
1496

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

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2025

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
2025

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
2024

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
403–405

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