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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
