[CRT][LIT] split target_cflags using shlex

[jsji]

Summary

We recently enabled crt for powerpc in
https://reviews.llvm.org/rGb7611ad0b16769d3bf172e84fa9296158f8f1910.

And we started to see some unexpected error message when running
check-runtimes.

eg:
https://lab.llvm.org/buildbot/#/builders/57/builds/9488/steps/6/logs/stdio
line 100 - 103:

"
clang-14: error: unknown argument: '-m64 -fno-function-sections'
clang-14: error: unknown argument: '-m64 -fno-function-sections'
clang-14: error: unknown argument: '-m64 -fno-function-sections'
clang-14: error: unknown argument: '-m64 -fno-function-sections'
"

Looks like we shouldn't strip the space at the beginning,
or else the command line passed to subprocess won't work well.

[MaskRay]

compiler-rt/test/crt/lit.cfg.py

25

Thank you for working on this.

Do you know what are config.clang and config.target_cflags?

rstrip() works while strip() doesn't looks weird.

[jsji]

compiler-rt/test/crt/lit.cfg.py

25

The cflags are passing down from compiler.

compiler-rt/test/crt/lit.site.cfg.py.in:config.target_cflags = "@CRT_TEST_TARGET_CFLAGS@"

ompiler-rt/test/crt/CMakeLists.txt:    get_test_cc_for_arch(${arch} CRT_TEST_TARGET_CC CRT_TEST_TARGET_CFLAGS)

[jsji]

compiler-rt/test/crt/lit.cfg.py

25

the target_cflags itself is OK.

Actually , you should be able to reproduce the same problem with following:

$ cat t.py
import subprocess

target_cflags= " -m64 -v " 
 
cmd = subprocess.Popen(['clang',
                            target_cflags.strip(),
                            '-print-file-name=t'],
                           stdout=subprocess.PIPE,
                           universal_newlines=True)
$ python t.py
$ clang-14: error: unknown argument: '-m64 -v'

[MaskRay]

compiler-rt/test/crt/lit.cfg.py

25

The code appears to make use of a fragile Popen behavior.

The proper fix should be import shlex; ... ['clang', '-print-file-name=t'] + shlex.split(target_cflags)

25

Perhaps even better to make sure target_cflags is a list in the first place. I haven't investigated further.

[jsji]

Address comments.

[jsji]

compiler-rt/test/crt/lit.cfg.py

25

You are right. While it works with previous patch, it is better to use shlex for better readability and maintenance.

[MaskRay]

Thanks!

[MaskRay]

It isn't easy to make config.target_cflags a list, so shlex is good.

[phosek]

LGTM

[filcab]

Please make sure to test on Windows (or check if some windows buildbotbis
running those tests and check it out). Shlex is, IIRC, using UNIX rules to
do the split.

Thank you,
Filipe

[jsji]

In D108329#2954033, @filcab wrote:

Please make sure to test on Windows (or check if some windows buildbotbis
running those tests and check it out). Shlex is, IIRC, using UNIX rules to
do the split.

Thank you,
Filipe

Good point Filipe! Unfortunately, there are no Windows buildbot running check-runtimes as far as I can tell.

[jsji]

Avoid using shlex due to win32.

[jsji]

Switch to the way as we used in build_invocation, let me know if you have better solution. Thanks.

[filcab]

Looks ok, I’ll check it out when I can and submit a patch if needed.

Thanks!
Filipe

[MaskRay]

shlex works perfectly fine on Windows.

In D108329#2954033, @filcab wrote:

Please make sure to test on Windows (or check if some windows buildbotbis
running those tests and check it out). Shlex is, IIRC, using UNIX rules to
do the split.

Thank you,
Filipe

shlex.split works perfectly fine. We don't do crazy space and double quotes on Windows command lines. (https://stackoverflow.com/questions/33560364/python-windows-parsing-command-lines-with-shlex)

The committed version [config.target_cflags] is inferior IMO

[jsji]

shlex.split works perfectly fine. We don't do crazy space and double quotes on Windows command lines. (https://stackoverflow.com/questions/33560364/python-windows-parsing-command-lines-with-shlex)

The committed version [config.target_cflags] is inferior IMO

:), since shlex.split is using posix and the cflags are actually passing down from user input.
So there is still some chance that user use space in path or something? eg: -isystem C:\\.... Program Files \...

Anyway, I can do a following commit with something like following if it is OK for everyone.

-                           [config.target_cflags],
+                           shlex.split(config.target_cflags, posix='win' not in sys.platform),

[MaskRay]

It seems that the posix argument doesn't mean POSIX/Windows distinction. Using shlex.split only for *NIX platforms seems fine.

I suspect if sys.platform in ['win32'] is likely irrelevant here because we have

if config.host_os not in ['Linux']:
    config.unsupported = True

[jsji]

if config.host_os not in ['Linux']:
    config.unsupported = True

Good point. I committed the version using shlex.split in 337bd67d836b7cb7a18fb08df8f3a7902338c7fa.

compiler-rt/test/crt/lit.cfg.py

if use_lit_shell:
     # 0 is external, "" is default, and everything else is internal.
     execute_external = (use_lit_shell == "0")
 else:
     # Otherwise we default to internal on Windows and external elsewhere, as
     # bash on Windows is usually very slow.
     execute_external = (not sys.platform in ['win32'])
 
 def get_library_path(file):
     cmd = subprocess.Popen([config.clang.strip(),
-                            config.target_cflags.strip(),
-                            '-print-file-name=%s' % file],
+                            '-print-file-name=%s' % file] +
+                           [config.target_cflags],
                            stdout=subprocess.PIPE,
                            env=config.environment,
                            universal_newlines=True)
     if not cmd.stdout:
       lit_config.fatal("Couldn't find the library path for '%s'" % file)
     dir = cmd.stdout.read().strip()
     if sys.platform in ['win32'] and execute_external:
         # Don't pass dosish path separator to msys bash.exe.
         dir = dir.replace('\\', '/')
     return dir
 
 
 def get_libgcc_file_name():
     cmd = subprocess.Popen([config.clang.strip(),
-                            config.target_cflags.strip(),
-                            '-print-libgcc-file-name'],
+                            '-print-libgcc-file-name'] +
+                           [config.target_cflags],
                            stdout=subprocess.PIPE,
                            env=config.environment,
                            universal_newlines=True)
     if not cmd.stdout:
       lit_config.fatal("Couldn't find the library path for '%s'" % file)
     dir = cmd.stdout.read().strip()
     if sys.platform in ['win32'] and execute_external:
         # Don't pass dosish path separator to msys bash.exe.