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

[Steven Wan via Phabricator via cfe-commits]

stevewan 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");
----------------
jasonliu wrote:
> Xiangling_L wrote:
> > Is there any reason to use llvm_unreachable here? I think we should use  'assertion' instead here:
> > 
> > ```
> > assert((IsArch32Bit || IsArch64Bit) && "...");
> > ```
> IsArch64Bit used only in the assertion could cause warning when the assertion is turned off. 
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!


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


================
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
----------------
hubert.reinterpretcast wrote:
> For 32-bit mode, there is a "reentrant" variant for when `-pthread` or `-pthreads` is specified.
The `crt0_r.o` has become a symlink to `crt0.o`, we don't need to add extra handling for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68340