This is an archive of the discontinued LLVM Phabricator instance.

[llvm-cxxfilt][NFC] Fix test for running in Windows cmd
ClosedPublic

Authored by Orlando on Oct 4 2021, 9:35 AM.

Details

Summary

The test llvm\test\tools\llvm-cxxfilt\delimiters.test started failling when run
from cmd.exe on Windows after D110986 which added a unicode character (⦙) to it.
Piping the unicode character in cmd.exe causes it to be converted to a '?'.
That causes the test to fail because the llvm-cxxfilt output becomes Foo?Bar
rather than the expected Foo⦙Bar.

Redirect the echo output to and from a temporary file to get around this
problem.

Diff Detail

Event Timeline

Orlando requested review of this revision.Oct 4 2021, 9:35 AM
Orlando created this revision.

We were seeing this in a downstream windows bot, although it doesn't seem to affect any public bots. If anyone has suggestions about why that might be, we're open to fiddling our downstream bot.

This looks fine as a workaround, but I have no idea why it's needed - I don't think cmd is even involved in the pipe in this case (it should just be python piping between subprocess calls, although it's been a while since I looked at the lit internals). My best guess is something to do with python versions - it would be worth checking to see if there is a difference between the downstream and upstream versions being run.

FYI, there are a couple of llvm-ar tests that work with non-ascii characters, but it always writes to and then reads from a file. It might be informative to confirm whether lit is definitely the issue or not by playing with this or writing a trivial test that doesn't actually exercise any llvm tools (e.g. by piping the result of echo directly to FileCheck).

Maybe worth doing some extra investigation of lit so we're not tripped up by this in the future?

hans accepted this revision.Oct 5 2021, 12:30 AM
hans added a subscriber: hans.

We're also hitting the error in a downstream build (https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1808), and it reproduces for me locally.

Not sure why it doesn't happen on other Windows bots, but the workaround looks good to me, please commit :-)

llvm/test/tools/llvm-cxxfilt/delimiters.test
33

Maybe add a comment somewhere explaining why piping directly to llvm-cxxfilt doesn't work, to reduce the risk of someone "optimizing" this again.

This revision is now accepted and ready to land.Oct 5 2021, 12:30 AM

This looks fine as a workaround, but I have no idea why it's needed - I don't think cmd is even involved in the pipe in this case (it should just be python piping between subprocess calls, although it's been a while since I looked at the lit internals). My best guess is something to do with python versions - it would be worth checking to see if there is a difference between the downstream and upstream versions being run.

FYI, there are a couple of llvm-ar tests that work with non-ascii characters, but it always writes to and then reads from a file. It might be informative to confirm whether lit is definitely the issue or not by playing with this or writing a trivial test that doesn't actually exercise any llvm tools (e.g. by piping the result of echo directly to FileCheck).

Maybe worth doing some extra investigation of lit so we're not tripped up by this in the future?

We're also hitting the error in a downstream build (https://ci.chromium.org/ui/p/chromium/builders/try/win_upload_clang/1808), and it reproduces for me locally.

Not sure why it doesn't happen on other Windows bots, but the workaround looks good to me, please commit :-)

I'll spend some time looking deeper today and write a ticket if I can't find a fix, but I'll commit this for now as a work around. Thanks all.

This revision was landed with ongoing or failed builds.Oct 5 2021, 4:10 AM
This revision was automatically updated to reflect the committed changes.

Ok, I have found that it's actually the echo that is causing the issue for me; it is the echo chosen based on my build config doing the the to ? conversion. It turns out that redirecting to and from a temporary file side steps the problem because echo is special-cased to run as an "in-process builtins" in llvm-lit iff it's not running as part of a pipeline.

Is there a config tweak needed to get this to run properly for us? Given the upstream Windows bots are not seeing a problem.

hans added a comment.Oct 5 2021, 6:37 AM

Is there a config tweak needed to get this to run properly for us? Given the upstream Windows bots are not seeing a problem.

I'm guessing the upstream bots get their "echo" from msys/Git. In Chromium we use "echo" etc. from GnuWin, and that's what I have locally too.

Maybe the best fix would be to make lit's internal "echo" work both in the pipe and redirect-to-file cases? That way we'd reduce our reliance on external tools, which seems like a good thing.

Is there a config tweak needed to get this to run properly for us? Given the upstream Windows bots are not seeing a problem.

I'm guessing the upstream bots get their "echo" from msys/Git. In Chromium we use "echo" etc. from GnuWin, and that's what I have locally too.

My "bad" echo is from GnuWin too. Testing locally it looks like the echo shipped with git for Windows handles unicode without an issue.

Maybe the best fix would be to make lit's internal "echo" work both in the pipe and redirect-to-file cases? That way we'd reduce our reliance on external tools, which seems like a good thing.

I'm not very familiar with llvm-lit internals and am unsure if there's a reason this wasn't done before. I've mentioned this on llvm-dev (subject [llvm-dev][llvm-lit] Old echo causes test failure on windows) to get others' thoughts on the topic too.