[PATCH] D68340: Add AIX toolchain and basic linker functionality

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:1
+//===--- AIX.cpp - AIX ToolChain Implementations --------*- C++ -*-===//
+//
----------------
See Jason's comment about the line length.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:21
+using namespace clang;
+using namespace llvm::opt;
+
----------------
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;
```



================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:24
+void aix::Linker::ConstructJob(Compilation &C, const JobAction &JA,
+                                   const InputInfo &Output,
+                                   const InputInfoList &Inputs,
----------------
See the comment regarding `clang-format`.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:34
+  const bool IsArch64Bit = ToolChain.getTriple().isArch64Bit();
+  // Only support 32 and 64 bit
+  if (!IsArch32Bit && !IsArch64Bit)
----------------
Periods at the end of sentences in comments please. Please update the comments in this patch.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:36
+  if (!IsArch32Bit && !IsArch64Bit)
+    llvm_unreachable("Unsupported bit width value");
+
----------------
Add a period to the end of the sentence.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:38
+
+  if (!Args.hasArg(options::OPT_nostdlib)) {
+    CmdArgs.push_back("-e");
----------------
Xiangling_L wrote:
> 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'?
Agreed. It does not seem that the compiler (GCC or XL) passes `-e __start` explicitly. This block should be removed.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:57
+
+  // Set linking mode (i.e. 32/64-bit) and the address of
+  // text and data sections based on arch bit width
----------------
Add a comma after "i.e.".


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:74
+    if (Args.hasArg(options::OPT_pg))
+      crt0 = IsArch32Bit ? "gcrt0.o" : "gcrt0_64.o";
+    // Enable profiling when "-p" is specified
----------------
For 32-bit mode, there is a "reentrant" variant for when `-pthread` or `-pthreads` is specified.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:81
+
+    if (crt0)
+      CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crt0)));
----------------
This is always true. Maybe use a `getCrt0Basename` lambda?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:105
+
+/// AIX - AIX tool chain which can call as(1) and ld(1) directly.
+AIX::AIX(const Driver &D, const llvm::Triple &Triple,
----------------
See comment regarding a TODO.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:107
+AIX::AIX(const Driver &D, const llvm::Triple &Triple,
+                 const ArgList &Args)
+    : ToolChain(D, Triple, Args) {
----------------
See comment regarding `clang-format`.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:114
+
+Tool *AIX::buildLinker() const { 
+  return new tools::aix::Linker(*this); 
----------------
Use a trailing return type to make use of name lookup rules to find `Tool`:
```
auto AIX::buildLinker() const -> Tool * {
```


================
Comment at: clang/lib/Driver/ToolChains/AIX.h:19
+
+/// aix -- Directly call system default assembler and linker
+namespace aix {
----------------
This patch does not cover the assembler as the comment claims. Please add a TODO.


================
Comment at: clang/lib/Driver/ToolChains/AIX.h:35
+} // end namespace aix
+} // end namespace tools
+
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/AIX.h:42
+  AIX(const Driver &D, const llvm::Triple &Triple,
+          const llvm::opt::ArgList &Args);
+  ~AIX() override;
----------------
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.


================
Comment at: clang/test/Driver/aix-ld.c:1
+// General tests that ld invocations on AIX targets sane. Note that we use
+// sysroot to make these tests independent of the host system.
----------------
Add "are" before "sane".


================
Comment at: clang/test/Driver/aix-ld.c:3
+// sysroot to make these tests independent of the host system.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
----------------
There seem to be no tests about `nostdlib` and `nodefaultlibs`.


================
Comment at: clang/test/Driver/aix-ld.c:14
+// CHECK-LD32: "-e" "__start" 
+// CHECK-LD32: "-bso"
+// CHECK-LD32: "-b32" 
----------------
If removing explicit `-bso`, then perhaps check that there isn't `-bnso`.


================
Comment at: clang/test/Driver/aix-ld.c:57
+
+// Check powerpc-ibm-aix7.1.0.0, 64-bit. Enable POSIX thread support with aliased pthreads option
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
----------------
No need for the line to be so long.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68340/new/

https://reviews.llvm.org/D68340