This is an archive of the discontinued LLVM Phabricator instance.

Add AIX toolchain and basic linker functionality
ClosedPublic

Authored by stevewan on Oct 2 2019, 7:57 AM.

Details

Summary

This patch adds AIX toolchain infrastructure into driver, and enables AIX
system linker invocation with some basic functionality support

Diff Detail

Event Timeline

stevewan created this revision.Oct 2 2019, 7:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 7:57 AM

Any reason we are testing library search path using libgcc.a? We could use any dummy path there, and upload a dummy archive library for the testing purpose.

clang/lib/Driver/ToolChains/AIX.cpp
113

We are not doing anything with the destructor. Do we need to declare and define it?

clang/lib/Driver/ToolChains/AIX.h
2

The length of this line should match with line 7.

nit: We might want to add period consistently for all the comments in the patch.

Xiangling_L added inline comments.
clang/lib/Driver/ToolChains/AIX.cpp
36

Is there any reason to use llvm_unreachable here? I think we should use 'assertion' instead here:

assert((IsArch32Bit || IsArch64Bit) && "...");
55

I am not sure, if we compile with assertion off, does this extra 'else' {} have any side effect?

71

line 38 and line 70 use the same query, should they be put together? Or is there any exact order we should follow to push args into 'CmdArgs'?

94

One line of comment can be <= 80 characters.

clang/lib/Driver/ToolChains/AIX.h
34

An extra blank line preferred below.

44

Since we are not doing anything special in AIX toolchain destructor, seems like we don't need to override it?

Xiangling_L added inline comments.Oct 8 2019, 12:50 PM
clang/lib/Driver/ToolChains/AIX.cpp
39

Test with Clangtana on terran, when no '-nostdlib' specified, since '-e' & 'start' are the default behavior for AIX system linker, so there are no explicitly '-e' & 'start' found on linker input commanline, so I am wondering do we need to explicitly add them to 'CmdArgs'?

48

Ditto. Since by default, AIX linker is dynamically linked, '-bso' is implicitly set on AIX system linker when testing with Clangtana, so do we need to explicitly set '-bso' in LLVM?

jasonliu added inline comments.Oct 8 2019, 12:51 PM
clang/lib/Driver/ToolChains/AIX.cpp
36

IsArch64Bit used only in the assertion could cause warning when the assertion is turned off.

clang/lib/Driver/ToolChains/AIX.cpp
2

See Jason's comment about the line length.

22

The name lookup rules should be sufficient to avoid needing so many using directives.

Try:

namespace aix = clang::driver::tools::aix;
using AIX = clang::driver::toolchains::AIX;

using namespace llvm::opt;
25

See the comment regarding clang-format.

35

Periods at the end of sentences in comments please. Please update the comments in this patch.

37

Add a period to the end of the sentence.

39

Agreed. It does not seem that the compiler (GCC or XL) passes -e __start explicitly. This block should be removed.

58

Add a comma after "i.e.".

75

For 32-bit mode, there is a "reentrant" variant for when -pthread or -pthreads is specified.

82

This is always true. Maybe use a getCrt0Basename lambda?

106

See comment regarding a TODO.

108

See comment regarding clang-format.

115

Use a trailing return type to make use of name lookup rules to find Tool:

auto AIX::buildLinker() const -> Tool * {
clang/lib/Driver/ToolChains/AIX.h
20

This patch does not cover the assembler as the comment claims. Please add a TODO.

36

I would suggest adding a blank line before closing tools and also closing the driver and clang namespaces here. Reopen the latter two in a block with the opening of toolchains below.

43

I believe clang-format would indent this so that the first character comes right after the position of the left parenthesis that opened the parameter list.

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

Add "are" before "sane".

4

There seem to be no tests about nostdlib and nodefaultlibs.

15

If removing explicit -bso, then perhaps check that there isn't -bnso.

58

No need for the line to be so long.

stevewan updated this revision to Diff 225521.Oct 17 2019, 2:27 PM

Address comments

jasonliu added inline comments.Oct 18 2019, 7:25 AM
clang/lib/Driver/ToolChains/AIX.h
20

remove "and".

37

Sorry, I don't think we need '.' for any of the "end namespace ..." comment.

64

We don't need '.' here as well.

stevewan updated this revision to Diff 225631.Oct 18 2019, 8:06 AM
stevewan marked 25 inline comments as done.
stevewan added reviewers: jasonliu, Xiangling_L.

Fix periods at the end of comments

stevewan marked 3 inline comments as done.Oct 18 2019, 8:10 AM
stevewan updated this revision to Diff 225925.Oct 21 2019, 11:06 AM
stevewan marked 8 inline comments as done.

Tidy code with lambda expression

clang/lib/Driver/ToolChains/AIX.cpp
36

Jason has provided a good point why llvm_unreachable was preferred here. Other than that, I believe the two are fairly interchangeable in this particular case. That said, I'm leaning towards keeping llvm_unreachable, but definitely add more comment if you have good reasons for using assert. Thanks!

55

As per my tests with assertion-off build, I found no side effect and/or unexpected behaviour caused by this. There was no warning or anything unexpected that would've not appeared when assertion is on.

75

The crt0_r.o has become a symlink to crt0.o, we don't need to add extra handling for it.

Xiangling_L added inline comments.Oct 22 2019, 11:14 AM
clang/lib/Driver/ToolChains/AIX.cpp
36

Jason is right, I am fine with keeping llvm_unreachable.

45

Glad to know the build without assertion on would not be affected by this. I just have slight preference that we don't have this blank block in our product code when the assertion is off. Is that better we put this assertion before if block, and do something like this;

assert((Output.isFilename() || Output.isNothing()) && "Invalid output.");

if (Output.isFilename()) {
    CmdArgs.push_back("-o");
    CmdArgs.push_back(Output.getFilename());
  }
jasonliu added inline comments.Oct 23 2019, 10:51 AM
clang/lib/Driver/CMakeLists.txt
33

Looks like this list is following alphabetical order here, which means we should probably put "ToolChains/AIX.cpp" right after "ToolChain.cpp".

stevewan marked 3 inline comments as done.Oct 23 2019, 11:43 AM
stevewan added inline comments.
clang/lib/Driver/CMakeLists.txt
33

I had the same doubt when I added it. There is definitely an alphabetical order, yet I found the distinction between Arch and OS took precedence. As we could see that the "n" in "Ananas" and "M" in "AMDGPU" would have both came before the "r" in "Arch" if they only followed alphabetical order. That said, we could still consider moving all three but that might be an unnecessary hassle, and having OS followed by Arch and back to OS seems a bit counter-intuitive after all.

stevewan marked an inline comment as done.Oct 23 2019, 11:43 AM
stevewan updated this revision to Diff 226170.Oct 23 2019, 12:35 PM

Avoid blank else block when assertion is off

stevewan marked an inline comment as done.Oct 23 2019, 12:37 PM
jasonliu accepted this revision.EditedOct 24 2019, 8:01 AM

Aside from the nit comment that can be addressed when commit.
LGTM.

clang/lib/Driver/ToolChains/AIX.cpp
60

nit: There is no need to capture IsArch32Bit by reference.

This revision is now accepted and ready to land.Oct 24 2019, 8:01 AM
stevewan updated this revision to Diff 226304.Oct 24 2019, 11:19 AM

Capture local variable IsArch32Bit by value.

stevewan marked 2 inline comments as done.Oct 24 2019, 11:29 AM
stevewan added inline comments.
clang/lib/Driver/ToolChains/AIX.cpp
60

Agreed. It's safer to capture IsArch32Bit by value here.

stevewan marked an inline comment as done.Oct 24 2019, 11:30 AM
This revision was automatically updated to reflect the committed changes.

@daltenty's patch rG201ed14aea8c should fix the problem. In the latest build http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/6813, AIX.cpp.o has been successfully generated. Thanks for letting us know.