Skip to content

[PS4,PS5][Driver] Check for absent SDK when -nostdlib/-nodefaultlibs #107112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 4, 2024
Merged

[PS4,PS5][Driver] Check for absent SDK when -nostdlib/-nodefaultlibs #107112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions clang/lib/Driver/ToolChains/PS4CPU.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
8 changes: 2 additions & 6 deletions clang/test/Driver/ps4-sdk-root.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Comment on lines -39 to -40
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@playstation-edd playstation-edd Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.


// NO-WARN-NOT: {{warning:|error:}}
// WARN-SYS-HEADERS: warning: unable to find PS4 system headers directory
// WARN-ISYSROOT: warning: no such sysroot directory: 'foo'
Expand Down
10 changes: 3 additions & 7 deletions clang/test/Driver/ps5-sdk-root.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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'
Expand Down
Loading