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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:35
+  // Only support 32 and 64 bit
+  if (!IsArch32Bit && !IsArch64Bit)
+    llvm_unreachable("Unsupported bit width value");
----------------
Is there any reason to use llvm_unreachable here? I think we should use  'assertion' instead here:

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


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:54
+  } else {
+    assert(Output.isNothing() && "Invalid output.");
+  }
----------------
I am not sure, if we compile with assertion off, does this extra 'else' {} have any side effect?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:70
+
+  if (!Args.hasArg(options::OPT_nostdlib)) {
+    const char *crt0 = nullptr;
----------------
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'?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:93
+  if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
+    // Support POSIX threads if "-pthreads" or 
+    // "-pthread" is present
----------------
One line of comment can be <= 80 characters.


================
Comment at: clang/lib/Driver/ToolChains/AIX.h:33
+                    const char *LinkingOutput) const override;
+};
+} // end namespace aix
----------------
An extra blank line preferred below.


================
Comment at: clang/lib/Driver/ToolChains/AIX.h:43
+          const llvm::opt::ArgList &Args);
+  ~AIX() override;
+
----------------
Since we are not doing anything special in AIX toolchain destructor, seems like we don't need to override it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68340