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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits