Skip to content

Conversation

joaosaffran
Copy link
Contributor

@joaosaffran joaosaffran commented Oct 1, 2025

Running those test with validation enabled, causes emitting an additional intermediary object, which causes the checks to fail, since it will emit 2 obj, instead of one obj and one dxo. This patch changes the test to make sure validation is disabled, making the test consistent across environments.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Oct 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Oct 1, 2025

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: None (joaosaffran)

Changes

Running those test in WSL seem to be generating a different file extension, obj instead of dxo, to fix that I am allowing the test to pass with both files


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

2 Files Affected:

  • (modified) clang/test/Driver/dxc_frs.hlsl (+1-1)
  • (modified) clang/test/Driver/dxc_rootsignature_target.hlsl (+1-1)
diff --git a/clang/test/Driver/dxc_frs.hlsl b/clang/test/Driver/dxc_frs.hlsl
index 767cab604c829..b3b685b10f01f 100644
--- a/clang/test/Driver/dxc_frs.hlsl
+++ b/clang/test/Driver/dxc_frs.hlsl
@@ -3,7 +3,7 @@
 // Test to demonstrate extracting the root signature to the specified
 // output file with /Frs.
 
-// CHECK: "{{.*}}llvm-objcopy{{(.exe)?}}" "{{.*}}.obj" "{{.*}}.dxo" "--extract-section=RTS0={{.*}}.rs.dxo"
+// CHECK: "{{.*}}llvm-objcopy{{(.exe)?}}" "{{.*}}.obj" "{{.*}}.{{(dxo|obj)}}" "--extract-section=RTS0={{.*}}.rs.dxo"
 
 [shader("compute"), RootSignature("")]
 [numthreads(1,1,1)]
diff --git a/clang/test/Driver/dxc_rootsignature_target.hlsl b/clang/test/Driver/dxc_rootsignature_target.hlsl
index 08cd1ab00089b..784523707746d 100644
--- a/clang/test/Driver/dxc_rootsignature_target.hlsl
+++ b/clang/test/Driver/dxc_rootsignature_target.hlsl
@@ -3,6 +3,6 @@
 // CMDS: "{{.*}}clang{{.*}}" "-cc1"
 // CMDS-SAME: "-triple" "dxilv1.1-unknown-shadermodel1.1-rootsignature"
 // CMDS-SAME: "-hlsl-entry" "EntryRS"
-// CMDS: "{{.*}}llvm-objcopy{{(.exe)?}}" "{{.*}}.dxo" "--only-section=RTS0"
+// CMDS: "{{.*}}llvm-objcopy{{(.exe)?}}" "{{.*}}.{{(dxo|obj)}}" "--only-section=RTS0"
 
 #define EntryRS "UAV(u0)"

Icohedron
Icohedron previously approved these changes Oct 1, 2025
Copy link
Contributor

@Icohedron Icohedron left a comment

Choose a reason for hiding this comment

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

Great! I was having issues with this too

Copy link
Contributor

@bogner bogner left a comment

Choose a reason for hiding this comment

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

Why is the extension different under WSL? This change might be reasonable, but I find that difference quite surprising and wonder if this is masking another bug. Can we look into the answer to that before making this change please?

@bogner bogner added the HLSL HLSL Language Support label Oct 2, 2025
@spall
Copy link
Contributor

spall commented Oct 2, 2025

Why is the extension different under WSL? This change might be reasonable, but I find that difference quite surprising and wonder if this is masking another bug. Can we look into the answer to that before making this change please?

from googling it seems that the dxo file type is not supported on linux? But before looking at this PR I'd never heard of this file type, so its possible I'm not reading about the correct one.

@damyanp
Copy link
Contributor

damyanp commented Oct 2, 2025

If you specify /Fo foo.dxo on the command-line then clang-dxc should write out a file called foo.dxo, not foo.obj.

@joaosaffran joaosaffran requested a review from bogner October 3, 2025 00:02
@Icohedron Icohedron dismissed their stale review October 3, 2025 00:16

Code changed significantly.

@joaosaffran
Copy link
Contributor Author

After investigating a little further:

If validation is disabled, that leads to 2 actions being created. Otherwise, validation is enabled, it creates 3 actions in the driver, that means that without validation only emits 1 intermediary object, meanwhile, with validation emits 2. In this section of the code https://github.com/llvm/llvm-project/blob/79d1524bde4c0253b349304e70716c3fb4f7193e/clang/lib/Driver… We handle the root signature objcopy flags, the files are handled differently if it is the last job or not. If it is not the last job, we create a temporary file, according to the job we are handling, the first 2 are object jobs, therefore they use the extension obj. If validation is enabled, since the second job is the last one, it doesn't create another temporary file, instead it uses the value associated with /Fo arg

Here is the response in this 2 scenarios:

WITHOUT VALIDATION:
"/home/jderezende/llvm-project/build/bin/llvm-objcopy" "/tmp/dxc_frs-8879c7.obj" "/home/jderezende/llvm-project/build/tools/clang/test/Driver/Output/dxc_frs.hlsl.tmp.dxo" "--extract-section=RTS0=/home/jderezende/llvm-project/build/tools/clang/test/Driver/Output/dxc_frs.hlsl.tmp.rs.dxo"

WITH VALIDATION:
"/home/jderezende/llvm-project/build/bin/llvm-objcopy" "/tmp/dxc_frs-e01b84.obj" "/tmp/dxc_frs-3308ab.obj" "--extract-section=RTS0=/home/jderezende/llvm-project/build/tools/clang/test/Driver/Output/dxc_frs.hlsl.tmp.rs.dxo"

I think the fix will need another approach, other than change the test....

Copy link
Contributor

@bogner bogner left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this. LGTM!

@joaosaffran joaosaffran merged commit d682f61 into llvm:main Oct 3, 2025
9 checks passed
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 HLSL HLSL Language Support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants