Skip to content
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.

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

[PS5][Driver] Restore whole-archive state when -fjmc #115181

Conversation

playstation-edd
Copy link
Contributor

--whole-archive is passed to the linker to have it consume all objects within the SIE Just My Code library, rather than just those that fulfil outstanding references.

Prior to this change, --no-whole-archive was used to reset the associated archive handling state in the linker, under the assumption that --whole-archive wasn't already in effect. But that assumption may be incorrect. So use --push/pop-state to restore the previous state, whatever that may be.

Given the position of these switches on the link line, the problem described with the outgoing code is unlikely to cause an issue in practice. But push/pop protect against accidents due to future additions to and reorderings of arguments.

PS5 only. The proprietary PS4 linker doesn't support --push/pop-state, or an equivalent.

SIE tracker: TOOLCHAIN-16704.

`--whole-archive` is passed to the linker to have it consume all objects
within the SIE Just My Code library, rather than just those that fulfil
outstanding references.

Prior to this change, `--no-whole-archive` was used to reset the
associated archive handling state in the linker, under the assumption
that `--whole-archive` wasn't already in effect. But that assumption may
be incorrect. So use `--push/pop-state` to restore the previous state,
whatever that may be.

Given the position of these switches on the link line, the problem
described with the outgoing code is unlikely to cause an issue in
practice. But push/pop protect against accidents due to future additions
to and reorderings of arguments.

PS5 only. The proprietary PS4 linker doesn't support `--push/pop-state`,
or an equivalent.

SIE tracker: TOOLCHAIN-16704.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 6, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Edd Dawson (playstation-edd)

Changes

--whole-archive is passed to the linker to have it consume all objects within the SIE Just My Code library, rather than just those that fulfil outstanding references.

Prior to this change, --no-whole-archive was used to reset the associated archive handling state in the linker, under the assumption that --whole-archive wasn't already in effect. But that assumption may be incorrect. So use --push/pop-state to restore the previous state, whatever that may be.

Given the position of these switches on the link line, the problem described with the outgoing code is unlikely to cause an issue in practice. But push/pop protect against accidents due to future additions to and reorderings of arguments.

PS5 only. The proprietary PS4 linker doesn't support --push/pop-state, or an equivalent.

SIE tracker: TOOLCHAIN-16704.


Full diff: https://github.com/llvm/llvm-project/pull/115181.diff

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/PS4CPU.cpp (+2-1)
  • (modified) clang/test/Driver/ps5-linker.c (+1-1)
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 261c136de782e7..df43da93d77555 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -375,9 +375,10 @@ void tools::PS5cpu::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   }
 
   if (UseJMC) {
+    CmdArgs.push_back("--push-state");
     CmdArgs.push_back("--whole-archive");
     CmdArgs.push_back("-lSceJmc_nosubmission");
-    CmdArgs.push_back("--no-whole-archive");
+    CmdArgs.push_back("--pop-state");
   }
 
   if (Args.hasArg(options::OPT_fuse_ld_EQ)) {
diff --git a/clang/test/Driver/ps5-linker.c b/clang/test/Driver/ps5-linker.c
index 8a0ade91871295..fb5f8a9ec5cee5 100644
--- a/clang/test/Driver/ps5-linker.c
+++ b/clang/test/Driver/ps5-linker.c
@@ -95,7 +95,7 @@
 // CHECK: -plugin-opt=-enable-jmc-instrument
 
 // Check the default library name.
-// CHECK-LIB: "--whole-archive" "-lSceJmc_nosubmission" "--no-whole-archive"
+// CHECK-LIB: "--push-state" "--whole-archive" "-lSceJmc_nosubmission" "--pop-state"
 
 // Test the driver's control over the -fcrash-diagnostics-dir behavior with linker flags.
 

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

LGTM, although maybe CHECK_JMC instead of CHECK_LIB as it seems very JMC specific? No strong opinion.

@playstation-edd
Copy link
Contributor Author

LGTM, although maybe CHECK_JMC instead of CHECK_LIB as it seems very JMC specific? No strong opinion.

Done - thanks.

@playstation-edd playstation-edd merged commit 38cc03f into llvm:main Nov 6, 2024
4 of 6 checks passed
@playstation-edd playstation-edd deleted the ps5-driver-restore-whole-archive-jmc branch November 6, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants