[vscode.py] Make read_packet only return None when EOF

[aadsm]

Summary

lldb-vscode has an issue when run in stdout/stdin mode because it will send all stdout generated by scripts through well.. stdout :)
This is problematic for any adapter using lldb-vscode in that mode since it will not produce DAP messages.
Even if we ignore that the method explicitly says that None is only returned in EOF situation which was not the case.

[clayborg]

So what is this fix actually doing? Allowing random STDOUT to be ignored?

[aadsm]

Yes. This happens when lldb-vscode is run in stdin/stdout mode (which is what happens in tests and I've also seen some IDEs doing the same) and installed commands are doing "print". In this case the client gets data that does not start with Content-Length.
This fixes the tests but overall is still a problem for IDEs (but that's another discussion).

[MaskRay]

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py

58

Prefer single quotes.

[clayborg]

Although I am ok with this patch, it will only work if the unexpected output comes before we expect any packets or perfectly in between packets. Not sure we should do this. For example this would work:

random outut
Content Length:2

{}

And this would work:

Content Length:2

{}
random outut
Content Length:2

{}

But this wouldn't:

Content Length:2

random outut
{}

We could also end up with:

Content Length:2


{random outut}

where "random outut" was written to STDOUT and magically appeared between the two braces.

So the main question is:

• do we try and deal with random output being able to be mixed in and have our solution only work some of the time
• or just never allow it (current state of things)

[labath]

This patch seems really weird to me... We may be able to work around random lldb output in the test this way, but that won't help any other DAP client who ends up talking to lldb-vscode (or do they have similar workarounds already?). In fact it might make things worse -- right now, I guess this is only an issue if the user has a noisy .lldbinit file, but if we start ignoring this, other random output can appear in the future.

If having a "clean" stdout is important in lldb-vscode, then I think it would be better to *not* merge this patch, and try to fix any places which are polluting it instead. Since everything should be using the SBDebuggers notion of "stdout", anything which prints to the process stdout directly is a bug...

[aadsm]

Fair enough, I haven't seen evidence of this (haven't searched for it) but I imagine IDEs need to ignore this as well otherwise they just barf if they're expecting Content-Length and a wild print appears. The notion of stdout of SBDebugger is the SBCommandReturnObject which doesn't print right away (only when the command finishes executing) and from my previous tests .flush() doesn't help with this. I believe this is the reason why people opt to use print instead in their custom commands. But I don't think it's a reasonable assumption (or api contract) to tell people they can't use print or it breaks lldb-vscode.

I'm going to fix this in lldb-vscode itself to wrap all stdout in a proper DAP console response.

[labath]

In D70883#1771686, @aadsm wrote:

Fair enough, I haven't seen evidence of this (haven't searched for it) but I imagine IDEs need to ignore this as well otherwise they just barf if they're expecting Content-Length and a wild print appears. The notion of stdout of SBDebugger is the SBCommandReturnObject which doesn't print right away (only when the command finishes executing) and from my previous tests .flush() doesn't help with this. I believe this is the reason why people opt to use print instead in their custom commands. But I don't think it's a reasonable assumption (or api contract) to tell people they can't use print or it breaks lldb-vscode.

I'm going to fix this in lldb-vscode itself to wrap all stdout in a proper DAP console response.

Interesting.. I was expecting this was already handled somehow within lldb -- we have a fair amount of code which tries to install fake stdin/out handles when running a python script. I'd start with checking out why that piece of code isn't kicking in...

[jingham]

In D70883#1771686, @aadsm wrote:

Fair enough, I haven't seen evidence of this (haven't searched for it) but I imagine IDEs need to ignore this as well otherwise they just barf if they're expecting Content-Length and a wild print appears. The notion of stdout of SBDebugger is the SBCommandReturnObject which doesn't print right away (only when the command finishes executing) and from my previous tests .flush() doesn't help with this. I believe this is the reason why people opt to use print instead in their custom commands. But I don't think it's a reasonable assumption (or api contract) to tell people they can't use print or it breaks lldb-vscode.

I'm going to fix this in lldb-vscode itself to wrap all stdout in a proper DAP console response.

This is an aside, but, lldb knows how to immediately output command results. For instance, when stop hooks are run the output is sent to the command result's "Immediate output stream", since the stop hook might auto-continue and you don't want to wait till somebody's "next" or "continue" actually returns to see stop hook output. There are some other commands that produce lots of output that are also immediate.

It should be possible to do this from Python as well, by inserting the debugger's Output file into the command's immediate output handle. That's the way it is supposed to work, though I didn't check to make sure this is all wired up. But it should not be necessary to use "print" to output command results just because you want to see them immediately output.

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py

while i < data_len:
         i = i + curr_data_len
         outfile.write('\n')
 
 
 def read_packet(f, verbose=False, trace_file=None):
     '''Decode a JSON packet that starts with the content length and is
        followed by the JSON bytes from a file 'f'. Returns None on EOF.
     '''
+    while True:
         line = f.readline().decode("utf-8")
         if len(line) == 0:
             return None  # EOF.
 
         # Watch for line that starts with the prefix
         prefix = 'Content-Length: '
         if line.startswith(prefix):
             # Decode length of JSON bytes
             if verbose:
                 print('content: "%s"' % (line))
             length = int(line[len(prefix):])
             if verbose:
                 print('length: "%u"' % (length))
             # Skip empty line
             line = f.readline()
             if verbose:
                 print('empty: "%s"' % (line))
             # Read JSON bytes
             json_str = f.read(length)
             if verbose:
                 print('json: "%s"' % (json_str))
             if trace_file:
                 trace_file.write('from adaptor:\n%s\n' % (json_str))
             # Decode the JSON bytes into a python dictionary
             return json.loads(json_str)
 
-    return None
-
 
 def packet_type_is(packet, packet_type):
     return 'type' in packet and packet['type'] == packet_type
 
 
 def read_packet_thread(vs_comm):
     done = False
     while not done: