[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Rainer Orth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
ro marked 3 inline comments as done.
Closed by commit rG710949482edb: [clang][Driver] Dont hardcode 
--as-needed/--no-as-needed on Illumos (authored by ro).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84412

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -630,6 +630,16 @@
   return false;
 }
 
+static const char *getAsNeededOption(const ToolChain , bool as_needed) {
+  // While the Solaris 11.2 ld added --as-needed/--no-as-needed as aliases
+  // for the native forms -z ignore/-z record, they are missing in Illumos,
+  // so always use the native form.
+  if (TC.getTriple().isOSSolaris())
+return as_needed ? "-zignore" : "-zrecord";
+  else
+return as_needed ? "--as-needed" : "--no-as-needed";
+}
+
 void tools::linkSanitizerRuntimeDeps(const ToolChain ,
  ArgStringList ) {
   // Fuchsia never needs these.  Any sanitizer runtimes with system
@@ -639,7 +649,7 @@
 
   // Force linking against the system libraries sanitizers depends on
   // (see PR15823 why this is necessary).
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   // There's no libpthread or librt on RTEMS & Android.
   if (TC.getTriple().getOS() != llvm::Triple::RTEMS &&
   !TC.getTriple().isAndroid()) {
@@ -836,7 +846,7 @@
 }
 
 void tools::linkXRayRuntimeDeps(const ToolChain , ArgStringList ) {
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   CmdArgs.push_back("-lpthread");
   if (!TC.getTriple().isOSOpenBSD())
 CmdArgs.push_back("-lrt");
@@ -1261,7 +1271,7 @@
   bool AsNeeded = LGT == LibGccType::UnspecifiedLibGcc &&
   !TC.getTriple().isAndroid() && !TC.getTriple().isOSCygMing();
   if (AsNeeded)
-CmdArgs.push_back("--as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, true));
 
   switch (UNW) {
   case ToolChain::UNW_None:
@@ -1289,7 +1299,7 @@
   }
 
   if (AsNeeded)
-CmdArgs.push_back("--no-as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, false));
 }
 
 static void AddLibgcc(const ToolChain , const Driver ,


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -630,6 +630,16 @@
   return false;
 }
 
+static const char *getAsNeededOption(const ToolChain , bool as_needed) {
+  // While the Solaris 11.2 ld added --as-needed/--no-as-needed as aliases
+  // for the native forms -z ignore/-z record, they are missing in Illumos,
+  // so always use the native form.
+  if (TC.getTriple().isOSSolaris())
+return as_needed ? "-zignore" : "-zrecord";
+  else
+return as_needed ? "--as-needed" : "--no-as-needed";
+}
+
 void tools::linkSanitizerRuntimeDeps(const ToolChain ,
  ArgStringList ) {
   // Fuchsia never needs these.  Any sanitizer runtimes with system
@@ -639,7 +649,7 @@
 
   // Force linking against the system libraries sanitizers depends on
   // (see PR15823 why this is necessary).
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   // There's no libpthread or librt on RTEMS & Android.
   if (TC.getTriple().getOS() != llvm::Triple::RTEMS &&
   !TC.getTriple().isAndroid()) {
@@ -836,7 +846,7 @@
 }
 
 void tools::linkXRayRuntimeDeps(const ToolChain , ArgStringList ) {
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   CmdArgs.push_back("-lpthread");
   if (!TC.getTriple().isOSOpenBSD())
 CmdArgs.push_back("-lrt");
@@ -1261,7 +1271,7 @@
   bool AsNeeded = LGT == LibGccType::UnspecifiedLibGcc &&
   !TC.getTriple().isAndroid() && !TC.getTriple().isOSCygMing();
   if (AsNeeded)
-CmdArgs.push_back("--as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, true));
 
   switch (UNW) {
   case ToolChain::UNW_None:
@@ -1289,7 +1299,7 @@
   }
 
   if (AsNeeded)
-CmdArgs.push_back("--no-as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, false));
 }
 
 static void AddLibgcc(const ToolChain , const Driver ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84412

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Rainer Orth via Phabricator via cfe-commits]

ro marked 3 inline comments as done.
ro added a comment.

In D84412#2193600 , @MaskRay wrote:

> #clang  does not have many people. You 
> might need to add people explicitly..

I always found finding reviewers sort of a black art: sometimes the people from 
the `CODE_OWNERS.TXT` files work, sometimes people that
recently reviewed changes to the same files are willing to do so again, while 
at others only repeated nagging works...




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:633
 
+static const char *getAsNeededOption(const ToolChain , bool ignore) {
+  // FIXME: Need to handle GNU ld on Solaris.

MaskRay wrote:
> `ignore` seems strange. How about `start`?
I'd used `ignore` based no the description of the option in the `ld` man page:
```
   -z ignore | record
   --as-needed | --no-as-needed

   Ignores, or records, shared object dependencies that are not refer-
   enced as part  of  the  link-edit.  These  options  are  positional
```
`start` doesn't tell me much either, so I've gone for `as_needed`.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:634
+static const char *getAsNeededOption(const ToolChain , bool ignore) {
+  // FIXME: Need to handle GNU ld on Solaris.
+  if (TC.getTriple().isOSSolaris())

MaskRay wrote:
> Can you improve the comment to mention that this is to work around illumnos 
> ld?
I hope it's better now.  I've removed the GNU `ld` reference.
You can see my current hack to make `clang` work with `gld` on Solaris in 
D85309.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:636
+  if (TC.getTriple().isOSSolaris())
+return ignore ? "-zignore" : "-zrecord";
+  else

MaskRay wrote:
> I'll assume that `-zignore` has semantics similar to --as-needed.
Right: the two are identical semantically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84412

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Rainer Orth via Phabricator via cfe-commits]

ro updated this revision to Diff 283232.
ro added a comment.

Incorporate review comments: clarify comment and arg name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84412

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -630,6 +630,16 @@
   return false;
 }
 
+static const char *getAsNeededOption(const ToolChain , bool as_needed) {
+  // While the Solaris 11.2 ld added --as-needed/--no-as-needed as aliases
+  // for the native forms -z ignore/-z record, they are missing in Illumos,
+  // so always use the native form.
+  if (TC.getTriple().isOSSolaris())
+return as_needed ? "-zignore" : "-zrecord";
+  else
+return as_needed ? "--as-needed" : "--no-as-needed";
+}
+
 void tools::linkSanitizerRuntimeDeps(const ToolChain ,
  ArgStringList ) {
   // Fuchsia never needs these.  Any sanitizer runtimes with system
@@ -639,7 +649,7 @@
 
   // Force linking against the system libraries sanitizers depends on
   // (see PR15823 why this is necessary).
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   // There's no libpthread or librt on RTEMS & Android.
   if (TC.getTriple().getOS() != llvm::Triple::RTEMS &&
   !TC.getTriple().isAndroid()) {
@@ -836,7 +846,7 @@
 }
 
 void tools::linkXRayRuntimeDeps(const ToolChain , ArgStringList ) {
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   CmdArgs.push_back("-lpthread");
   if (!TC.getTriple().isOSOpenBSD())
 CmdArgs.push_back("-lrt");
@@ -1261,7 +1271,7 @@
   bool AsNeeded = LGT == LibGccType::UnspecifiedLibGcc &&
   !TC.getTriple().isAndroid() && !TC.getTriple().isOSCygMing();
   if (AsNeeded)
-CmdArgs.push_back("--as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, true));
 
   switch (UNW) {
   case ToolChain::UNW_None:
@@ -1289,7 +1299,7 @@
   }
 
   if (AsNeeded)
-CmdArgs.push_back("--no-as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, false));
 }
 
 static void AddLibgcc(const ToolChain , const Driver ,


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -630,6 +630,16 @@
   return false;
 }
 
+static const char *getAsNeededOption(const ToolChain , bool as_needed) {
+  // While the Solaris 11.2 ld added --as-needed/--no-as-needed as aliases
+  // for the native forms -z ignore/-z record, they are missing in Illumos,
+  // so always use the native form.
+  if (TC.getTriple().isOSSolaris())
+return as_needed ? "-zignore" : "-zrecord";
+  else
+return as_needed ? "--as-needed" : "--no-as-needed";
+}
+
 void tools::linkSanitizerRuntimeDeps(const ToolChain ,
  ArgStringList ) {
   // Fuchsia never needs these.  Any sanitizer runtimes with system
@@ -639,7 +649,7 @@
 
   // Force linking against the system libraries sanitizers depends on
   // (see PR15823 why this is necessary).
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   // There's no libpthread or librt on RTEMS & Android.
   if (TC.getTriple().getOS() != llvm::Triple::RTEMS &&
   !TC.getTriple().isAndroid()) {
@@ -836,7 +846,7 @@
 }
 
 void tools::linkXRayRuntimeDeps(const ToolChain , ArgStringList ) {
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   CmdArgs.push_back("-lpthread");
   if (!TC.getTriple().isOSOpenBSD())
 CmdArgs.push_back("-lrt");
@@ -1261,7 +1271,7 @@
   bool AsNeeded = LGT == LibGccType::UnspecifiedLibGcc &&
   !TC.getTriple().isAndroid() && !TC.getTriple().isOSCygMing();
   if (AsNeeded)
-CmdArgs.push_back("--as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, true));
 
   switch (UNW) {
   case ToolChain::UNW_None:
@@ -1289,7 +1299,7 @@
   }
 
   if (AsNeeded)
-CmdArgs.push_back("--no-as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, false));
 }
 
 static void AddLibgcc(const ToolChain , const Driver ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

#clang  does not have many people. You 
might need to add people explicitly..




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:633
 
+static const char *getAsNeededOption(const ToolChain , bool ignore) {
+  // FIXME: Need to handle GNU ld on Solaris.

`ignore` seems strange. How about `start`?



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:634
+static const char *getAsNeededOption(const ToolChain , bool ignore) {
+  // FIXME: Need to handle GNU ld on Solaris.
+  if (TC.getTriple().isOSSolaris())

Can you improve the comment to mention that this is to work around illumnos ld?



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:636
+  if (TC.getTriple().isOSSolaris())
+return ignore ? "-zignore" : "-zrecord";
+  else

I'll assume that `-zignore` has semantics similar to --as-needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84412

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Rainer Orth via Phabricator via cfe-commits]

ro added a reviewer: MaskRay.
ro added a comment.

Ping?  It's been two weeks now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84412

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84412: [clang][Driver] Don't hardcode --as-needed/--no-as-needed on Illumos

[Rainer Orth via Phabricator via cfe-commits]

ro created this revision.
ro added a reviewer: clang.
ro added a project: clang.
Herald added subscribers: fedor.sergeev, jyknight.

`ninja check-all` currently fails on Illumos:

  [84/716] Generating default/Asan-i386-inline-Test
  FAILED: projects/compiler-rt/lib/asan/tests/default/Asan-i386-inline-Test
  cd /var/llvm/dist-amd64-release/projects/compiler-rt/lib/asan/tests && 
/var/llvm/dist-amd64-release/./bin/clang 
ASAN_INST_TEST_OBJECTS.gtest-all.cc.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_globals_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_interface_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_internal_interface_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_oob_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_mem_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_str_test.cpp.i386-inline.o 
ASAN_INST_TEST_OBJECTS.asan_test_main.cpp.i386-inline.o -o 
/var/llvm/dist-amd64-release/projects/compiler-rt/lib/asan/tests/default/./Asan-i386-inline-Test
 -g --driver-mode=g++ -fsanitize=address -m32
  ld: fatal: unrecognized option '--no-as-needed'
  ld: fatal: use the -z help option for usage information
  clang-11: error: linker command failed with exit code 1 (use -v to see 
invocation)

`clang` unconditionally passes `--as-needed`/`--no-as-needed` to the linker.  
This works
on Solaris 11.[34] which added a couple of option aliases to the native linker 
to improve
compatibility with GNU `ld`. Illumos `ld` didn't do this, so one needs to use 
the corresponding
native options `-z ignore`/`-z record` instead.

Because this works on both Solaris and Illumos, the current patch always passes 
the
native options on Solaris.  This isn't fully correct, however: when using GNU 
`ld` on 
Solaris (not yet supported; I'm working on that), one still needs `--as-needed` 
instead.

I'm hardcoding this decision because a generic detection via a `cmake` test is 
hard:
many systems have their own implementation of `getDefaultLinker` and `cmake` 
would
have to duplicate the information encoded there.  Besides, it would still break 
when
`-fuse-ld` is used.

Tested on `amd64-pc-solaris2.11` (Solaris 11.4 and OpenIndiana 2020.04), 
`sparcv9-sun-solaris2.11`, and
`x86_64-pc-linux-gnu`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84412

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -630,6 +630,14 @@
   return false;
 }
 
+static const char *getAsNeededOption(const ToolChain , bool ignore) {
+  // FIXME: Need to handle GNU ld on Solaris.
+  if (TC.getTriple().isOSSolaris())
+return ignore ? "-zignore" : "-zrecord";
+  else
+return ignore ? "--as-needed" : "--no-as-needed";
+}
+
 void tools::linkSanitizerRuntimeDeps(const ToolChain ,
  ArgStringList ) {
   // Fuchsia never needs these.  Any sanitizer runtimes with system
@@ -639,7 +647,7 @@
 
   // Force linking against the system libraries sanitizers depends on
   // (see PR15823 why this is necessary).
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   // There's no libpthread or librt on RTEMS & Android.
   if (TC.getTriple().getOS() != llvm::Triple::RTEMS &&
   !TC.getTriple().isAndroid()) {
@@ -833,7 +841,7 @@
 }
 
 void tools::linkXRayRuntimeDeps(const ToolChain , ArgStringList ) {
-  CmdArgs.push_back("--no-as-needed");
+  CmdArgs.push_back(getAsNeededOption(TC, false));
   CmdArgs.push_back("-lpthread");
   if (!TC.getTriple().isOSOpenBSD())
 CmdArgs.push_back("-lrt");
@@ -1258,7 +1266,7 @@
   bool AsNeeded = LGT == LibGccType::UnspecifiedLibGcc &&
   !TC.getTriple().isAndroid() && !TC.getTriple().isOSCygMing();
   if (AsNeeded)
-CmdArgs.push_back("--as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, true));
 
   switch (UNW) {
   case ToolChain::UNW_None:
@@ -1286,7 +1294,7 @@
   }
 
   if (AsNeeded)
-CmdArgs.push_back("--no-as-needed");
+CmdArgs.push_back(getAsNeededOption(TC, false));
 }
 
 static void AddLibgcc(const ToolChain , const Driver ,


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -630,6 +630,14 @@
   return false;
 }
 
+static const char *getAsNeededOption(const ToolChain , bool ignore) {
+  // FIXME: Need to handle GNU ld on Solaris.
+  if (TC.getTriple().isOSSolaris())
+return ignore ? "-zignore" : "-zrecord";
+  else
+return ignore ? "--as-needed" : "--no-as-needed";
+}
+
 void tools::linkSanitizerRuntimeDeps(const ToolChain ,
  ArgStringList ) {
   // Fuchsia never needs these.  Any sanitizer