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

Augment uncaught-exception.test fuzzer test to be msvc-compatible #125924

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davidmrdavid
Copy link
Contributor

Today, the uncaught-exception.test fuzzer test checks for the string "libFuzzer: deadly signal" in the program output as the result of an uncaught exception.

Although this is correct for clang, msvc reports a different error message: "libFuzzer: uncaught C++ exception". Since msvc reuses the libFuzzer infrastructure for ASan regression testing, it would help us greatly if the test handled the msvc divergence more gracefully.

This PR: augments this test so check for a different string (namely "libFuzzer: uncaught C++ exception") if the compiler target matches the msvc naming scheme.

I understand if this is outside the scope of support for LLVM as well, and I'm also open for different approaches here. Thanks!

@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: David Justo (davidmrdavid)

Changes

Today, the uncaught-exception.test fuzzer test checks for the string "libFuzzer: deadly signal" in the program output as the result of an uncaught exception.

Although this is correct for clang, msvc reports a different error message: "libFuzzer: uncaught C++ exception". Since msvc reuses the libFuzzer infrastructure for ASan regression testing, it would help us greatly if the test handled the msvc divergence more gracefully.

This PR: augments this test so check for a different string (namely "libFuzzer: uncaught C++ exception") if the compiler target matches the msvc naming scheme.

I understand if this is outside the scope of support for LLVM as well, and I'm also open for different approaches here. Thanks!


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

1 Files Affected:

  • (modified) compiler-rt/test/fuzzer/uncaught-exception.test (+6-3)
diff --git a/compiler-rt/test/fuzzer/uncaught-exception.test b/compiler-rt/test/fuzzer/uncaught-exception.test
index b055c88f6d9039..d1b98cfb7c74ba 100644
--- a/compiler-rt/test/fuzzer/uncaught-exception.test
+++ b/compiler-rt/test/fuzzer/uncaught-exception.test
@@ -4,7 +4,10 @@
 REQUIRES: windows
 RUN: %cpp_compiler %S/UncaughtException.cpp -o %t-UncaughtException
 
-RUN: not %run %t-UncaughtException 2>&1 | FileCheck %s
+# Clang will fail the test with 'deadly signal', but other compilers may fail with different error messages.
+# For example, msvc fails with 'uncaught C++ exception'. So the error we check depends on the compiler target.
+RUN: not %run %t-UncaughtException 2>&1 | FileCheck %s --check-prefixes=CHECK-CRASH,%if target={{.*-windows-msvc.*}} %{CHECK-MSVC%} %else %{CHECK-ERROR%}
 
-CHECK: ERROR: libFuzzer: deadly signal
-CHECK: Test unit written to ./crash
+CHECK-ERROR: ERROR: libFuzzer: deadly signal
+CHECK-MSVC: ERROR: libFuzzer: uncaught C++ exception
+CHECK-CRASH: Test unit written to ./crash

@davidmrdavid
Copy link
Contributor Author

@vitalybuka - any chance you could take a look at this one as well (please let me know if I'm not following process here)? I noticed no one got added as a reviewer. Thank you!

@davidmrdavid
Copy link
Contributor Author

A friendly ping here again :)

@Enna1 Enna1 requested a review from vitalybuka February 20, 2025 03:14
@davidmrdavid
Copy link
Contributor Author

Thanks for the approval!
I don't have merge commit access (though I'd love to work towards that, since I'm looking to upstream more of our changes), so I can't merge this myself :) .

@davidmrdavid
Copy link
Contributor Author

@vitalybuka - are you able to help me merge this? I notice it's approved, but I don't have commit permissions (though I'd love to gain them eventually).

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

Successfully merging this pull request may close these issues.

3 participants