[PATCH] D157210: [symbolizer] Change reaction on invalid input

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.

In D157210#4587435 <https://reviews.llvm.org/D157210#4587435>, @jhenderson wrote:

> In D157210#4584620 <https://reviews.llvm.org/D157210#4584620>, @sepavloff wrote:
>
>> In D157210#4584361 <https://reviews.llvm.org/D157210#4584361>, @jhenderson wrote:
>>
>>> My opinion is that if someone is in interactive mode and they get an error, it may make sense to exit llvm-symbolizer, rather than continue. How does this differ to the previous (prior to your patch) behaviour for other error cases (i.e. cases other than "missing an input file/address pair")?
>>
>> This is behavior of `llvm-symbolizer` before this patch. Whenever it detects an invalid input it echoes the input and continue waiting on stdin. This behavior makes sense, - if the program got wrong command from interactive input, it can continue working because the next commands may do useful job.
>>
>> Anyway, it is not `llvm-symbolizer` that caused the hang, it is blocking read in the test script, which waits input from the stream that is empty in the case of new `llvm-symbolizer`.
>
> Sorry, I'm struggling to follow what you're saying. My concern is that because we had a test script that has gone from working to hanging, it's possible that an end user's script will be in the same state, so when they get the updated llvm-symbolizer, they will have a script that was previously working but is now hanging. I'm not concerned by llvm-symbolizer changing in other ways that could potentially invalidate their script, but introducing a potential path to a hang is a cause for concern. What does llvm-addr2line do in this sort of situation? Does it print something like "??:0:0" (as well as the error), because I think that might be a good answer.
>
> @MaskRay/@ikudrin, any thoughts?

Is `llvm/test/Support/interrupts.test` the only failing test? (https://lab.llvm.org/buildbot/#/builders/216/builds/25497/steps/7/logs/stdio linked by @dyung above has tons of logs but I cannot find the relevant line.)

If yes, I think the latest revision contains the right change.

  -     proc.stdin.write(b'foo\n')
  +    proc.stdin.write(bytes(sys.argv[2] + " 0\n", 'ascii'))



  % /tmp/Debug/bin/llvm-symbolizer
  /tmp/Debug/bin/llvm-symbolizer 0
  ??
  ??:0:0
  
  foo
  /tmp/Debug/bin/llvm-symbolizer: error: 'foo': no input filename is specified

Since llvm-symbolizer no longer echoes (to stdout) the invalid `foo` input, `proc.stdout.readline()` in the Python script will block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157210