[PATCH] D35337: [Solaris] gcc runtime dropped support for .ctors, switch to .init_array

[David L. Jones via Phabricator via cfe-commits]

dlj accepted this revision.
dlj added a comment.

I think this change is structurally fine, but I'll defer to others on whether it's actually doing the right thing. (I'm pretty sure it is, but I'm far from an expert on Solaris or Sparc.)



================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2467
   const Generic_GCC::GCCVersion &V = GCCInstallation.getVersion();
   bool UseInitArrayDefault =
       getTriple().getArch() == llvm::Triple::aarch64 ||
----------------
fedor.sergeev wrote:
> davide wrote:
> > this is really a mess.
> Any suggestions on how to improve? :)
>From my perspective, this is really no worse than what's already here. I tried to avoid changing logic in https://reviews.llvm.org/rL297250, but this function is an example of something that still can use a lot of work.

Since this variable depends on all the parts of the triple, it probably won't break down very cleanly among the various subclasses. That said, if you do want to refactor this, you'll of course want to spend some time to think through exactly how this should work, and spend some time gathering feedback from other Clang devs... so it's probably out of the scope of this patch. I would be happy to help review that change... but, more pragmatically, I'd say land this change first before starting any surgery.


================
Comment at: test/Driver/constructors.c:83
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1        \
+// RUN:     -target i386-pc-solaris2.11 \
+// RUN:   | FileCheck --check-prefix=CHECK-INIT-ARRAY %s
----------------
Thank you for adding these. :-D Most of the toolchain classes lack explanatory comments about which platform combinations are expected to work for different features, so test cases like this will be invaluable to anyone who wants to refactor parts of the driver.


https://reviews.llvm.org/D35337