[PS4,PS5][Driver] Check for absent SDK when -nostdlib/-nodefaultlibs #107112
Conversation
The PlayStation drivers emit warnings if it looks like SDK libraries are missing. Until this point, the check was skipped when either -nostdlib or -nodefaultlibs was supplied. I believe the idea is that if you aren't linking default libraries, you won't be in need of the SDK. However, in a situation where these switches are supplied, users may still want to pass `-lSomeSDKLib` to the driver/linker with the expectation that libSomeSDKLib.a will be sourced from the SDK. That is, -nodefaultlibs and -nostdlib affect the libraries passed to the linker, but not the library search paths. So this change removes -nostdlib/-nodefaultlibs from consideration when deciding whether or not to probe for the SDK's existence. N.B. complete behaviour for -nostdlib and -nodefaultlibs is yet to be added to the PlayStation compiler drivers. Coming soon. SIE tracker: TOOLCHAIN-16704
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Edd Dawson (playstation-edd) ChangesThe PlayStation drivers emit warnings if it looks like SDK libraries are missing. Until this point, the check was skipped when either -nostdlib or -nodefaultlibs was supplied. I believe the idea is that if you aren't linking default libraries, you won't be in need of the SDK. However, in a situation where these switches are supplied, users may still want to pass So this change removes -nostdlib/-nodefaultlibs from consideration when deciding whether or not to probe for the SDK's existence. N.B. complete behaviour for -nostdlib and -nodefaultlibs is yet to be added to the PlayStation compiler drivers. Coming soon. SIE tracker: TOOLCHAIN-16704 Full diff: https://github.com/llvm/llvm-project/pull/107112.diff 3 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 22103eb50803a5..54ec59e6398f85 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -354,9 +354,7 @@ toolchains::PS4PS5Base::PS4PS5Base(const Driver &D, const llvm::Triple &Triple,
SmallString<512> SDKLibDir(SDKRootDir);
llvm::sys::path::append(SDKLibDir, "target/lib");
- if (!Args.hasArg(options::OPT_nostdlib) &&
- !Args.hasArg(options::OPT_nodefaultlibs) &&
- !Args.hasArg(options::OPT__sysroot_EQ) && !Args.hasArg(options::OPT_E) &&
+ if (!Args.hasArg(options::OPT__sysroot_EQ) && !Args.hasArg(options::OPT_E) &&
!Args.hasArg(options::OPT_c) && !Args.hasArg(options::OPT_S) &&
!Args.hasArg(options::OPT_emit_ast) &&
!llvm::sys::fs::exists(SDKLibDir)) {
diff --git a/clang/test/Driver/ps4-sdk-root.c b/clang/test/Driver/ps4-sdk-root.c
index e1a04522030c1e..3e02fa9fc3bc29 100644
--- a/clang/test/Driver/ps4-sdk-root.c
+++ b/clang/test/Driver/ps4-sdk-root.c
@@ -6,9 +6,8 @@
// Check that PS4 clang doesn't report a warning message when locating
// system libraries (either by looking at the value of SCE_ORBIS_SDK_DIR
-// or relative to the location of the compiler driver), if "-c", "-S", "-E",
-// "--sysroot", "-nostdlib" or "-nodefaultlibs" option is specified on
-// the command line.
+// or relative to the location of the compiler driver), if "-c", "-S", "-E"
+// or "--sysroot" option is specified on the command line.
// Otherwise, check that PS4 clang reports a warning.
// Setting up SCE_ORBIS_SDK_DIR to existing location, which is not a PS4 SDK.
@@ -36,9 +35,6 @@
// RUN: env SCE_ORBIS_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -emit-ast -isysroot foo -target x86_64-scei-ps4 %s 2>&1 | FileCheck -check-prefix=WARN-ISYSROOT -check-prefix=NO-WARN %s
// RUN: env SCE_ORBIS_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### --sysroot=foo/ -isysroot foo -target x86_64-scei-ps4 %s 2>&1 | FileCheck -check-prefix=WARN-ISYSROOT -check-prefix=NO-WARN %s
-// RUN: env SCE_ORBIS_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -nostdlib -target x86_64-scei-ps4 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s
-// RUN: env SCE_ORBIS_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -nodefaultlibs -target x86_64-scei-ps4 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s
-
// NO-WARN-NOT: {{warning:|error:}}
// WARN-SYS-HEADERS: warning: unable to find PS4 system headers directory
// WARN-ISYSROOT: warning: no such sysroot directory: 'foo'
diff --git a/clang/test/Driver/ps5-sdk-root.c b/clang/test/Driver/ps5-sdk-root.c
index c3672aef9dc0c1..2a82d8e72283b7 100644
--- a/clang/test/Driver/ps5-sdk-root.c
+++ b/clang/test/Driver/ps5-sdk-root.c
@@ -8,12 +8,11 @@
// Check that PS5 clang doesn't report a warning message when locating
// system libraries (either by looking at the value of SCE_PROSPERO_SDK_DIR
-// or relative to the location of the compiler driver), if "-c", "-S", "-E",
-// "--sysroot", "-nostdlib" or "-nodefaultlibs" option is specified on
-// the command line.
+// or relative to the location of the compiler driver), if "-c", "-S", "-E"
+// or "--sysroot" option is specified on the command line.
// Otherwise, check that PS5 clang reports a warning.
-// Setting up SCE_PROSPERO_SDK_DIR to existing location, which is not a PS4 SDK.
+// Setting up SCE_PROSPERO_SDK_DIR to existing location, which is not a PS5 SDK.
// RUN: env SCE_PROSPERO_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -target x86_64-sie-ps5 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=WARN-SYS-LIBS -check-prefix=NO-WARN %s
// RUN: env SCE_PROSPERO_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -c -target x86_64-sie-ps5 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s
@@ -38,9 +37,6 @@
// RUN: env SCE_PROSPERO_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -emit-ast -isysroot foo -target x86_64-sie-ps5 %s 2>&1 | FileCheck -check-prefix=WARN-ISYSROOT -check-prefix=NO-WARN %s
// RUN: env SCE_PROSPERO_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### --sysroot=foo/ -isysroot foo -target x86_64-sie-ps5 %s 2>&1 | FileCheck -check-prefix=WARN-ISYSROOT -check-prefix=NO-WARN %s
-// RUN: env SCE_PROSPERO_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -nostdlib -target x86_64-sie-ps5 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s
-// RUN: env SCE_PROSPERO_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -nodefaultlibs -target x86_64-sie-ps5 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s
-
// NO-WARN-NOT: {{warning:|error:}}
// WARN-SYS-HEADERS: warning: unable to find PS5 system headers directory
// WARN-ISYSROOT: warning: no such sysroot directory: 'foo'
|
| // RUN: env SCE_ORBIS_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -nostdlib -target x86_64-scei-ps4 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s | ||
| // RUN: env SCE_ORBIS_SDK_DIR=.. %clang -Winvalid-or-nonexistent-directory -### -nodefaultlibs -target x86_64-scei-ps4 %s 2>&1 | FileCheck -check-prefix=WARN-SYS-HEADERS -check-prefix=NO-WARN %s |
There was a problem hiding this comment.
Just to confirm my understanding, we're intending that the library check occurs, testing that the diagnostic occurs should be covered by an earlier RUN line, thus it's correct to delete these?
The opposing view would be "we're changing the compiler-driver behaviour, so should positively test that it does what we want", which would mean leaving these RUN lines here and checking that they behave the same as without the flags (i.e. --check-prefixes=WARN-SYS-HEADERS,WARN-SYS-LIBS,NO-WARN). I lean slightly in this direction, @pogo59 how do you feel?
There was a problem hiding this comment.
If we had done it this way in the first place, we wouldn't have added RUN lines to check these conditions, so on that basis I'm okay with deleting these RUN lines and not checking that -nodefaultlibs -nostdlib have no effect on the warnings.
There was a problem hiding this comment.
(EDIT: I was racing with Paul while typing this)
I generally favour leaving the test as it would be if the code was like that in the first place, as it avoids indefinite accumulation. Assuming the commit goes in as thing stands, there's no more reason to check for interaction with -nodefaultlibs as there is for interaction with -nolibc (which the code ignores, currently). And so it also avoids hitches for the reader ("well of course there's no interaction with xyz... why would be checking it?").
With any luck, near future changes will go on to implement complete behaviour for all these -no... switches for library control. At which point new affirmative checks will be introduced.
With my case made, I'm also content to amend if we want that.
The PlayStation drivers emit warnings if it looks like SDK libraries are missing. Until this point, the check was skipped when either -nostdlib or -nodefaultlibs was supplied. I believe the idea is that if you aren't linking default libraries, you won't be in need of the SDK.
However, in a situation where these switches are supplied, users may still want to pass
-lSomeSDKLibto the driver/linker with the expectation that libSomeSDKLib.a will be sourced from the SDK. That is, -nodefaultlibs and -nostdlib affect the libraries passed to the linker, but not the library search paths.So this change removes -nostdlib/-nodefaultlibs from consideration when deciding whether or not to probe for the SDK's existence.
N.B. complete behaviour for -nostdlib and -nodefaultlibs is yet to be added to the PlayStation compiler drivers. Coming soon.
SIE tracker: TOOLCHAIN-16704