[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359603: [Driver] Support compiler-rt crtbegin.o/crtend.o for 
Linux (authored by phosek, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59264?vs=193733&id=197414#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59264

Files:
  cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
  cfe/trunk/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtbegin-i386.o
  cfe/trunk/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtbegin-x86_64.o
  cfe/trunk/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtend-i386.o
  cfe/trunk/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtend-x86_64.o
  cfe/trunk/test/Driver/linux-ld.c

Index: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
@@ -21,6 +21,7 @@
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
+#include "clang/Driver/ToolChain.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Path.h"
@@ -453,20 +454,29 @@
 
 if (IsIAMCU)
   CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crt0.o")));
-else {
-  const char *crtbegin;
-  if (Args.hasArg(options::OPT_static))
-crtbegin = isAndroid ? "crtbegin_static.o" : "crtbeginT.o";
-  else if (Args.hasArg(options::OPT_shared))
-crtbegin = isAndroid ? "crtbegin_so.o" : "crtbeginS.o";
-  else if (IsPIE || IsStaticPIE)
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbeginS.o";
-  else
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbegin.o";
-
-  if (HasCRTBeginEndFiles)
-CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crtbegin)));
-}
+else if (HasCRTBeginEndFiles) {
+  std::string P;
+  if (ToolChain.GetRuntimeLibType(Args) == ToolChain::RLT_CompilerRT &&
+  !isAndroid) {
+std::string crtbegin = ToolChain.getCompilerRT(Args, "crtbegin",
+   ToolChain::FT_Object);
+if (ToolChain.getVFS().exists(crtbegin))
+  P = crtbegin;
+  }
+  if (P.empty()) {
+const char *crtbegin;
+if (Args.hasArg(options::OPT_static))
+  crtbegin = isAndroid ? "crtbegin_static.o" : "crtbeginT.o";
+else if (Args.hasArg(options::OPT_shared))
+  crtbegin = isAndroid ? "crtbegin_so.o" : "crtbeginS.o";
+else if (IsPIE || IsStaticPIE)
+  crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbeginS.o";
+else
+  crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbegin.o";
+P = ToolChain.GetFilePath(crtbegin);
+  }
+  CmdArgs.push_back(Args.MakeArgString(P));
+	  }
 
 // Add crtfastmath.o if available and fast math is enabled.
 ToolChain.AddFastMathRuntimeIfAvailable(Args, CmdArgs);
@@ -560,16 +570,27 @@
 }
 
 if (!Args.hasArg(options::OPT_nostartfiles) && !IsIAMCU) {
-  const char *crtend;
-  if (Args.hasArg(options::OPT_shared))
-crtend = isAndroid ? "crtend_so.o" : "crtendS.o";
-  else if (IsPIE || IsStaticPIE)
-crtend = isAndroid ? "crtend_android.o" : "crtendS.o";
-  else
-crtend = isAndroid ? "crtend_android.o" : "crtend.o";
-
-  if (HasCRTBeginEndFiles)
-CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crtend)));
+  if (HasCRTBeginEndFiles) {
+std::string P;
+if (ToolChain.GetRuntimeLibType(Args) == ToolChain::RLT_CompilerRT &&
+!isAndroid) {
+  std::string crtend = ToolChain.getCompilerRT(Args, "crtend",
+   ToolChain::FT_Object);
+  if (ToolChain.getVFS().exists(crtend))
+P = crtend;
+}
+if (P.empty()) {
+  const char *crtend;
+  if (Args.hasArg(options::OPT_shared))
+crtend = isAndroid ? "crtend_so.o" : "crtendS.o";
+  else if (IsPIE || IsStaticPIE)
+crtend = isAndroid ? "crtend_android.o" : "crtendS.o";
+  else
+crtend = isAndroid ? "crtend_android.o" : "crtend.o";
+  P = ToolChain.GetFilePath(crtend);
+}
+CmdArgs.push_back(Args.MakeArgString(P));
+  }
   if (!isAndroid)
 CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtn.o")));
 }
Index: cfe/trunk/test/Driver/linux-ld.c
===
--- cfe/trunk/test/Driver/linux-ld.c
+++ cfe/trunk/test/Driver/linux-ld.c
@@ -2,7 +2,7 @@
 // sysroot to make these tests independent of the host system.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Fangrui Song via Phabricator via cfe-commits]

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

Thank you for unifying `crtbegin{,S,T}.o` 😊 I hope Android people can make 
their `crtbegin*.o` files simpler, one day..


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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

@MaskRay can you review this again now that there's just a single `crtbegin.o` 
and `crtend.o` version?


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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek updated this revision to Diff 193733.

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

https://reviews.llvm.org/D59264

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtbegin-i386.o
  clang/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtbegin-x86_64.o
  clang/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtend-i386.o
  clang/test/Driver/Inputs/resource_dir/lib/linux/clang_rt.crtend-x86_64.o
  clang/test/Driver/linux-ld.c

Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -2,7 +2,7 @@
 // sysroot to make these tests independent of the host system.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux \
+// RUN: --target=i386-unknown-linux -rtlib=platform \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-32 %s
@@ -51,16 +51,18 @@
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN: --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT %s
 // CHECK-LD-RT-NOT: warning:
+// CHECK-LD-RT: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-LD-RT: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT: "--eh-frame-hdr"
 // CHECK-LD-RT: "-m" "elf_x86_64"
 // CHECK-LD-RT: "-dynamic-linker"
-// CHECK-LD-RT: "{{.*}}/usr/lib/gcc/x86_64-unknown-linux/4.6.0{{/|}}crtbegin.o"
+// CHECK-LD-RT: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtbegin-x86_64.o"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../../../x86_64-unknown-linux/lib"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../.."
@@ -69,19 +71,22 @@
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
 // CHECK-LD-RT: "-lc"
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtend-x86_64.o"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=i686-unknown-linux \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN: --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT-I686 %s
 // CHECK-LD-RT-I686-NOT: warning:
+// CHECK-LD-RT-I686: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-LD-RT-I686: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT-I686: "--eh-frame-hdr"
 // CHECK-LD-RT-I686: "-m" "elf_i386"
 // CHECK-LD-RT-I686: "-dynamic-linker"
-// CHECK-LD-RT-I686: "{{.*}}/usr/lib/gcc/i686-unknown-linux/4.6.0{{/|}}crtbegin.o"
+// CHECK-LD-RT-I686: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtbegin-i386.o"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../../../i686-unknown-linux/lib"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../.."
@@ -90,6 +95,7 @@
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
 // CHECK-LD-RT-I686: "-lc"
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtend-i386.o"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=arm-linux-androideabi \
@@ -110,7 +116,6 @@
 // RUN: --target=x86_64-unknown-linux -rtlib=platform \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
-// RUN: --rtlib=libgcc \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-GCC %s
 // CHECK-LD-GCC-NOT: warning:
 // CHECK-LD-GCC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
@@ -291,7 +296,7 @@
 // RUN:   | FileCheck --check-prefix=CHECK-LD-64-STATIC %s
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux -m32 \
+// RUN: --target=i386-unknown-linux -rtlib=platform -m32 \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/multilib_32bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-32-TO-32 %s
@@ -308,7 +313,7 @@
 // CHECK-32-TO-32: "-L[[SYSROOT]]/usr/lib"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux -m64 \
+// RUN: --target=i386-unknown-linux -rtlib=platform -m64 \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/multilib_32bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-32-TO-64 %s
@@ -326,7 +331,7 @@
 // CHECK-32-TO-64: "-L[[SYSRO

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D59264#1428785 , @MaskRay wrote:

> > A nonzero __dso_handle has to match the value passed to __cxa_atexit but a 
> > zero __dso_handle matches every function registered. So it matters that DSO 
> > fini calls use &__dso_handle to match their registrations for the dlclose 
> > case
>
> Yes, but this won't matter if we change `void *__dso_handle = 0;` to `void 
> *__dso_handle = &__dso_handle;`.
>
>   static void __do_global_dtors_aux() { // static void __do_fini() in D28791
> if (__cxa_finalize)
>   __cxa_finalize(__dso_handle); // what happens if we change `void 
> *__dso_handle = 0;` to `void *__dso_handle = &__dso_handle;`.
> ...
>   }
>
>
> exit calls `__run_exit_handlers`, which goes through `__exit_funcs` and calls 
> the hooks one by one. `_dl_fini` is the last in the list because 
> `__cxa_atexit(_dl_fini,0,__dso_handle)` runs before other atexit registered 
> functions.[1]
>
> `__do_global_dtors_aux` is called by `_dl_fini`. When it gets called and it 
> calls `__cxa_finalize(0)`, other atexit registered functions have run, thus 
> __cxa_finalize(0) will do nothing.
>
> The differentiation of `crtbegin.o crtbeginS.o` is unnecessary. It adds 
> complexity for little size benefit (crtbegin.o is a bit smaller than 
> crtbeginS.o).
>  While we are adding support for crtbegin.o, it'd be really good to get this 
> fixed.
>
> [1]: If a shared object has a constructor that calls `__cxa_atexit`, 
> `__cxa_atexit(_dl_fini,...)` will not be the first. [2]
>
> [2]: If you try really hard you can break that in glibc (but not in FreeBSD 
> libc), but I'll call that an unsupported land as functions in the main 
> executable (`register_in_exe`) shouldn't be called before it is initialized: 
> `__attribute__((constructor)) void init_in_dso() { register_in_exe(); 
> atexit(fini_in_dso); }`
>  Moreover, if you have several DSOs, the global reverse order of 
> `atexit`-registered functions won't be guaranteed. 
> `__cxa_finalize(__dso_handle in exe)` `__cxa_finalize(__dso_handle in b.so)` 
> `__cxa_finalize(__dso_handle c.so)`


I sent an email to glibc maintainers to clarify their position on this: 
https://sourceware.org/ml/libc-alpha/2019-04/msg00028.html. If they're fine 
with this change, I'll update the implementation as you suggested.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> A nonzero __dso_handle has to match the value passed to __cxa_atexit but a 
> zero __dso_handle matches every function registered. So it matters that DSO 
> fini calls use &__dso_handle to match their registrations for the dlclose case

Yes, but this won't matter if we change `void *__dso_handle = 0;` to `void 
*__dso_handle = &__dso_handle;`.

  static void __do_global_dtors_aux() { // static void __do_fini() in D28791
if (__cxa_finalize)
  __cxa_finalize(__dso_handle); // what happens if we change `void 
*__dso_handle = 0;` to `void *__dso_handle = &__dso_handle;`.
...
  }

exit calls `__run_exit_handlers` which goes through `__exit_funcs`.
`__cxa_atexit(_dl_fini,0,__dso_handle)` runs before other atexit registered 
functions after main() is executed.[1]

`__do_global_dtors_aux` is called by `_dl_fini`. When it gets called and it 
calls `__cxa_finalize(0)`, other atexit registered functions have run, thus 
__cxa_finalize(0) will do nothing.

The differentiation of `crtbegin.o crtbeginS.o` is unnecessary. It adds 
complexity for little size benefit (crtbegin.o is a bit smaller than 
crtbeginS.o).
While we are adding support for crtbegin.o, it'd be really good to get this 
fixed.

[1]: If a shared object has a constructor that calls `__cxa_atexit`, 
`__cxa_atexit(_dl_fini,...)` will not be the first.

  If you try really hard you can break that in glibc (but not in FreeBSD libc), 
but I'll call that an unsupported land as functions in the main executable 
(`register_in_exe`) shouldn't be called before it is initialized.
  `__attribute__((constructor)) void init_in_dso() { register_in_exe(); 
atexit(fini_in_dso); }`


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:447
   const char *crtbegin;
-  if (Args.hasArg(options::OPT_static))
-crtbegin = isAndroid ? "crtbegin_static.o" : "crtbeginT.o";
-  else if (Args.hasArg(options::OPT_shared))
-crtbegin = isAndroid ? "crtbegin_so.o" : "crtbeginS.o";
-  else if (IsPIE || IsStaticPIE)
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbeginS.o";
-  else
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbegin.o";
-
-  if (HasCRTBeginEndFiles)
+  if (ToolChain.GetRuntimeLibType(Args) == ToolChain::RLT_CompilerRT &&
+  !isAndroid) {

phosek wrote:
> manojgupta wrote:
> > This is currently unconditional on using compiler-rt. Given that 
> > compiler-rt provides the crt*.o files only under the CMake flag 
> > COMPILER_RT_BUILD_CRT, shouldn't there be an equivalent clang CMake flag to 
> > conditionally enable this.
> `COMPILER_RT_BUILD_CRT` is `ON` by default now in D28791, but even if we 
> change that, I think this logic should be independent since you should be 
> able to build Clang separately from compiler-rt.
Since it is possible to build compiler-rt without CRT files, I believe that 
clang should also be able to use compiler-rt without compiler-rt provided CRT 
files.
Using them unconditionally will also break the case of using newer clang with 
older compiler-rt.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek marked an inline comment as done.
phosek added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:447
   const char *crtbegin;
-  if (Args.hasArg(options::OPT_static))
-crtbegin = isAndroid ? "crtbegin_static.o" : "crtbeginT.o";
-  else if (Args.hasArg(options::OPT_shared))
-crtbegin = isAndroid ? "crtbegin_so.o" : "crtbeginS.o";
-  else if (IsPIE || IsStaticPIE)
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbeginS.o";
-  else
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbegin.o";
-
-  if (HasCRTBeginEndFiles)
+  if (ToolChain.GetRuntimeLibType(Args) == ToolChain::RLT_CompilerRT &&
+  !isAndroid) {

manojgupta wrote:
> This is currently unconditional on using compiler-rt. Given that compiler-rt 
> provides the crt*.o files only under the CMake flag COMPILER_RT_BUILD_CRT, 
> shouldn't there be an equivalent clang CMake flag to conditionally enable 
> this.
`COMPILER_RT_BUILD_CRT` is `ON` by default now in D28791, but even if we change 
that, I think this logic should be independent since you should be able to 
build Clang separately from compiler-rt.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added subscribers: manojgupta, llozano.
manojgupta added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:447
   const char *crtbegin;
-  if (Args.hasArg(options::OPT_static))
-crtbegin = isAndroid ? "crtbegin_static.o" : "crtbeginT.o";
-  else if (Args.hasArg(options::OPT_shared))
-crtbegin = isAndroid ? "crtbegin_so.o" : "crtbeginS.o";
-  else if (IsPIE || IsStaticPIE)
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbeginS.o";
-  else
-crtbegin = isAndroid ? "crtbegin_dynamic.o" : "crtbegin.o";
-
-  if (HasCRTBeginEndFiles)
+  if (ToolChain.GetRuntimeLibType(Args) == ToolChain::RLT_CompilerRT &&
+  !isAndroid) {

This is currently unconditional on using compiler-rt. Given that compiler-rt 
provides the crt*.o files only under the CMake flag COMPILER_RT_BUILD_CRT, 
shouldn't there be an equivalent clang CMake flag to conditionally enable this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Ethan Sommer via Phabricator via cfe-commits]

E5ten added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:563
+  crtend = Args.hasArg(options::OPT_shared) || IsPIE || IsStaticPIE ?
+  "crtend_shared" : "crtend";
+  CmdArgs.push_back(ToolChain.getCompilerRTArgString(

phosek wrote:
> MaskRay wrote:
> > phosek wrote:
> > > MaskRay wrote:
> > > > I believe `crtbegin.o` `crtend.o` should just work. It is not necessary 
> > > > to use `crtbegin_shared.o` `crtend_shared.o`.
> > > This is related to your comments on D28791, specifically that we should 
> > > be using `crtbegin_shared.o` for `-shared` or `-pie` and `crtbegin.o` 
> > > otherwise, is that not the case?
> > Yes. I think we can rename `crtbegin_shared.o` to `crtbegin.o` and use it 
> > for every configuration: `-no-pie` `-pie` `-shared` `-static` `-static 
> > -pie`.
> We've checked the glibc implementation of `__cxa_finalize`. A nonzero 
> `__dso_handle` has to match the value passed to `__cxa_atexit` but a zero 
> `__dso_handle` matches every function registered. So it matters that DSO fini 
> calls use `&__dso_handle` to match their registrations for the `dlclose` 
> case, but it also matters that the main executable fini call use zero to run 
> all the dtors at exit time. It's not clear it really needs to be that way, 
> but it would affect how the dtors get run which might affect some use cases. 
> Hence, I don't think we can combine `crtbegin.o` and `crtbegin_shared.o`.
I may be wrong but from what I can see crtend and crtend_shared are identical, 
so while the crtbegins must stay separate can't the 2 crtends be merged into 
one that gets used in all cases instead of having a duplicate object under a 
different name?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek marked an inline comment as done.
phosek added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:563
+  crtend = Args.hasArg(options::OPT_shared) || IsPIE || IsStaticPIE ?
+  "crtend_shared" : "crtend";
+  CmdArgs.push_back(ToolChain.getCompilerRTArgString(

MaskRay wrote:
> phosek wrote:
> > MaskRay wrote:
> > > I believe `crtbegin.o` `crtend.o` should just work. It is not necessary 
> > > to use `crtbegin_shared.o` `crtend_shared.o`.
> > This is related to your comments on D28791, specifically that we should be 
> > using `crtbegin_shared.o` for `-shared` or `-pie` and `crtbegin.o` 
> > otherwise, is that not the case?
> Yes. I think we can rename `crtbegin_shared.o` to `crtbegin.o` and use it for 
> every configuration: `-no-pie` `-pie` `-shared` `-static` `-static -pie`.
We've checked the glibc implementation of `__cxa_finalize`. A nonzero 
`__dso_handle` has to match the value passed to `__cxa_atexit` but a zero 
`__dso_handle` matches every function registered. So it matters that DSO fini 
calls use `&__dso_handle` to match their registrations for the `dlclose` case, 
but it also matters that the main executable fini call use zero to run all the 
dtors at exit time. It's not clear it really needs to be that way, but it would 
affect how the dtors get run which might affect some use cases. Hence, I don't 
think we can combine `crtbegin.o` and `crtbegin_shared.o`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:563
+  crtend = Args.hasArg(options::OPT_shared) || IsPIE || IsStaticPIE ?
+  "crtend_shared" : "crtend";
+  CmdArgs.push_back(ToolChain.getCompilerRTArgString(

phosek wrote:
> MaskRay wrote:
> > I believe `crtbegin.o` `crtend.o` should just work. It is not necessary to 
> > use `crtbegin_shared.o` `crtend_shared.o`.
> This is related to your comments on D28791, specifically that we should be 
> using `crtbegin_shared.o` for `-shared` or `-pie` and `crtbegin.o` otherwise, 
> is that not the case?
Yes. I think we can rename `crtbegin_shared.o` to `crtbegin.o` and use it for 
every configuration: `-no-pie` `-pie` `-shared` `-static` `-static -pie`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek marked an inline comment as done.
phosek added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:563
+  crtend = Args.hasArg(options::OPT_shared) || IsPIE || IsStaticPIE ?
+  "crtend_shared" : "crtend";
+  CmdArgs.push_back(ToolChain.getCompilerRTArgString(

MaskRay wrote:
> I believe `crtbegin.o` `crtend.o` should just work. It is not necessary to 
> use `crtbegin_shared.o` `crtend_shared.o`.
This is related to your comments on D28791, specifically that we should be 
using `crtbegin_shared.o` for `-shared` or `-pie` and `crtbegin.o` otherwise, 
is that not the case?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:563
+  crtend = Args.hasArg(options::OPT_shared) || IsPIE || IsStaticPIE ?
+  "crtend_shared" : "crtend";
+  CmdArgs.push_back(ToolChain.getCompilerRTArgString(

I believe `crtbegin.o` `crtend.o` should just work. It is not necessary to use 
`crtbegin_shared.o` `crtend_shared.o`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek created this revision.
phosek added a reviewer: echristo.
Herald added subscribers: cfe-commits, jdoerfert, fedor.sergeev, dberris, 
srhines.
Herald added a project: clang.
phosek edited the summary of this revision.

When compiler-rt is selected as the runtime library for Linux
use its crtbegin.o/crtend.o implementation rather than platform one.


Repository:
  rC Clang

https://reviews.llvm.org/D59264

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/linux-ld.c

Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -2,7 +2,7 @@
 // sysroot to make these tests independent of the host system.
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux \
+// RUN: --target=i386-unknown-linux -rtlib=platform \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-32 %s
@@ -51,16 +51,18 @@
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN: --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT %s
 // CHECK-LD-RT-NOT: warning:
+// CHECK-LD-RT: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-LD-RT: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT: "--eh-frame-hdr"
 // CHECK-LD-RT: "-m" "elf_x86_64"
 // CHECK-LD-RT: "-dynamic-linker"
-// CHECK-LD-RT: "{{.*}}/usr/lib/gcc/x86_64-unknown-linux/4.6.0{{/|}}crtbegin.o"
+// CHECK-LD-RT: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtbegin-x86_64.o"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../../../x86_64-unknown-linux/lib"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/4.6.0/../../.."
@@ -69,19 +71,22 @@
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
 // CHECK-LD-RT: "-lc"
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtend-x86_64.o"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=i686-unknown-linux \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN: --rtlib=compiler-rt \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RT-I686 %s
 // CHECK-LD-RT-I686-NOT: warning:
+// CHECK-LD-RT-I686: "-resource-dir" "[[RESDIR:[^"]*]]"
 // CHECK-LD-RT-I686: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-LD-RT-I686: "--eh-frame-hdr"
 // CHECK-LD-RT-I686: "-m" "elf_i386"
 // CHECK-LD-RT-I686: "-dynamic-linker"
-// CHECK-LD-RT-I686: "{{.*}}/usr/lib/gcc/i686-unknown-linux/4.6.0{{/|}}crtbegin.o"
+// CHECK-LD-RT-I686: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtbegin-i386.o"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../../../i686-unknown-linux/lib"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib/gcc/i686-unknown-linux/4.6.0/../../.."
@@ -90,6 +95,7 @@
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
 // CHECK-LD-RT-I686: "-lc"
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "[[RESDIR]]{{/|}}lib{{/|}}linux{{/|}}clang_rt.crtend-i386.o"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=arm-linux-androideabi \
@@ -110,7 +116,6 @@
 // RUN: --target=x86_64-unknown-linux -rtlib=platform \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
-// RUN: --rtlib=libgcc \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-GCC %s
 // CHECK-LD-GCC-NOT: warning:
 // CHECK-LD-GCC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
@@ -291,7 +296,7 @@
 // RUN:   | FileCheck --check-prefix=CHECK-LD-64-STATIC %s
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux -m32 \
+// RUN: --target=i386-unknown-linux -rtlib=platform -m32 \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/multilib_32bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-32-TO-32 %s
@@ -308,7 +313,7 @@
 // CHECK-32-TO-32: "-L[[SYSROOT]]/usr/lib"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
-// RUN: --target=i386-unknown-linux -m64 \
+// RUN: --target=i386-unknown-linux -rtlib=platform -m64 \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/multilib_32bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-32-TO-64 %s
@@ -326,7 +331,7 @@
 // CHECK-32-TO-64: "-L[[SYSROOT]]/usr/lib"
 //
 // RUN: %cl