[PATCH] D82754: [lit] Prevent hang when lit sees non-ASCII characters

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D82754#2136412 <https://reviews.llvm.org/D82754#2136412>, @richard.barton.arm wrote:

> I will push an update with a new comment and my new test.


Thanks!

> I guess the evilness of it when/if it were to regress is ok - make sure we don't regress I suppose!

I agree that the possibility of regressions causing hangs is not nice, but I think it's better for them to occur in our test suite immediately than in the wild later.



================
Comment at: llvm/utils/lit/lit/display.py:89
                     pass
-                out = out.decode(encoding=sys.stdout.encoding)
+                out = out.decode(encoding=sys.stdout.encoding, errors="ignore")
             print(out)
----------------
jdenny wrote:
> Please add a comment documenting the platform where this problem can be seen.  Include the python version.  If people try to simplify this subtle passage of code later, perhaps to abandon python 2 support, such comments should prove helpful.
When I first read this new comment, I thought it might be repeating the info from the earlier comment but at a more specific location in the code.  To make it clear this is a different issue, I'd prefer "can raise UnicodeDecodeError" -> "can raise UnicodeDecodeError here too".

Don't forget the `.` on the last sentence.


================
Comment at: llvm/utils/lit/tests/shtest-shell-ascii.py:7
+# RUN: cat %t.out
+# RUN: FileCheck --input-file %t.out %s
+#
----------------
This test mostly copies a piece of `shtest-shell.py` but with `PYTHONIOENCODING=ascii`.  In doing so, it separates the original `shtest-encoding.txt` from the new one even though they're covering related bugs.

I wonder if it would be better to keep everything together and avoid duplication by just extending `shtest-shell.py` with these new `RUN:` lines.  A comment could explain that `shtest-encoding.txt` is the main focus but that the other tests are covered by these new `RUN:` lines too just in case a problem crops up in them.

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82754/new/

https://reviews.llvm.org/D82754