[llvm-lit] lit internal shell failing to parse and execute curly brace syntax

[connieyzhu]

lit’s internal shell cannot recognize and execute the {} syntax, which is used in tests across lld and compiler-rt.

Example error message:

FAIL: lld :: ELF/error-handling-script-linux.test (9 of 2939)
******************** TEST 'lld :: ELF/error-handling-script-linux.test' FAILED ********************
Exit Code: 127

Command Output (stdout):
--

# RUN: at line 21
{ echo 'a_: ret'; echo 'bar: movl a(%rip), %eax' ; } | /usr/local/google/home/connieyzhu/llvm-project/build/bin/llvm-mc -filetype=obj -triple=x86_64 - -o /usr/local/google/home/connieyzhu/llvm-project/build/tools/lld/test/ELF/Output/error-handling-script-linux.test.tmp3.o
# executed command: '{' echo 'a_: ret'
# .---command stderr------------
# | '{': command not found
# `-----------------------------
# error: command failed with exit status: 127

[llvmbot]

@llvm/issue-subscribers-lld-elf

Author: Connie (connieyzhu)

lit’s internal shell cannot recognize and execute the `{}` syntax, which is used in tests across lld and compiler-rt.

Example error message:

FAIL: lld :: ELF/error-handling-script-linux.test (9 of 2939)
******************** TEST 'lld :: ELF/error-handling-script-linux.test' FAILED ********************
Exit Code: 127

Command Output (stdout):
--

# RUN: at line 21
{ echo 'a_: ret'; echo 'bar: movl a(%rip), %eax' ; } | /usr/local/google/home/connieyzhu/llvm-project/build/bin/llvm-mc -filetype=obj -triple=x86_64 - -o /usr/local/google/home/connieyzhu/llvm-project/build/tools/lld/test/ELF/Output/error-handling-script-linux.test.tmp3.o
# executed command: '{' echo 'a_: ret'
# .---command stderr------------
# | '{': command not found
# `-----------------------------
# error: command failed with exit status: 127

[jh7370]

I don't think we should be supporting this functionality in lit. If the one test mentioned is the only test that fails without it, I think we should just update the test. Doing so would be fairly trivial and would avoid us having to maintain this feature and its corresponding tests.

[connieyzhu]

I don't think we should be supporting this functionality in lit. If the one test mentioned is the only test that fails without it, I think we should just update the test. Doing so would be fairly trivial and would avoid us having to maintain this feature and its corresponding tests.

There are 2 tests that use the curly braces functionality, so it is true that updating tests would be much more straight-forward. However, I am planning on using this implementation framework for other features as well (command substitution, for loops, parens). This makes for many more tests that would need updating, and I think it might be better to just have this functionality. What do you think?

[jh7370]

I don't think we should be supporting this functionality in lit. If the one test mentioned is the only test that fails without it, I think we should just update the test. Doing so would be fairly trivial and would avoid us having to maintain this feature and its corresponding tests.

There are 2 tests that use the curly braces functionality, so it is true that updating tests would be much more straight-forward. However, I am planning on using this implementation framework for other features as well (command substitution, for loops, parens). This makes for many more tests that would need updating, and I think it might be better to just have this functionality. What do you think?

It sounds to me like this needs a dedicated RFC on Discourse. On the surface, I'm not convinced we need some of those, and others already have prior art on how to implement using lit (see for example lit's if syntax).

[ilovepi]

I think this is part of the work already proposed in an RFC: https://discourse.llvm.org/t/rfc-enabling-the-lit-internal-shell-by-default/80179. These items were called out specifically in the implementation plans, IIRC. As I understand it, @connieyzhu expects there to be significant shared implementation work for these features, so I understand why she'd want to enable additional feature if they're only minor extensions of a core functionality.

You do bring up a good point on the if syntax as a way forward for structures like loops. It may be worth revisiting the importance/priority of the set of features if one of them can be handled w/ that kind of syntax extension.

[jh7370]

I got the impression from that thread that most people think changing tests to work with lit's current functionality is generally preferred over adding new functionality. If you read the thread differently, could I suggest that this is raised directly on the RFC. I'll post a bit more on the RFC.