[PATCH] D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: compiler-rt/lib/crt/CMakeLists.txt:81
+    SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/crtbegin.c
+    CFLAGS ${CRT_CFLAGS}
+    PARENT_TARGET crt)
----------------
phosek wrote:
> MaskRay wrote:
> > phosek wrote:
> > > MaskRay wrote:
> > > > Just to ask if my understanding is correct.
> > > > 
> > > > clang_rt.crtbegin.o: replacement of crtbegin.o
> > > > (-fno-PIC can be used here as some toolchains default to -fPIC or -fPIE, but it doesn't matter much here)
> > > > 
> > > > clang_rt.crtbegin_shared.o: replacement of crtbeginS.o crtbeginT.o
> > > > 
> > > Correct, `crtbeginS.o` and `crtbeginT.o` don't match the naming conventions of other compiler-rt artifacts, so I came up with this scheme which should be a better fit for compiler-rt. I'm open to other suggestions though.
> > Since `crtbeginT.o` is used by `-static` (`gcc -dumpspecs`: `%{static:crtbeginT.o%s;      shared|!no-pie:crtbeginS.o%s;      :crtbegin.o%s}`), `crtbegin_shared` may not be a good name. But I don't know of a better name, either...
> > 
> > Other than this, other parts look great! Thank you for doing this.
> Android seems t be using `crtbegin_dynamic.o` but I don't know if that's much better?
Given a second thought. `crtbegin_shared.o` looks good to me now. I propose one change: adding `-fPIC` to the `crtbegin.o` rule.

While I still don't understand why ligcc seems to define `void *__dso_handle = &dso_handle;` in `crtbeginT.o`, I cannot find a relocation in my `/usr/lib/gcc/x86_64-linux-gnu/8/crtbeginT.o`, not sure if it has other rule to remove that...

```
# https://github.com/gcc-mirror/gcc/blob/master/libgcc/Makefile.in
crtbeginT$(objext): $(srcdir)/crtstuff.c
	$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $< -DCRT_BEGIN -DCRTSTUFFT_O

// https://github.com/gcc-mirror/gcc/blob/master/libgcc/crtstuff.c
#ifdef CRTSTUFFS_O
void *__dso_handle = &__dso_handle; // crtbeginS.o crtbeginT.o
#else
void *__dso_handle = 0; // crtbegin.o
#endif
```

If we unify 2 of the 3, I think unifying `crtbegin.o` and `crtbeginT.o` rather than unifying `crtbeginS.o` `crtbeginT.o` makes more sense, but we'd need `-fPIC` for `crtbegin.o` (`-static -pie`, or `-fPIC` object files going into `-static` link). This resembles the status I observed from my Linux `crtbegin*.o` and https://svnweb.freebsd.org/base/head/lib/csu/common/crtbegin.c?view=markup

I even think unifying all 3 is possible. `__dso_handle` doesn't need to be 0 if all modules (including `LD_PRELOAD` DSOs) are unloaded before the main executable. There would be no difference if a nullptr or another address is used as the `__cxa_finalize` argument in that case. There may be cases I missed, so the current approach (unifying 2) is good enough :)


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

https://reviews.llvm.org/D28791