[PATCH] D62371: [llvm-symbolizer] Flush output on bad input

[James Henderson via Phabricator via llvm-commits]

jhenderson marked an inline comment as done.
jhenderson added inline comments.


================
Comment at: test/tools/llvm-symbolizer/Inputs/flush-output.py:8
+    process.kill()
+    sys.exit(1)
+
----------------
ikudrin wrote:
> jhenderson wrote:
> > ikudrin wrote:
> > > It looks like when `process.kill()` is executed, the main thread stops waiting, processes the rest and exits with the exit status `0`.
> > > 
> > > [[ https://docs.python.org/3/library/sys.html#sys.exit | The python documentation]] says:
> > > > Since exit() ultimately “only” raises an exception, it will only exit the process when called from the main thread, and the exception is not intercepted.
> > I'm not sure what you're asking me to do here? This code causes llvm-symbolizer to end, rather than continuing to hang, and then the python script exits.
> > 
> > For reference, I verified that the test fails and only hangs for 20 seconds as expected, without the bug fix, and passes with the bug fix.
> Yes, the test fails, but not because the python script finishes with the exit code "1". The actual failure is that FileCheck cannot find all "CHECK" lines. Thus, the line `sys.exit(1)` is a bit of misleading.
> 
> If I understand your intention right, I would suggest rewriting the script in the following way:
> ```
> exit_status = 0
> 
> def kill_subprocess(process):
>     global exit_status
>     exit_status = 1
>     process.kill()
> 
> cmd = ...
> ...
> sys.exit(exit_status)
> ```
Oh, I didn't realise that sys.exit() doesn't do anything in a subthread. process.kill() ends up causing an IOError later on, but I think this can be worked around by using a try/except block. I'll update the diff in a bit to show what I mean.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62371