Skip to content

Conversation

@playstation-edd
Copy link
Contributor

If a linker script is explicitly supplied, there's no benefit to supplying a default script.

SIE tracker: TOOLCHAIN-17524

If a linker script is supplied, there's no benefit to supplying a default
script.

SIE tracker: TOOLCHAIN-17524
@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 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Edd Dawson (playstation-edd)

Changes

If a linker script is explicitly supplied, there's no benefit to supplying a default script.

SIE tracker: TOOLCHAIN-17524


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/PS4CPU.cpp (+6-4)
  • (modified) clang/test/Driver/ps5-linker.c (+6-1)
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index df43da93d77555..03445375796533 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -303,10 +303,12 @@ void tools::PS5cpu::Linker::ConstructJob(Compilation &C, const JobAction &JA,
     // with the SDK. The scripts are inside <sdkroot>/target/lib, which is
     // added as a search path elsewhere.
     // "PRX" has long stood for "PlayStation Relocatable eXecutable".
-    CmdArgs.push_back("--default-script");
-    CmdArgs.push_back(Static   ? "static.script"
-                      : Shared ? "prx.script"
-                               : "main.script");
+    if (!Args.hasArgNoClaim(options::OPT_T)) {
+      CmdArgs.push_back("--default-script");
+      CmdArgs.push_back(Static   ? "static.script"
+                        : Shared ? "prx.script"
+                                 : "main.script");
+    }
   }
 
   if (Static)
diff --git a/clang/test/Driver/ps5-linker.c b/clang/test/Driver/ps5-linker.c
index 95267942edc172..216b11a8c52d71 100644
--- a/clang/test/Driver/ps5-linker.c
+++ b/clang/test/Driver/ps5-linker.c
@@ -67,12 +67,14 @@
 // CHECK-NO-EXE-NOT: "--unresolved-symbols
 // CHECK-NO-EXE-NOT: "-z"
 
-// Test that an appropriate linker script is supplied by the driver.
+// Test that an appropriate linker script is supplied by the driver, but can
+// be overridden with -T.
 
 // RUN: %clang --target=x86_64-sie-ps5 %s -### 2>&1 | FileCheck --check-prefixes=CHECK-SCRIPT -DSCRIPT=main %s
 // RUN: %clang --target=x86_64-sie-ps5 %s -shared -### 2>&1 | FileCheck --check-prefixes=CHECK-SCRIPT -DSCRIPT=prx %s
 // RUN: %clang --target=x86_64-sie-ps5 %s -static -### 2>&1 | FileCheck --check-prefixes=CHECK-SCRIPT -DSCRIPT=static %s
 // RUN: %clang --target=x86_64-sie-ps5 %s -r -### 2>&1 | FileCheck --check-prefixes=CHECK-NO-SCRIPT %s
+// RUN: %clang --target=x86_64-sie-ps5 %s -T custom.script -### 2>&1 | FileCheck --check-prefixes=CHECK-CUSTOM-SCRIPT --implicit-check-not "\"{{-T|--script|--default-script}}\"" %s
 
 // CHECK-SCRIPT: {{ld(\.exe)?}}"
 // CHECK-SCRIPT-SAME: "--default-script" "[[SCRIPT]].script"
@@ -80,6 +82,9 @@
 // CHECK-NO-SCRIPT: {{ld(\.exe)?}}"
 // CHECK-NO-SCRIPT-NOT: "--default-script"
 
+// CHECK-CUSTOM-SCRIPT: {{ld(\.exe)?}}"
+// CHECK-CUSTOM-SCRIPT-SAME: "-T" "custom.script"
+
 // Test that -static is forwarded to the linker
 
 // RUN: %clang --target=x86_64-sie-ps5 -static %s -### 2>&1 | FileCheck --check-prefixes=CHECK-STATIC %s


// Test that an appropriate linker script is supplied by the driver.
// Test that an appropriate linker script is supplied by the driver, but can
// be overridden with -T.
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: -T/--script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no --script on clang. I may not understand your comment.

Copy link
Collaborator

@bd1976bris bd1976bris Nov 13, 2024

Choose a reason for hiding this comment

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

No you haven't. I just assumed that there was, apologies.

Copy link
Collaborator

@bd1976bris bd1976bris left a comment

Choose a reason for hiding this comment

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

LGTM. I would have preferred a simpler way of checking that there are no other occurrences of -T/--script/--default-script but couldn't think of anything.

I didn't notice that this was an upstream review initially. It would be worth waiting for approval from Jeremy.

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

@playstation-edd playstation-edd merged commit 67c4345 into llvm:main Nov 13, 2024
11 checks passed
@playstation-edd playstation-edd deleted the ps5-driver-override-default-script branch November 13, 2024 17:18
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.

4 participants