[PATCH] D158206: [Driver] Add PIE support on Solaris

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:136
+    const char *crtbegin = nullptr;
+    if (Args.hasArg(options::OPT_shared) || IsPIE)
+      crtbegin = "crtbeginS.o";
----------------
Q: Interesting. If -shared used crtbegin.o before, how did it work?

On Linux, using GCC crtbegin.o (compiled with `-fno-PIC` or similar non-PIC option) will cause:
```
ld.lld: error: relocation R_X86_64_32S cannot be used against local symbol; recompile with -fPIC
>>> defined in /usr/lib/gcc/x86_64-linux-gnu/12/crtbegin.o
>>> referenced by crtstuff.c
>>>               /usr/lib/gcc/x86_64-linux-gnu/12/crtbegin.o:(.text+0x7)

ld.lld: error: relocation R_X86_64_32 cannot be used against symbol '_ITM_deregisterTMCloneTable'; recompile with -fPIC
>>> defined in /usr/lib/gcc/x86_64-linux-gnu/12/crtbegin.o
>>> referenced by crtstuff.c
>>>               /usr/lib/gcc/x86_64-linux-gnu/12/crtbegin.o:(.text+0xE)

ld.lld: error: relocation R_X86_64_32 cannot be used against local symbol; recompile with -fPIC
>>> defined in /usr/lib/gcc/x86_64-linux-gnu/12/crtbegin.o
>>> referenced by crtstuff.c
>>>               /usr/lib/gcc/x86_64-linux-gnu/12/crtbegin.o:(.text+0x18)
```


================
Comment at: clang/test/Driver/solaris-ld.c:112
+// RUN:     --target=sparc-sun-solaris2.11 \
+// RUN:     --gcc-toolchain="" \
+// RUN:     --sysroot=%S/Inputs/solaris_sparc_tree 2>&1 \
----------------
Ultra nit: you may pack --target=sparc-sun-solaris2.11 on the first line to make the commands compacter, like `%clang --target=sparc-sun-solaris2.11 -### %s` below.

FYI: I think `--gcc-toolchain=""` is to support `GCC_INSTALL_PREFIX`. I propose that we deprecate the CMake variable to make test maintaining easier: D158218.


================
Comment at: clang/test/Driver/solaris-ld.c:110
+// Check the right ld flags are present with -pie.
+// RUN: %clang -### %s -pie 2>&1 \
+// RUN:     --target=sparc-sun-solaris2.11 \
----------------
ro wrote:
> MaskRay wrote:
> > The convention is to put `2>&1` at the end of the command, aka before `|`
> I've changed just the newly added tests.  There are a few others, but I've left the as is for now to avoid cluttering the patch with unrelated changes.
Ultra nit: you may pack --target=sparc-sun-solaris2.11 on the first line to make the commands compacter, like `%clang --target=sparc-sun-solaris2.11 -### %s` below.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158206