[llvm-lit][test] Resolved typo in raising InternalShellError for export command in lit's internal shell #105961

connieyzhu · 2024-08-24T22:15:29Z

This patch fixes the incorrect usage of lit's built-in export command. There is a typo in raising the error itself where the error being raised had the wrong number of parameters passed in.

Fixes #102386.

command This patch fixes a typo in lit's built-in export command, which returns an InternalShellError when more than one argument is given to the export. The InternalShellError was missing the cmd parameter when raised, leading to issues when a test using export fails in high-address-dereference.c in compiler-rt.

This patch fixes the incorrect usage of lit's built-in export command in this compiler-rt test. The export command only allows for one argument, but %env_asan_opts expands to "env ASAN_OPTIONS", which gives export two arguments instead of only one. Instead of using export, which propogates the environment variable value to all subsequent commands, the environment variable is manually set before all RUN lines following the original export line.

llvmbot · 2024-08-24T22:15:59Z

@llvm/pr-subscribers-testing-tools

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

Author: Connie Zhu (connieyzhu)

Changes

This patch fixes the incorrect usage of lit's built-in export command.

First, a typo in raising the error itself is resolved. The error being raised had the wrong number of parameters passed in.

Then, the export command was given the wrong number of arguments, as %env_asan_opts expands to env ASAN_OPTIONS, giving export two arguments instead of the expected one argument. To avoid this, the use of export is completely removed, and the setting of the environment variable ASAN_OPTIONS is added before all RUN lines following the initial export RUN line. This is essentially the same as using export, which propagates the environment variable value to all subsequent commands.

Fixes #102386.

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

2 Files Affected:

(modified) compiler-rt/test/asan/TestCases/Posix/high-address-dereference.c (+5-6)
(modified) llvm/utils/lit/lit/TestRunner.py (+1-1)

diff --git a/compiler-rt/test/asan/TestCases/Posix/high-address-dereference.c b/compiler-rt/test/asan/TestCases/Posix/high-address-dereference.c
index 845e126d3f89b9..37a7b1702e982e 100644
--- a/compiler-rt/test/asan/TestCases/Posix/high-address-dereference.c
+++ b/compiler-rt/test/asan/TestCases/Posix/high-address-dereference.c
@@ -5,12 +5,11 @@
 
 // REQUIRES: x86_64-target-arch
 // RUN: %clang_asan %s -o %t
-// RUN: export %env_asan_opts=print_scariness=1
-// RUN: not %run %t 0x0000000000000000 2>&1 | FileCheck %s --check-prefixes=ZERO,HINT-PAGE0
-// RUN: not %run %t 0x0000000000000FFF 2>&1 | FileCheck %s --check-prefixes=LOW1,HINT-PAGE0
-// RUN: not %run %t 0x0000000000001000 2>&1 | FileCheck %s --check-prefixes=LOW2,HINT-NONE
-// RUN: not %run %t 0x4141414141414141 2>&1 | FileCheck %s --check-prefixes=HIGH,HINT-HIGHADDR
-// RUN: not %run %t 0xFFFFFFFFFFFFFFFF 2>&1 | FileCheck %s --check-prefixes=MAX,HINT-HIGHADDR
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x0000000000000000 2>&1 | FileCheck %s --check-prefixes=ZERO,HINT-PAGE0
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x0000000000000FFF 2>&1 | FileCheck %s --check-prefixes=LOW1,HINT-PAGE0
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x0000000000001000 2>&1 | FileCheck %s --check-prefixes=LOW2,HINT-NONE
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x4141414141414141 2>&1 | FileCheck %s --check-prefixes=HIGH,HINT-HIGHADDR
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0xFFFFFFFFFFFFFFFF 2>&1 | FileCheck %s --check-prefixes=MAX,HINT-HIGHADDR
 
 #include <stdint.h>
 #include <stdlib.h>
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index da7fa86fd39173..63ab5e4c183494 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -356,7 +356,7 @@ def executeBuiltinPopd(cmd, shenv):
 def executeBuiltinExport(cmd, shenv):
     """executeBuiltinExport - Set an environment variable."""
     if len(cmd.args) != 2:
-        raise InternalShellError("'export' supports only one argument")
+        raise InternalShellError(cmd, "'export' supports only one argument")
     updateEnv(shenv, cmd.args)
     return ShellCommandResult(cmd, "", "", 0, False)

ilovepi

I'm fine w/ the code change in TestRunner.py, but

this needs a test in lit's test suite
the compiler-rt change should be split out on its own

llvm/utils/lit/lit/TestRunner.py

ilovepi · 2024-08-26T19:07:13Z

compiler-rt/test/asan/TestCases/Posix/high-address-dereference.c

-// RUN: export %env_asan_opts=print_scariness=1
-// RUN: not %run %t 0x0000000000000000 2>&1 | FileCheck %s --check-prefixes=ZERO,HINT-PAGE0
-// RUN: not %run %t 0x0000000000000FFF 2>&1 | FileCheck %s --check-prefixes=LOW1,HINT-PAGE0
-// RUN: not %run %t 0x0000000000001000 2>&1 | FileCheck %s --check-prefixes=LOW2,HINT-NONE
-// RUN: not %run %t 0x4141414141414141 2>&1 | FileCheck %s --check-prefixes=HIGH,HINT-HIGHADDR
-// RUN: not %run %t 0xFFFFFFFFFFFFFFFF 2>&1 | FileCheck %s --check-prefixes=MAX,HINT-HIGHADDR
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x0000000000000000 2>&1 | FileCheck %s --check-prefixes=ZERO,HINT-PAGE0
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x0000000000000FFF 2>&1 | FileCheck %s --check-prefixes=LOW1,HINT-PAGE0
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x0000000000001000 2>&1 | FileCheck %s --check-prefixes=LOW2,HINT-NONE
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0x4141414141414141 2>&1 | FileCheck %s --check-prefixes=HIGH,HINT-HIGHADDR
+// RUN: %env_asan_opts=print_scariness=1 not %run %t 0xFFFFFFFFFFFFFFFF 2>&1 | FileCheck %s --check-prefixes=MAX,HINT-HIGHADDR


This seems unrelated to this patch, is there a reason its included here?

I included it because it was related to the RUN line that caused the InternalShellError. I will split it out into it's own patch.

I think that's preferable.

This patch creates a test to show the failing behavior for lit's built-in export command when given more than one argument.

ilovepi

LGTM.

…nts (#106143) This patch fixes the incorrect usage of lit's built-in export command in this compiler-rt test. The export command only allows for one argument, but %env_asan_opts expands to "env ASAN_OPTIONS", which gives export two arguments instead of only one. Instead of using export, which propogates the environment variable value to all subsequent commands, the environment variable is manually set before all RUN lines following the original export line. This patch depends on #105961. Fixes #106142.

connieyzhu added 2 commits August 24, 2024 21:53

llvmbot added compiler-rt compiler-rt:asan Address sanitizer llvm-lit compiler-rt:sanitizer testing-tools labels Aug 24, 2024

connieyzhu requested review from petrhosek and ilovepi August 24, 2024 22:16

ilovepi requested changes Aug 26, 2024

View reviewed changes

[llvm-lit][test] Created test for built-in export with too many args

c83d7e0

This patch creates a test to show the failing behavior for lit's built-in export command when given more than one argument.

ilovepi approved these changes Aug 26, 2024

View reviewed changes

connieyzhu changed the title ~~[compiler-rt][test] Resolved export command failure in test run with lit's internal shell~~ [llvm-lit][test] Resolved typo in raising InternalShellError for export command in lit's internal shell Aug 26, 2024

connieyzhu mentioned this pull request Aug 26, 2024

[compiler-rt][test] Rewrote test to fix usage of export with 2 arguments #106143

Merged

connieyzhu merged commit 1990d8d into llvm:main Aug 27, 2024
8 checks passed

connieyzhu deleted the compiler-rt-InternalShellError branch August 27, 2024 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[llvm-lit][test] Resolved typo in raising InternalShellError for export command in lit's internal shell #105961

[llvm-lit][test] Resolved typo in raising InternalShellError for export command in lit's internal shell #105961

connieyzhu commented Aug 24, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 24, 2024 •

edited

Loading

Uh oh!

ilovepi left a comment

Uh oh!

Uh oh!

ilovepi Aug 26, 2024

Uh oh!

connieyzhu Aug 26, 2024 •

edited

Loading

Uh oh!

ilovepi Aug 26, 2024

Uh oh!

ilovepi left a comment

Uh oh!

Uh oh!

Uh oh!

[llvm-lit][test] Resolved typo in raising InternalShellError for export command in lit's internal shell #105961

[llvm-lit][test] Resolved typo in raising InternalShellError for export command in lit's internal shell #105961

Conversation

connieyzhu commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilovepi Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

connieyzhu Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilovepi Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

connieyzhu commented Aug 24, 2024 •

edited

Loading

llvmbot commented Aug 24, 2024 •

edited

Loading

connieyzhu Aug 26, 2024 •

edited

Loading