Skip to content

[compiler-rt][test] Rewrote test to remove curly braces #105696

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

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

connieyzhu
Copy link
Contributor

@connieyzhu connieyzhu commented Aug 22, 2024

This patch removes curly braces from a test, as lit's internal shell implementation does not support curly brace syntax.

Fixes #102382.

@llvmbot
Copy link
Member

llvmbot commented Aug 22, 2024

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

Author: Connie Zhu (connieyzhu)

Changes

This patch removes curly braces from a test, as lit's internal shell implementation does not support curly brace syntax.


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

1 Files Affected:

  • (modified) compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c (+1-1)
diff --git a/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c b/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
index 05b55f9e3fcc28..f17d2c9324d219 100644
--- a/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
+++ b/compiler-rt/test/asan/TestCases/Linux/dlopen-mixed-c-cxx.c
@@ -1,7 +1,7 @@
 // RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
 // RUN: %clang_asan %s -o %t.out -ldl
 //
-// RUN: { env ASAN_OPTIONS=verbosity=1 %t.out %t.so || : ; } 2>&1 | FileCheck %s
+// RUN: env ASAN_OPTIONS=verbosity=1 %t.out %t.so 2>&1 | FileCheck %s || : 
 //
 // CHECK: AddressSanitizer: failed to intercept '__cxa_throw'
 //

Copy link

github-actions bot commented Aug 22, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

This patch removes curly braces from a test, as lit's internal shell
implementation does not support curly brace syntax.
@@ -1,7 +1,7 @@
// RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
// RUN: %clang_asan %s -o %t.out -ldl
//
// RUN: { env ASAN_OPTIONS=verbosity=1 %t.out %t.so || : ; } 2>&1 | FileCheck %s
// RUN: env ASAN_OPTIONS=verbosity=1 %t.out %t.so 2>&1 | FileCheck %s || :
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these are equivalent, and we probably don't want to change the return code for FileCheck, since we still want CHECK lines to trigger a failure. It seems that this program will always return a non-zero exit code, so maybe we can just use not instead of || :? Alternatively, we could make the program return 0, but I'm guessing the author were worried about some optimization preventing the test from working correctly, and I'm not familar enough w/ this test to know if that would be problematic.

@connieyzhu connieyzhu force-pushed the compiler-rt-curly-brace-test branch from bf13596 to 011ddea Compare August 22, 2024 17:20
@ilovepi ilovepi requested review from fmayer and vitalybuka August 22, 2024 17:20
This patch fixes incorrect usage of '|| :', which modified the function
of the original test. '|| :' should be used with the env command, and
not with FileCheck.
// RUN: { env ASAN_OPTIONS=verbosity=1 %t.out %t.so || : ; } 2>&1 | FileCheck %s
// The program can potentially return a non-zero exit code, so use || : to
// ensure command returns true
// RUN: env ASAN_OPTIONS=verbosity=1 %t.out %t.so &> %t_env || :
Copy link
Member

Choose a reason for hiding this comment

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

The initial version of this commit used not: cbd28cd but for some reason that was changed in a30a4a3

Maybe removing not was used to work around the missing UNSUPPORTED: that was only added in 307daa7 CC: @serge-sans-paille @hahnjo

I'd be tempted to restore this to using not instead of || : and see if the builders are happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced ||: with not in my most recent commit. The tests are passing when I run things locally - still need to wait for builders though.

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

Comment needs updating otherwise LGTM

@@ -1,7 +1,9 @@
// RUN: %clangxx_asan -xc++ -shared -fPIC -o %t.so - < %s
// RUN: %clang_asan %s -o %t.out -ldl
//
// RUN: { env ASAN_OPTIONS=verbosity=1 %t.out %t.so || : ; } 2>&1 | FileCheck %s
// The program can potentially return a non-zero exit code, so use || : to
Copy link
Member

Choose a reason for hiding this comment

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

Stale comment here.

@connieyzhu connieyzhu merged commit b978bcc into llvm:main Aug 28, 2024
7 checks passed
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.

[llvm-lit] lit internal shell failing to parse and execute curly brace syntax
5 participants