Page MenuHomePhabricator

[AIX] Add '-bcdtors:all:0:s' to linker to gather static init functions
ClosedPublic

Authored by stevewan on May 21 2020, 3:33 PM.

Details

Summary

On AIX, add '-bcdtors:all:0:s' to the linker implicitly through the driver so that we can collect all static constructor and destructor functions.

Diff Detail

Event Timeline

stevewan created this revision.May 21 2020, 3:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2020, 3:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ZarkoCA added inline comments.May 22 2020, 8:04 AM
clang/test/Driver/aix-ld.cpp
1 ↗(On Diff #265631)

s/Arguemnt/Argument/

7 ↗(On Diff #265631)

Add a line between the run steps and check, and can you align things vertically.

Eg.

CHECK-LD32-ARG-ORDER-NOT: warning:
CHECK-LD32-ARG-ORDER:     {{.*}}clang++" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
ZarkoCA added inline comments.May 22 2020, 8:12 AM
clang/test/Driver/aix-ld.cpp
3 ↗(On Diff #265631)

formatting, align with the line above

hubert.reinterpretcast edited the summary of this revision. (Show Details)May 22 2020, 8:28 AM
clang/test/Driver/aix-ld.cpp
1 ↗(On Diff #265631)

Lowercase "argument".

2 ↗(On Diff #265631)

Let's not use a .o suffix for the link step output. It seems using no -o is fine for tests with -###.

stevewan updated this revision to Diff 265765.EditedMay 22 2020, 10:08 AM

Address comments and fix Windows compatibility issues with the test case.

stevewan marked 5 inline comments as done.May 22 2020, 10:10 AM
clang/test/Driver/aix-ld.cpp
9 ↗(On Diff #265765)

I am not sure I've seen this used in conjunction with %clangxx.
"{{.*}}clang{{.*}}" seems appropriate based on existing usage.

clang/test/Driver/aix-ld.cpp
18 ↗(On Diff #265765)

The main point of the test being that -bnocdtors from the user is not overriden by the implied -bcdtors:all:0:s, I think we should drop these last two lines and add a CHECK-LD32-ARG-ORDER-NOT against -bcdtors:all:0:s.

stevewan updated this revision to Diff 265781.May 22 2020, 12:01 PM

Address furthur comments on the test case.

clang/test/Driver/aix-ld.cpp
2 ↗(On Diff #265781)

I am wondering if it makes sense to put this into aix-ld.c. It helps to demonstrate that the link step behaviour is not determined by the source file name.

For example, in hexagon-toolchain-elf.c:

182 // RUN: %clangxx -### -target hexagon-unknown-elf \
183 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
184 // RUN:   -mcpu=hexagonv60 \
185 // RUN:   %s 2>&1 \
186 // RUN:   | FileCheck -check-prefix=CHECK031 %s
stevewan marked 3 inline comments as done.May 22 2020, 2:31 PM
stevewan added inline comments.
clang/test/Driver/aix-ld.cpp
2 ↗(On Diff #265781)

Yes, it makes sense to show that, and that was my original design. I moved this to a separate file to avoid getting a warning because of the inherited CHECK-LD32-ARG-ORDER-NOT: warning:. I'm now thinking of two ways to incorporate this into aix-ld.c,

  1. Simply remove the warning check like what's done in the example you've showed.
  2. Change the line CHECK-LD32-ARG-ORDER-NOT: warning: into something like,
// CHECK-LD32-ARG-ORDER-NOT: warning:
// CHECK-LD32-ARG-ORDER:     warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
// CHECK-LD32-ARG-ORDER-NOT: warning:

Do we have a preference for one way over another?

clang/test/Driver/aix-ld.cpp
2 ↗(On Diff #265781)

I think not adding something extra is better than adding something extra that's not particularly relevant. The intended behaviour for what we're testing is the same even if the deprecated behaviour is finally actually removed (and maybe with the removal of or a change to the warning message).

stevewan updated this revision to Diff 265813.May 22 2020, 3:38 PM

Incorporate the test case into 'aix-ld.c'.

stevewan marked 2 inline comments as done.May 22 2020, 3:41 PM
stevewan added a comment.EditedMay 22 2020, 4:01 PM

I'm planning to post an NFC patch after this to fix the formatting for existing cases in 'aix-ld.c' so that everything is consistent.

Thanks. LGTM with minor nit.

I'm planning to post an NFC patch after this to fix the formatting for existing cases in 'aix-ld.c' so that everything is consistent.

I'm not sure how NFC it is, but I would suggest removing the -o %t.o from the other cases as well.

clang/test/Driver/aix-ld.c
184

Minor nit: Maybe the prefix should be CHECK-LD32-CXX-ARG-ORDER.

This revision is now accepted and ready to land.May 22 2020, 4:15 PM
stevewan updated this revision to Diff 265818.May 22 2020, 4:55 PM

Update check prefix based on comments.

stevewan marked an inline comment as done.May 22 2020, 4:55 PM
This revision was automatically updated to reflect the committed changes.