[lit] Update internal shell lexer to handle LLDB persistent vars. #156125
Conversation
LLDB allows creation of 'persistent' variables, with names that start with '$'. The lit internal shell was escaping the '$', making it '\$', in some CHECK lines, which causes some LLDB tests to fail when using the lit internal shell. This PR fixes that by having the lexer remove the escape on the '$'.
|
@llvm/pr-subscribers-testing-tools Author: None (cmtice) ChangesLLDB allows creation of 'persistent' variables, with names that start with '$'. The lit internal shell was escaping the '$', making it '$', in some CHECK lines, which causes some LLDB tests to fail when using the lit internal shell. This PR fixes that by having the lexer remove the escape on the '$'. Full diff: https://github.com/llvm/llvm-project/pull/156125.diff 1 Files Affected:
diff --git a/llvm/utils/lit/lit/ShUtil.py b/llvm/utils/lit/lit/ShUtil.py
index fa13167cad1be..ff151b1e29330 100644
--- a/llvm/utils/lit/lit/ShUtil.py
+++ b/llvm/utils/lit/lit/ShUtil.py
@@ -115,6 +115,11 @@ def lex_arg_quoted(self, delim):
c = self.eat()
if c == delim:
return str
+ # LLDB uses "$" at the start of global variable names; it should
+ # not be escaped nor dropped.
+ elif c == "\\" and self.look() == "$":
+ c = self.eat()
+ str += c
elif c == "\\" and delim == '"':
# Inside a '"' quoted string, '\\' only escapes the quote
# character and backslash, otherwise it is preserved.
|
|
Is it possible to include a test for this? |
There was a problem hiding this comment.
This needs a test in llvm/utils/lit/tests.
Adding some information in the PR description on what LLDB tests are impacted by this would also be helpful. There are some existing issues related to enabling the lit internal shell in LLDB, and adding a link to those (particularly #102698, none of the others look relevant) wouldn't hurt.
I updated an existing internal shell lexer test to test for this. I also updated the PR description, giving a lot more detail. Also mentioned this PR in issue #102698. |
|
✅ With the latest revision this PR passed the Python code formatter. |
Thanks. Could you list the tests that this impacts somewhere as well? |
To the best of my knowledge it only impacts the test I mentioned in the commit message. |
|
Just making a note here in case anyone else comes across it: Certain sequences after this change need to be double escaped if they are supposed to be passed to the command escaped, like the following from an lld test: |
Is this behaviour different from POSIX sh? I think we should make sure that the internal shell has the same behaviour. I think maybe the difference we have here is that the removal of the |
| self.assertEqual(self.lex('"\\$y = 11"'), ["$y = 11"]) | ||
| self.assertEqual(self.lex('"expr \\$y = 11"'), ["expr $y = 11"]) |
There was a problem hiding this comment.
Could you add a test with single quotes?
There was a problem hiding this comment.
See #156742 for requested fix & test.
LLDB allows creation of 'persistent' variables, with names that start with '$'. The lit internal shell was escaping the '$', making it '\$', in some CHECK lines, which causes an LLDB test, TestExprWithSideEffectOnConvenienceVar, to fail when using the lit internal shell.
Further explanation of the failing LLDB test: LLDB convenience variables start with '$'. The test passes several quoted commands that use and update convenience variables to lldb as arguments to be run in batch mode. The tool that packages up the complete string and passes it to the lit internal shell lexer for lexing inserts a backslash in front of the '$' before passing the string in for lexing. The lexer was passing this change along, causing the tests to fail.
This PR fixes the issue by having the lexer remove the newly added escape on the '$'.