Skip to content

[llvm-lit] Fix TypeError string argument expected in lit's internal shell #105925

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[llvm-lit] Fix TypeError string argument expected in lit's internal shell #105925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions llvm/utils/lit/lit/TestRunner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,8 @@ def expand_glob(arg, cwd):


def expand_glob_expressions(args, cwd):
result = [args[0]]
for arg in args[1:]:
result = []
for arg in args:
result.extend(expand_glob(arg, cwd))
return result

Expand Down Expand Up @@ -776,8 +776,8 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
# FIXME: Standardize on the builtin echo implementation. We can use a
# temporary file to sidestep blocking pipe write issues.

# Ensure args[0] is hashable.
args[0] = expand_glob(args[0], cmd_shenv.cwd)[0]
# Expand all glob expressions
args = expand_glob_expressions(args, cmd_shenv.cwd)
Copy link
Member

Choose a reason for hiding this comment

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

With this change the call to expand_glob_expressions in executeBuiltinMkdir and executeBuiltinRm() can also be removed.

It would probably also be good to add a test that checks something like the following

# RUN: touch %t-first.txt %t-second.txt
# RUN: ls %t-*.txt
# RUN: rm -v %t-*.txt
# Check that both files were deleted.
# RUN: test ! -e %t-first.txt 
# RUN: test ! -e %t-second.txt

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 initially didn’t remove the call to expand_glob_expressions in executeBuiltinMkdir and executeBuiltinRm because removing it was causing an error. Here's the error I encountered:

  File "/usr/local/google/home/harinidonthula/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 467, in executeBuiltinRm
    opts, args = getopt.gnu_getopt(args, "frR", ["--recursive"])

UnboundLocalError: cannot access local variable 'args' where it is not associated with a value

This error occurs because args wasn't being initialized correctly after I removed the expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:] line from these functions. The issue is that the functions still expect args to be expanded and processed, but when I rely solely on the expansion happening in _executeShCmd, the arguments aren't passed in the correct form to these built-in functions.

Specifically, in the context of commands like mkdir and rm, args[0] represents the command itself (e.g., mkdir or rm), and this should not be expanded. Expanding args[0] could cause the command to be mistakenly treated as a file or directory to expand, which would break the logic. Therefore, only args[1:] (the options or arguments) should undergo glob expansion. For example, in a command like mkdir -p folder*, we only want folder* to be expanded, not the command mkdir itself. Correct me if I am wrong, but that is what I was getting.

In short, removing expand_glob_expressions from these functions without ensuring proper argument handling causes the built-in commands to fail when trying to access args later in the flow, especially since args[0] should not be expanded.

I'm also working on writing the other test. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see expanding args[0] being a problem - I just tested with zsh and it just expands all globs:

❯ echo -e '#!/bin/sh\necho script executed' > script.sh && chmod +x script.sh
❯ ./sc*
script executed
❯ sc*
Command script.sh not found

Copy link
Member

Choose a reason for hiding this comment

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

And yes you will have to make additional changes to the builtinMkdir and builtinRM:
Instead of args (which came from the line args = expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:] you just use cmd.args. args won't magically appear in this function if you remove the line that assigns to the variable.


inproc_builtin = inproc_builtins.get(args[0], None)
if inproc_builtin and (args[0] != "echo" or len(cmd.commands) == 1):
Expand Down Expand Up @@ -875,9 +875,6 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
named_temp_files.append(f.name)
args[i] = arg.replace(kDevNull, f.name)

# Expand all glob expressions
args = expand_glob_expressions(args, cmd_shenv.cwd)

# On Windows, do our own command line quoting for better compatibility
# with some core utility distributions.
if kIsWindows:
Expand Down
3 changes: 1 addition & 2 deletions llvm/utils/lit/tests/shtest-glob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
#
# END.

# CHECK: UNRESOLVED: shtest-glob :: glob-echo.txt ({{[^)]*}})
# CHECK: TypeError: string argument expected, got 'GlobItem'
# CHECK: PASS: shtest-glob :: glob-echo.txt ({{[^)]*}})

# CHECK: FAIL: shtest-glob :: glob-mkdir.txt ({{[^)]*}})
# CHECK: # error: command failed with exit status: 1
Loading