[llvm-symbolizer] Flush output on bad input

[jhenderson]

Summary

One way of using llvm-symbolizer is to interactively within a process write a line from a parent process to llvm-symbolizer's stdin, and then read the output, then write the next line, read, etc. This worked as long as all the lines were good. However, this didn't work prior to this patch if any of the inputs were bad inputs, because the output is not flushed after a bad input, meaning the parent process is sat waiting for output, whilst llvm-symbolizer is sat waiting for input. This patch flushes the output after every invocation of symbolizeInput when reading from stdin. It also removes unnecessary flushing when llvm-symbolizer is not reading addresses from stdin, which should give a slight performance boost in these situations.

[jhenderson]

Make script python3 compatible by using bytes and flushing stdin.

[ikudrin]

I am a bit worried about the test which may, potentially, hang. Don't you think there should be any kind of a watchdog?

[jhenderson]

In D62371#1515682, @ikudrin wrote:

I am a bit worried about the test which may, potentially, hang. Don't you think there should be any kind of a watchdog?

Fair point, given that without the fix it does hang. Does lit have a built-in timeout option? Otherwise, I think the easiest thing to do is add a timeout to the test script or similar.

[jhenderson]

Add watchdog to timeout the test after 20 seconds.

[jhenderson]

In D62371#1515765, @jhenderson wrote:

In D62371#1515682, @ikudrin wrote:

I am a bit worried about the test which may, potentially, hang. Don't you think there should be any kind of a watchdog?

Fair point, given that without the fix it does hang. Does lit have a built-in timeout option? Otherwise, I think the easiest thing to do is add a timeout to the test script or similar.

I found lit does have a per-test timeout option, but it cannot be configured on an individual test level, only on a whole directory level, which I wasn't keen to do, so I've done a similar thing to what lit does in my test script.

[ikudrin]

test/tools/llvm-symbolizer/Inputs/flush-output.py

8 ↗

(On Diff #201234)

It looks like when process.kill() is executed, the main thread stops waiting, processes the rest and exits with the exit status 0.

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.

[jhenderson]

test/tools/llvm-symbolizer/Inputs/flush-output.py

8 ↗

(On Diff #201234)

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.

[ikudrin]

test/tools/llvm-symbolizer/Inputs/flush-output.py

8 ↗

(On Diff #201234)

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)

[jhenderson]

test/tools/llvm-symbolizer/Inputs/flush-output.py

8 ↗

(On Diff #201234)

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.

[jhenderson]

Switch to using os._exit() instead of sys.exit(). This terminates the process even from the subthread. There's no need to do any cleanup etc, so this should be sufficient for our test.

(I originally was going to put a try/except block around the stdin/stdout reading/writing blocks, but on second thoughts, I don't think that's necessary).

[ikudrin]

LGTM

[jhenderson]

Reinstate 20s timeout back up from 2s (used for experimentation).

[jhenderson]

Re-add FileCheck run accidentally removed in previous version.

llvm/trunk/test/tools/llvm-symbolizer/Inputs/flush-output.py

+from __future__ import print_function
+import os
+import subprocess
+import sys
+import threading
+
+def kill_subprocess(process):
+    process.kill()
+    os._exit(1)
+
+# Pass -f=none and --output-style=GNU to get only one line of output per input.
+cmd = subprocess.Popen([sys.argv[1],
+                        '--obj=' + sys.argv[2],
+                        '-f=none',
+                        '--output-style=GNU'], stdout=subprocess.PIPE, stdin=subprocess.PIPE)
+watchdog = threading.Timer(20, kill_subprocess, args=[cmd])
+watchdog.start()
+cmd.stdin.write(b'0\n')
+cmd.stdin.flush()
+print(cmd.stdout.readline())
+cmd.stdin.write(b'bad\n')
+cmd.stdin.flush()
+print(cmd.stdout.readline())
+watchdog.cancel()

llvm/trunk/test/tools/llvm-symbolizer/flush-output.s

+# REQUIRES: x86-registered-target
+
+## If a process spawns llvm-symbolizer, and wishes to feed it addresses one at a
+## time, llvm-symbolizer needs to flush its output after each input has been
+## processed or the parent process will not be able to read the output and may
+## deadlock. This test runs a script that simulates this situation for both a
+## a good and bad input.
+
+foo:
+    nop
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o -g
+# RUN: %python %p/Inputs/flush-output.py llvm-symbolizer %t.o \
+# RUN:   | FileCheck %s
+
+# CHECK: flush-output.s:10
+# CHECK: bad

llvm/trunk/tools/llvm-symbolizer/llvm-symbolizer.cpp

static void symbolizeInput(StringRef InputString, LLVMSymbolizer &Symbolizer,
   } else {
     auto ResOrErr = Symbolizer.symbolizeCode(
         ModuleName, {Offset, object::SectionedAddress::UndefSection},
         ClDwpName);
     Printer << (error(ResOrErr) ? DILineInfo() : ResOrErr.get());
   }
   if (ClOutputStyle == DIPrinter::OutputStyle::LLVM)
     outs() << "\n";
-  outs().flush();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   bool IsAddr2Line = sys::path::stem(argv[0]).contains("addr2line");
 
   if (IsAddr2Line) {
int main(int argc, char **argv) {
   DIPrinter Printer(outs(), ClPrintFunctions != FunctionNameKind::None,
                     ClPrettyPrint, ClPrintSourceContextLines, ClVerbose,
                     ClBasenames, ClOutputStyle);
 
   if (ClInputAddresses.empty()) {
     const int kMaxInputStringLength = 1024;
     char InputString[kMaxInputStringLength];
 
-    while (fgets(InputString, sizeof(InputString), stdin))
+    while (fgets(InputString, sizeof(InputString), stdin)) {
       symbolizeInput(InputString, Symbolizer, Printer);
+      outs().flush();
+    }
   } else {
     for (StringRef Address : ClInputAddresses)
       symbolizeInput(Address, Symbolizer, Printer);
   }
 
   return 0;
 }