Skip to content

Conversation

AlexeySachkov
Copy link
Contributor

This fixes sporadic issue with check-sycl on Windows. Apparently we had a number of tests, which didn't specify -o %t.out and it caused some tests to write to the same default a.exe and fail with error like:

LINK : fatal error LNK1104: cannot open file 'a.exe'

To resolve this, switch most of such tests to use -fsyntax-only or explicitly added -o %t.out to prevent possible races.

"bad" tests were discovered using grep -rnP 'clangxx(?!.*\-o)' sycl/test/

This fixes sporadic issue with `check-sycl` on Windows.
Apparently we had a number of tests, which didn't specify `-o %t.out`
and it caused some tests to write to the same default `a.exe` and fail
with error like:
```
LINK : fatal error LNK1104: cannot open file 'a.exe'
```

To resolve this, switch most of such tests to use `-fsyntax-only` or
explicitly added `-o %t.out` to prevent possible races.

"bad" tests were discovered using `grep -rnP 'clangxx(?!.*\-o)' sycl/test/`
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner December 26, 2022 11:10
@@ -1,4 +1,4 @@
// RUN: %clangxx -fsycl -c -fno-color-diagnostics -Xclang -fdump-record-layouts %s | FileCheck %s
// RUN: %clangxx -fsycl -c -fno-color-diagnostics -Xclang -fdump-record-layouts %s -o %t.out | FileCheck %s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: most of abi tests started to fail with -fsyntax-only because output from clang was different. I haven't investigated it and simply decided to switch to -o %t.out instead.

@@ -1,4 +1,4 @@
// RUN: %clangxx -fsycl -c %s
// RUN: %clangxx -fsycl -fsyntax-only %s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: -c still creates an output file, which is a fat object file in case of -fsycl compilation and that temporary file may still cause races. Therefore, I switched -c tests to -fsyntax-only: those tests do not check LLVM IR anyway and -fsyntax-only doesn't create output files, which should make them a tiny bit faster (less I/O)

@AlexeySachkov AlexeySachkov temporarily deployed to aws December 26, 2022 11:26 — with GitHub Actions Inactive
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

LGTM! Can we also update some doc regarding writing the tests and put the following requirements to it? Can be done in the separate patch.

@bader
Copy link
Contributor

bader commented Dec 27, 2022

LGTM! Can we also update some doc regarding writing the tests and put the following requirements to it? Can be done in the separate patch.

In addition to that we need an automated check that all tests comply with these requirements.

"bad" tests were discovered using grep -rnP 'clangxx(?!.*\-o)' sycl/test/

This seems to be a good starting point, but we need to allow -fsyntax-only and probably other alternatives to -o. Right?

@AlexeySachkov
Copy link
Contributor Author

Can we also update some doc regarding writing the tests and put the following requirements to it? Can be done in the separate patch.

Yes, I will prepare some "Testing guidelines" document

In addition to that we need an automated check that all tests comply with these requirements.
"bad" tests were discovered using grep -rnP 'clangxx(?!.*\-o)' sycl/test/
This seems to be a good starting point, but we need to allow -fsyntax-only and probably other alternatives to -o. Right?

Yeah, that would be a good idea. I will try to come up with some script for that.

@AlexeySachkov
Copy link
Contributor Author

Failure in unit-tests is a known flaky issue, which is being addressed in #7855

@AlexeySachkov AlexeySachkov merged commit 40d80c0 into intel:sycl Dec 27, 2022
@AlexeySachkov
Copy link
Contributor Author

Hi folks, I've finally prepared a PR to follow-up on testing guidelines, see #8145

@AlexeySachkov AlexeySachkov deleted the private/asachkov/fix-race-in-LIT-tests branch March 29, 2023 12:26
ldrumm pushed a commit that referenced this pull request Mar 11, 2025
Use no output file instead of default output file in LIT tests.
This occasionally caused errors on windows when multiple files compile
to the same output file. Using `-fsyntax-only` hjere also avoids running the full compiler pipeline

See #7872 for more context
adamfidel pushed a commit to reble/llvm that referenced this pull request Mar 11, 2025
Use no output file instead of default output file in LIT tests.
This occasionally caused errors on windows when multiple files compile
to the same output file. Using `-fsyntax-only` hjere also avoids running the full compiler pipeline

See intel#7872 for more context
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Apr 2, 2025
Use no output file instead of default output file in LIT tests.
This occasionally caused errors on windows when multiple files compile
to the same output file. Using `-fsyntax-only` hjere also avoids running the full compiler pipeline

See intel#7872 for more context
AlexeySachkov added a commit that referenced this pull request Apr 3, 2025
This is a joined cherry-pick of #17367, #17362 and
#17746 intended to fix a few remaining issues we see in 6.1
nightly validation run.

Descriptions of cherry-picked commits:

---

[SYCL] Disable launch_policy_neg.cpp on Windows (#17367)

Fails to compile on new MSVC.

Patch-by: Sarnie, Nick <nick.sarnie@intel.com>

---

[SYCL] Avoid testing race condition

Use no output file instead of default output file in LIT tests.
This occasionally caused errors on windows when multiple files compile
to the same output file. Using `-fsyntax-only` hjere also avoids running
the full compiler pipeline

See #7872 for more context

Patch-By: David Garcia Orozco <david.garcia.orozco@intel.com>

---

[SYCL] Fix clang-linker-wrapper Windows test (#17746)

Failing now because I recently added `ptxas` to the path on these
runners.

Patch-by: Sarnie, Nick <nick.sarnie@intel.com>

---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants