Skip to content

Reapply "[nfc][Driver] Remove {{(.exe)?}} from sanitizer test (#121160)" #121162

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

Jump to bottom
Merged
merged 3 commits into from
Dec 26, 2024
Merged

Reapply "[nfc][Driver] Remove {{(.exe)?}} from sanitizer test (#121160)" #121162

merged 3 commits into from
Dec 26, 2024

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Dec 26, 2024 •
edited
Loading

This reverts commit ca28fcc.

Android targets pass on Windows buildbots, but fails on buildkite
https://buildkite.com/llvm-project/github-pull-requests/builds/132244

Re-apply #121160

@vitalybuka
[𝘀𝗽𝗿] initial version
5f34830 
Created using spr 1.3.4
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Dec 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 26, 2024

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

Target "androideabi" adds .exe in some cases.

#121160

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

1 Files Affected:

  • (modified) clang/test/Driver/sanitizer-ld.c (+4-4)
diff --git a/clang/test/Driver/sanitizer-ld.c b/clang/test/Driver/sanitizer-ld.c
index 763230568251c6..54e153f2e47eb4 100644
--- a/clang/test/Driver/sanitizer-ld.c
+++ b/clang/test/Driver/sanitizer-ld.c
@@ -242,7 +242,7 @@
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID
 //
-// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID: "-pie"
 // CHECK-ASAN-ANDROID-NOT: "-lc"
 // CHECK-ASAN-ANDROID-NOT: "-lpthread"
@@ -265,7 +265,7 @@
 // RUN:     -static-libasan \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID-STATICLIBASAN
 //
-// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-STATICLIBASAN: libclang_rt.asan.a"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lpthread"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lrt"
@@ -306,7 +306,7 @@
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID-X86
 //
-// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-X86: "-pie"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lc"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lpthread"
@@ -330,7 +330,7 @@
 // RUN:     -shared \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID-SHARED
 //
-// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lc"
 // CHECK-ASAN-ANDROID-SHARED: libclang_rt.asan.so"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lpthread"

@llvmbot
Copy link
Member

llvmbot commented Dec 26, 2024

@llvm/pr-subscribers-clang-driver

Author: Vitaly Buka (vitalybuka)

Changes

Target "androideabi" adds .exe in some cases.

#121160

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

1 Files Affected:

  • (modified) clang/test/Driver/sanitizer-ld.c (+4-4)
diff --git a/clang/test/Driver/sanitizer-ld.c b/clang/test/Driver/sanitizer-ld.c
index 763230568251c6..54e153f2e47eb4 100644
--- a/clang/test/Driver/sanitizer-ld.c
+++ b/clang/test/Driver/sanitizer-ld.c
@@ -242,7 +242,7 @@
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID
 //
-// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID: "-pie"
 // CHECK-ASAN-ANDROID-NOT: "-lc"
 // CHECK-ASAN-ANDROID-NOT: "-lpthread"
@@ -265,7 +265,7 @@
 // RUN:     -static-libasan \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID-STATICLIBASAN
 //
-// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID-STATICLIBASAN: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-STATICLIBASAN: libclang_rt.asan.a"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lpthread"
 // CHECK-ASAN-ANDROID-STATICLIBASAN-NOT: "-lrt"
@@ -306,7 +306,7 @@
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID-X86
 //
-// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID-X86: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-X86: "-pie"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lc"
 // CHECK-ASAN-ANDROID-X86-NOT: "-lpthread"
@@ -330,7 +330,7 @@
 // RUN:     -shared \
 // RUN:   | %{filecheck} --check-prefix=CHECK-ASAN-ANDROID-SHARED
 //
-// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld"
+// CHECK-ASAN-ANDROID-SHARED: "{{(.*[^.0-9A-Z_a-z])?}}ld.lld{{(.exe)?}}"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lc"
 // CHECK-ASAN-ANDROID-SHARED: libclang_rt.asan.so"
 // CHECK-ASAN-ANDROID-SHARED-NOT: "-lpthread"

@vitalybuka vitalybuka added the skip-precommit-approval PR for CI feedback, not intended for review label Dec 26, 2024
@vitalybuka vitalybuka requested a review from thurstond December 26, 2024 22:24
@vitalybuka
more android
4279503 
Created using spr 1.3.4
@vitalybuka
rebase
eb5d8ef 
Created using spr 1.3.4
@vitalybuka vitalybuka changed the title [Drive] Fix test on Windows Reapply "[nfc][Driver] Remove {{(.exe)?}} from sanitizer test (#121160)" Dec 26, 2024
@vitalybuka vitalybuka merged commit 8e9fda1 into main Dec 26, 2024
8 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/drive-fix-test-on-windows branch December 26, 2024 23:32
@llvm-ci
Copy link
Collaborator

llvm-ci commented Dec 26, 2024

LLVM Buildbot has detected a new failure on builder clang-cmake-x86_64-avx512-win running on avx512-intel64-win while building clang at step 4 "cmake stage 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/81/builds/3387

Here is the relevant piece of the build log for the reference 
Step 4 (cmake stage 1) failure: 'cmake -G ...' (failure)
'cmake' is not recognized as an internal or external command,
operable program or batch file.

@bogner
Copy link
Contributor

bogner commented Dec 28, 2024

These tests are failing for me locally on windows now. Why are you removing this?

@carlocab
Copy link
Member

I don't really understand why this is needed either. Should we revert?

bogner added a commit to bogner/llvm-project that referenced this pull request Dec 29, 2024
@bogner
Revert "[nfc][Driver] Remove {{(.exe)?}} from sanitizer test" again
650dd5a 
This reverts llvm#121162, which was a reapply of the previous revert earlier
in llvm#121160 - The change blatantly breaks tests on windows and it isn't
clear why it's being made.

This reverts commit 8e9fda1.
@bogner bogner mentioned this pull request Dec 29, 2024
Merged
bogner added a commit to bogner/llvm-project that referenced this pull request Dec 29, 2024
@bogner
Add optional .exe regex to the checks added after llvm#121162
ec25aad
bogner added a commit that referenced this pull request Dec 29, 2024
@bogner
Revert "[nfc][Driver] Remove {{(.exe)?}} from sanitizer test" again (#…
02e8972 
…121280)

This reverts #121162, which was a reapply of the previous revert earlier
in #121160 - The change blatantly breaks tests on windows and it isn't
clear why it's being made.

Note that I needed to add the optional .exe suffix to a few more check
lines that were added as a follow up.

This reverts commit 8e9fda1.
@bogner
Copy link
Contributor

bogner commented Dec 29, 2024

I've gone ahead and reverted it.

@MaskRay
Copy link
Member

MaskRay commented Dec 29, 2024

This is not correct. Windows GetLinkerPath does append .exe. For certain tests where Windows's .exe and backslashes are annoying, we could employ UNSUPPORTED: system-windows. I am puzzled by the description about Android.

@vitalybuka
Copy link
Collaborator Author

vitalybuka commented Dec 29, 2024 •
edited
Loading

I've gone ahead and reverted it.

Thanks for reverting.

Could you please provide some information on how it fails? Error output would be enough. Upstream windows bot are OK.

Windows GetLinkerPath does append .exe

How do upstream bots pass?
E.g. the one above is unrelated flake, and subsequent runs are green.

UNSUPPORTED: system-windows

It's not worth it. The idea was just to simplify the test. So I'll just give up on the patch.

@vitalybuka
Copy link
Collaborator Author

I am puzzled by the description about Android.

There is this configured buildkite https://buildkite.com/llvm-project/github-pull-requests/builds/132244

It's Windows, but only Android lines of the test fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thurstond thurstond thurstond approved these changes

Assignees
No one assigned
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category skip-precommit-approval PR for CI feedback, not intended for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vitalybuka @llvmbot @llvm-ci @bogner @carlocab @MaskRay @thurstond