Skip to content

[compiler-rt][tests] Removed the use of parentheses in compiler-rt tests with lit internal shell #105729

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 3 commits into from
Aug 27, 2024

Conversation

Harini0924
Copy link
Contributor

This patch addresses compatibility issues with the lit internal shell by removing the use of subshell execution (parentheses and subshell syntax) in the merge-posix.test and vptr.cpp tests. The lit internal shell does not support parentheses, so the tests have been refactored to use separate command invocations.

This change is relevant for enabling the lit internal shell by default, as outlined in [RFC] Enabling the Lit Internal Shell by Default

fixes: #102401

Updated `merge-posix.test` and `vptr.cpp` to remove the use of parentheses for subshell execution in the RUN lines, ensuring compatibility with the lit internal shell.
@llvmbot
Copy link
Member

llvmbot commented Aug 22, 2024

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

Author: None (Harini0924)

Changes

This patch addresses compatibility issues with the lit internal shell by removing the use of subshell execution (parentheses and subshell syntax) in the merge-posix.test and vptr.cpp tests. The lit internal shell does not support parentheses, so the tests have been refactored to use separate command invocations.

This change is relevant for enabling the lit internal shell by default, as outlined in [RFC] Enabling the Lit Internal Shell by Default

fixes: #102401


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

2 Files Affected:

  • (modified) compiler-rt/test/fuzzer/merge-posix.test (+2-1)
  • (modified) compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp (+3-1)
diff --git a/compiler-rt/test/fuzzer/merge-posix.test b/compiler-rt/test/fuzzer/merge-posix.test
index 9fece647ca60b9..597e2dd3c2791f 100644
--- a/compiler-rt/test/fuzzer/merge-posix.test
+++ b/compiler-rt/test/fuzzer/merge-posix.test
@@ -17,7 +17,8 @@ RUN: echo ....E. > %tmp/T2/5
 RUN: echo .....R > %tmp/T2/6
 
 # Check that we can report an error if file size exceeded
-RUN: (ulimit -f 1; not %run %t-FullCoverageSetTest -merge=1 %tmp/T1 %tmp/T2 2>&1 | FileCheck %s --check-prefix=SIGXFSZ)
+RUN: ulimit -f 1
+RUN: not %run %t-FullCoverageSetTest -merge=1 %tmp/T1 %tmp/T2 2>&1 | FileCheck %s --check-prefix=SIGXFSZ
 SIGXFSZ: ERROR: libFuzzer: file size exceeded
 
 # Check that we honor TMPDIR
diff --git a/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp b/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp
index 2d0e48cd84f3cf..b1242197caf75c 100644
--- a/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp
+++ b/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp
@@ -23,7 +23,9 @@
 // RUN: %env_ubsan_opts=halt_on_error=1 not %run %t nN 2>&1 | FileCheck %s --check-prefix=CHECK-NULL-MEMFUN --strict-whitespace
 // RUN: %env_ubsan_opts=print_stacktrace=1 %run %t dT 2>&1 | FileCheck %s --check-prefix=CHECK-DYNAMIC --allow-unused-prefixes --check-prefix=CHECK-%os-DYNAMIC --strict-whitespace
 
-// RUN: (echo "vptr_check:S"; echo "vptr_check:T"; echo "vptr_check:U") > %t.supp
+// RUN: echo "vptr_check:S" > %t.supp
+// RUN: echo "vptr_check:T" >> %t.supp
+// RUN: echo "vptr_check:U" >> %t.supp
 // RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t mS
 // RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t fS
 // RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t cS

@Harini0924 Harini0924 changed the title [compiler-rt][tests] Remove unsupported subshell execution in compiler-rt tests [compiler-rt][tests] Removed the use of parentheses in compiler-rt tests with lit internal shell Aug 22, 2024
@Harini0924
Copy link
Contributor Author

CC: @ilovepi @petrhosek

Copy link
Contributor

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

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

Other than addressing my one comment the rest of the change seems fine to me.

@ilovepi ilovepi requested a review from fmayer August 22, 2024 21:03
Simplified the suppression file creation in the test script by combining multiple RUN lines into a single echo command using the -e option. This change reduces redundancy, improves readability, and makes the script more efficient by creating the suppression file with all necessary entries in one step.
@Harini0924 Harini0924 merged commit d28c0fb into llvm:main Aug 27, 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 fail to execute parentheses syntax
4 participants