[lit] Prevent hang when lit sees non-ASCII characters

[richard.barton.arm]

Summary

As per discussion in D69207

[jdenny]

Please add a test to lit's test suite, perhaps something like llvm/utils/lit/tests/Inputs/shtest-shell/stdout-encoding.txt.

Thanks for working on this.

llvm/utils/lit/lit/display.py

89 ↗

(On Diff #274045)

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.

[richard.barton.arm]

Hi @jdenny

Agreed on adding a comment and test - the original patch was a bit information-light, sorry about that.

I am really struggling to figure out how to add a test for this and wonder if you can help/advise.

My environment is python 3.5, ubuntu 16.04 LTS, bash shell with this locale:

LANG=en_US.UTF-8
LANGUAGE=en_US:
LC_CTYPE="C"
LC_NUMERIC="C"
LC_TIME="C"
LC_COLLATE="C"
LC_MONETARY="C"
LC_MESSAGES="C"
LC_PAPER="C"
LC_NAME="C"
LC_ADDRESS="C"
LC_TELEPHONE="C"
LC_MEASUREMENT="C"
LC_IDENTIFICATION="C"
LC_ALL=C

In my environment, the test added under D69207 - shtest-shell/stdout-encoding.txt - hits the UnicodeDecodeError when I run it directly with llvm-lit -a. Setting LC_ALL=en_US.utf-8 avoids the error, obviously. When running the test via make with LIT_OPTS="-a --filter=shtest-shell" make check-lit it does not fail, which is, I assume, why the problem has gone un-detected. I cannot work out how to build a test that runs the 'inner lit' in such an environment where the error would be raised. I can make a test that is adapted from shtest-shell/stdout-encoding.txt with this RUN line

# RUN: not env PYTHONIOENCODING=ascii %{lit} -j 1 -v %{inputs}/shtest-shell-ascii > %t.out

which will trigger the error for me when run with llvm-lit without -a (although the failure mode is to hang, so pretty nasty) but this test passes in make check-all.

I can continue to look further but can you see a trick I am missing? Alternatively, am I on a fool's errand here and a new test will not be possible or practical?

[jdenny]

In D82754#2127919, @richard.barton.arm wrote:

Hi @jdenny

Hi. Sorry for the delay.

I can make a test that is adapted from shtest-shell/stdout-encoding.txt with this RUN line

# RUN: not env PYTHONIOENCODING=ascii %{lit} -j 1 -v %{inputs}/shtest-shell-ascii > %t.out

which will trigger the error for me when run with llvm-lit without -a (although the failure mode is to hang, so pretty nasty)

Thanks for finding this reproducer!

but this test passes in make check-all.

By "passes", I think you're saying that it doesn't reproduce the error. Right?

If so, I suspect that llvm-lit called directly uses python2 on your system, but check-all uses python3. Can you confirm?

I also suspect this is a python2-specific bug.

[richard.barton.arm]

By "passes", I think you're saying that it doesn't reproduce the error. Right?

Yes, sorry. I was trying to write a test that would fail _without_ my patch applied so had got myself turned around.

If so, I suspect that llvm-lit called directly uses python2 on your system, but check-all uses python3. Can you confirm?
I also suspect this is a python2-specific bug.

Spot on - I can confirm this is what is happening. Thanks for clearing that up for me.

I will push an update with a new comment and my new test. I guess the evilness of it when/if it were to regress is ok - make sure we don't regress I suppose!

[richard.barton.arm]

Add test and comment.

[richard.barton.arm]

Arg - finger trouble using arc!

I pushed the Flang test that I was writing that made me stumble across this
issue as part of this patch by mistake. Remove it from the new version.

[jdenny]

In 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.

llvm/utils/lit/lit/display.py

89 ↗

(On Diff #274045)

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.

llvm/utils/lit/tests/shtest-shell-ascii.py

7 ↗

(On Diff #276096)

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?

[richard.barton.arm]

Updated version taking into account review comments on test and comment.

[richard.barton.arm]

Actually delete the previous new test (rebasing issues - sorry)

[richard.barton.arm]

llvm/utils/lit/tests/shtest-shell-ascii.py

7 ↗

(On Diff #276096)

Agreed. We have to slightly relax the expected output to work for both though as now the shell will skip a character rather than emit one.

[jdenny]

LGTM. Thanks for the changes!

llvm/utils/lit/tests/shtest-shell-ascii.py

7 ↗

(On Diff #276096)

Ah, I didn't think of that. But it's probably fine.

flang/test/Semantics/modfile01.f90

-! RUN: %S/test_modfile.sh %s %t %f18
+! RUN: mkdir -p %t.dir
+! RUN: %f18 -module %t.dir -fdebug-resolve-names -fparse-only %s
+
+! Check that no additional module files have been created.
+! RUN: ls -l %t.dir/*.mod | wc -l | FileCheck --check-prefix=NO_MODFILES %s
+! NO_MODFILES: 4
+
+! Module files start with a 3-byte UTF BOM which lit struggles with. Remove it
+! with sed before lit sees it.
+! RUN: FileCheck --input-file=%t.dir/m1.mod --check-prefix=MOD-M1 %s
+! RUN: FileCheck --input-file=%t.dir/m2.mod --check-prefix=MOD-M2 %s
+! RUN: FileCheck --input-file=%t.dir/m3.mod --check-prefix=MOD-M3 %s
+! RUN: FileCheck --input-file=%t.dir/m4.mod --check-prefix=MOD-M4 %s
+
+
 ! Check correct modfile generation for type with private component.
-module m
+module m1
   integer :: i
   integer, private :: j
   type :: t
     integer :: i
     integer, private :: j
   end type
   type, private :: u
   end type
   type(t) :: x
 end
 
-!Expect: m.mod
-!module m
-!integer(4)::i
-!integer(4),private::j
-!type::t
-!integer(4)::i
-!integer(4),private::j
-!end type
-!type,private::u
-!end type
-!type(t)::x
-!end
+! MOD-M1: module m1
+! MOD-M1-NEXT: integer(4)::i
+! MOD-M1-NEXT: integer(4),private::j
+! MOD-M1-NEXT: type::t
+! MOD-M1-NEXT: integer(4)::i
+! MOD-M1-NEXT: integer(4),private::j
+! MOD-M1-NEXT: end type
+! MOD-M1-NEXT: type,private::u
+! MOD-M1-NEXT: end type
+! MOD-M1-NEXT: type(t)::x
+! MOD-M1-NEXT: end
 
 ! Check correct modfile generation for type with private module procedure.
 
 module m2
   private :: s1
 contains
   subroutine s1()
   end
   subroutine s2()
   end
 end
 
-!Expect: m2.mod
-!module m2
-! private::s1
-!contains
-! subroutine s1()
-! end
-! subroutine s2()
-! end
-!end
+! MOD-M2: module m2
+! MOD-M2-NEXT:  private::s1
+! MOD-M2-NEXT: contains
+! MOD-M2-NEXT:  subroutine s1()
+! MOD-M2-NEXT:  end
+! MOD-M2-NEXT:  subroutine s2()
+! MOD-M2-NEXT:  end
+! MOD-M2-NEXT: end
 
 module m3
   private
   public :: f1
 contains
   real function f1()
   end
   real function f2()
   end
 end
 
-!Expect: m3.mod
-!module m3
-! private::f2
-!contains
-! function f1()
-!  real(4)::f1
-! end
-! function f2()
-!  real(4)::f2
-! end
-!end
+! MOD-M3: module m3
+! MOD-M3-NEXT:  private::f2
+! MOD-M3-NEXT: contains
+! MOD-M3-NEXT:  function f1()
+! MOD-M3-NEXT:   real(4)::f1
+! MOD-M3-NEXT:  end
+! MOD-M3-NEXT:  function f2()
+! MOD-M3-NEXT:   real(4)::f2
+! MOD-M3-NEXT:  end
+! MOD-M3-NEXT: end
 
 ! Test optional dummy procedure
 module m4
 contains
   subroutine s(f)
     interface
       logical recursive function f()
         implicit none
       end function
     end interface
     optional f
   end
 end
 
-!Expect: m4.mod
-!module m4
-!contains
-! subroutine s(f)
-!  optional::f
-!  interface
-!   recursive function f()
-!    logical(4)::f
-!   end
-!  end interface
-! end
-!end
+! MOD-M4: module m4
+! MOD-M4-NEXT: contains
+! MOD-M4-NEXT:  subroutine s(f)
+! MOD-M4-NEXT:   optional::f
+! MOD-M4-NEXT:   interface
+! MOD-M4-NEXT:    recursive function f()
+! MOD-M4-NEXT:     logical(4)::f
+! MOD-M4-NEXT:    end
+! MOD-M4-NEXT:   end interface
+! MOD-M4-NEXT:  end
+! MOD-M4-NEXT: end
+