[lit] Fix to make "RUN: env PATH=..." work as intended #165308

bjope · 2025-10-27T19:55:57Z

There was a bug in llvm-lit related to setting PATH using env in the internal shell.

The new PATH wasn't used when looking up the command to be executed. So when doing things like this in a test case
RUN: mkdir %t
RUN: env PATH=%t program ...
the internal shell would search for "program" using the orignal PATH and not the PATH set by env when preceeding the command.

It seems like this was a simple mistake in commit 57782ef, since the logic to pick a PATH from the cmd_shenv instead of shenv actually was added in that patch, but the resulting path wasn't used.

There was a bug in llvm-lit related to setting PATH using env in the internal shell. The new PATH wasn't used when looking up the command to be executed. So when doing things like this in a test case RUN: mkdir %t RUN: env PATH=%t program ... the internal shell would search for "program" using the orignal PATH and not the PATH set by env when preceeding the command. It seems like this was a simple mistake in commit 57782ef, since the logic to pick a PATH from the cmd_shenv instead of shenv actually was added in that patch, but the resulting path wasn't used.

llvmbot · 2025-10-27T19:56:34Z

@llvm/pr-subscribers-testing-tools

Author: Björn Pettersson (bjope)

Changes

There was a bug in llvm-lit related to setting PATH using env in the internal shell.

The new PATH wasn't used when looking up the command to be executed. So when doing things like this in a test case
RUN: mkdir %t
RUN: env PATH=%t program ...
the internal shell would search for "program" using the orignal PATH and not the PATH set by env when preceeding the command.

It seems like this was a simple mistake in commit 57782ef, since the logic to pick a PATH from the cmd_shenv instead of shenv actually was added in that patch, but the resulting path wasn't used.

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

1 Files Affected:

(modified) llvm/utils/lit/lit/TestRunner.py (+1-1)

diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index f88314547bb3f..9fba96a1471a0 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -945,7 +945,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
             path = (
                 cmd_shenv.env["PATH"] if "PATH" in cmd_shenv.env else shenv.env["PATH"]
             )
-            executable = lit.util.which(args[0], shenv.env["PATH"])
+            executable = lit.util.which(args[0], path)
         if not executable:
             raise InternalShellError(j, "%r: command not found" % args[0])

bjope · 2025-10-27T19:59:36Z

I spent some time trying to figure out how to make a nice regression test for this. But I'm not quite sure how to.
I was aiming at adding something in utils/lit/tests, that would change the PATH to point at some Input directory with a script that it would execute. But I think it gets a bit complicated as I would need to make sure that would work on every host system etc (or find ways to limit it to Linux or something).

boomanaiden154

(or find ways to limit it to Linux or something).

I would try to see if it "just works", at least on premerge CI. If something breaks, you can split it out into a separate test file (see the shtest-ulimit-nondarwin.py/shtest-ulimit.py for an example of the split).

arichardson · 2025-10-27T20:33:04Z

llvm/utils/lit/lit/TestRunner.py

            # Use the path from cmd_shenv by default, but if the environment variable
            # is unset (like if the user is using env -i), use the standard path.
            path = (
                cmd_shenv.env["PATH"] if "PATH" in cmd_shenv.env else shenv.env["PATH"]


I'm not sure using the parent env is correct, if PATH is not set it looks like bash and zsh use a default value.

env -i bash -c 'echo $PATH; printenv' /usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin:. PWD=/usr/local/google/home/alexrichardson SHLVL=0 _=/usr/bin/printenv

https://unix.stackexchange.com/a/479083

Well, I also thought that it looked a bit weird to use the parent env in case of env -i.

The goal with this patch is to fix the case when PATH actually is set to something, and then use the correct env when looking up the executable. So I haven't really considered the scenario when PATH is unset in cmd_shenv.

Yeah I agree that handling the PATH is unset case in the same way as a "real" shell shouldn't matter too much - I was just curious to see what happens if you do that. Using the parent env instead of some shell-dependent hardcoded value seems fine to me (or we just use the bash value of /usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin:.).

arichardson · 2025-10-28T09:16:08Z

llvm/utils/lit/tests/shtest-env-path.py


 # The test is using /bin/sh. Limit to system known to have /bin/sh.
-# REQUIRED: system-linux
+# REQUIRES: system-linux


We have quite a few tests that have this requires even though they should also work on other systems such as freebsd. Should it be the following instead?

Suggested change

# REQUIRES: system-linux

# UNSUPPORTED: system-windows

I felt a bit uncomfortable with using UNSUPPORTED as I do not know how many different systems there are. So I enabled it for the only system I can test on myself and made sure the pre-commit tests would pass.
I could of course enable it speculatively and then someone need to update this when buildbots starts failing, although I won't really be able to do that for the rest of this week since I'll be out-of-office for some days.

Is perhaps REQUIRES: shell more appropriate?

Is perhaps REQUIRES: shell more appropriate?

Sorry. That is about using the lit internal shell or not. So that wouldn't fit for checking if /bin/sh exists.

Ah this is not about the behaviour of PATH but rather because the script needs a shell interpreter. I think we could make this work on windows too by using test.py instead of test.sh

arichardson

It would be great to run this test on all platforms but otherwise all looks good

arichardson · 2025-10-28T11:54:20Z

llvm/utils/lit/tests/Inputs/shtest-env-path/test.sh

@@ -0,0 +1,4 @@
+#!/bin/sh


Would making this a python script allow it to work on windows and avoid the /bin/sh dependency? This should be testable in pre-commit ci so you wouldn't need a windows system.

I thought about using python, but couldn't find a good way to make it work. The only executable that is in PATH is test.py itself, so python isn't in the PATH. That is why I though /bin/sh could be assumed to always exist in supported linux installations.

I thought about using python, but couldn't find a good way to make it work. The only executable that is in PATH is test.py itself, so python isn't in the PATH. That is why I though /bin/sh could be assumed to always exist in supported linux installations.

Ah yes that is awkward, we'd have to generate a script with #!{sys.executable} but that might not work on windows. So probably best to keep as-is

llvm-ci · 2025-10-28T13:44:03Z

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/29879

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll' FAILED ********************
Exit Code: 2

Command Output (stdout):
--
# RUN: at line 1
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
# .---command stderr------------
# | PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace and instructions to reproduce the bug.
# | Stack dump:
# | 0.	Program arguments: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli -jit-kind=orc-lazy -compile-threads=2 -thread-entry hello /Users/buildbot/buildbot-root/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
# |  #0 0x0000000101804768 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100f30768)
# |  #1 0x0000000101802518 llvm::sys::RunSignalHandlers() (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100f2e518)
# |  #2 0x0000000101805268 SignalHandler(int, __siginfo*, void*) (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100f31268)
# |  #3 0x000000019bd2f584 (/usr/lib/system/libsystem_platform.dylib+0x18047b584)
# |  #4 0x00000101013590d8
# |  #5 0x0000000101362e34 llvm::orc::ExecutionSession::removeJITDylibs(std::__1::vector<llvm::IntrusiveRefCntPtr<llvm::orc::JITDylib>, std::__1::allocator<llvm::IntrusiveRefCntPtr<llvm::orc::JITDylib>>>) (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100a8ee34)
# |  #6 0x0000000101362be4 llvm::orc::ExecutionSession::endSession() (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100a8ebe4)
# |  #7 0x00000001013ef85c llvm::orc::LLJIT::~LLJIT() (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100b1b85c)
# |  #8 0x00000001013f41e8 llvm::orc::LLLazyJIT::~LLLazyJIT() (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100b201e8)
# |  #9 0x00000001008dca2c runOrcJIT(char const*) (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100008a2c)
# | #10 0x00000001008d7f18 main (/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/lli+0x100003f18)
# | #11 0x000000019b973154
# `-----------------------------
# error: command failed with exit status: -11
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
# .---command stderr------------
# | FileCheck error: '<stdin>' is empty.
# | FileCheck command line:  /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/llvm-project/llvm/test/ExecutionEngine/OrcLazy/multiple-compile-threads-basic.ll
# `-----------------------------
# error: command failed with exit status: 2

--

********************

There was a bug in llvm-lit related to setting PATH using env in the internal shell. The new PATH wasn't used when looking up the command to be executed. So when doing things like this in a test case RUN: mkdir %t RUN: env PATH=%t program ... the internal shell would search for "program" using the orignal PATH and not the PATH set by env when preceeding the command. It seems like this was a simple mistake in commit 57782ef, since the logic to pick a PATH from the cmd_shenv instead of shenv actually was added in that patch, but the resulting path wasn't used.

bjope requested a review from boomanaiden154 October 27, 2025 19:55

llvmbot added llvm-lit testing-tools labels Oct 27, 2025

boomanaiden154 reviewed Oct 27, 2025

View reviewed changes

arichardson reviewed Oct 27, 2025

View reviewed changes

bjope added 2 commits October 27, 2025 23:09

Fixup: Add a regression test

6722c4b

Fix typo: REQUIRED -> REQUIRES

dec56d7

bjope requested a review from boomanaiden154 October 28, 2025 08:48

arichardson reviewed Oct 28, 2025

View reviewed changes

arichardson approved these changes Oct 28, 2025

View reviewed changes

bjope merged commit 44cb8c1 into llvm:main Oct 28, 2025
9 of 10 checks passed

[lit] Fix to make "RUN: env PATH=..." work as intended #165308

[lit] Fix to make "RUN: env PATH=..." work as intended #165308

Uh oh!

Conversation

bjope commented Oct 27, 2025

Uh oh!

llvmbot commented Oct 27, 2025

Uh oh!

bjope commented Oct 27, 2025

Uh oh!

boomanaiden154 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants