[RFC,2/3,gdb/dap] Allow Content-Length on separate line

Message ID 20230314130535.6369-3-tdevries@suse.de
State Changes Requested
Headers
Series Handle DAP command files |

Commit Message

Tom de Vries March 14, 2023, 1:05 p.m. UTC
  Currently this DAP input is accepted:
...
Content-Length: 54

{"seq": 1, ...}Content-Length: 163

{"seq": 2, ...}Content-Length: 136
...

Also allow:
...
Content-Length: 54

{"seq": 1, ...}
Content-Length: 163

{"seq": 2, ...}
Content-Length: 136
...
which makes command files a bit easier to read.
---
 gdb/python/lib/gdb/dap/io.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
  

Comments

Tom Tromey March 14, 2023, 2:13 p.m. UTC | #1
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Also allow:
Tom> ...
Tom> Content-Length: 54

Tom> {"seq": 1, ...}
Tom> Content-Length: 163

Tom> {"seq": 2, ...}
Tom> Content-Length: 136
Tom> ...
Tom> which makes command files a bit easier to read.

Doesn't this violate the XML-RPC protocol?  The issue being that the
header is terminated by a blank line, but now this ignores the blank
line.  Though of course it's also bad to not have a Content-Length.

Tom
  
Tom de Vries March 14, 2023, 3:35 p.m. UTC | #2
On 3/14/23 15:13, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Also allow:
> Tom> ...
> Tom> Content-Length: 54
> 
> Tom> {"seq": 1, ...}
> Tom> Content-Length: 163
> 
> Tom> {"seq": 2, ...}
> Tom> Content-Length: 136
> Tom> ...
> Tom> which makes command files a bit easier to read.
> 
> Doesn't this violate the XML-RPC protocol?  The issue being that the
> header is terminated by a blank line, but now this ignores the blank
> line.  Though of course it's also bad to not have a Content-Length.

OK, let's try the example from here ( 
https://microsoft.github.io/debug-adapter-protocol/overview ), two after 
each other, as confirming DAP:
...
Content-Length: 119\r\n
\r\n
{ "seq": <n>, ... }Content-Length: 119\r\n
\r\n
{ "seq": <n>, ... }
...

What I'm proposing with this patch allows in addition:
...
Content-Length: 119\r\n
\r\n
{ "seq": <n>, ... }\r\n
Content-Length: 119\r\n
\r\n
{ "seq": <n>, ... }
...

So the \r\n that terminates the header is still there.

And the blank line (^\r\n) that separates header and content is still there.

Whether this is still confirming DAP, I can't tell from the specification.

I can imagine that it makes sense for GDB to be as strict as possible to 
flush out problems in actual clients.

OTOH, the mock-up client we create by feeding a text file into gdb 
doesn't actually need to conform to DAP, and there's something to win by 
making this text file easy to read and edit, which is what the goal of 
this patch is.

So, perhaps we want to enable this selectively, say with a setting 
dap-parse-strict, and perhaps have some "set dap-cmd-input gdb.in" that 
automatically sets dap-parse-strict to 0.

Alternatively, we could move this into the command-sphere and do 
something like:
...
interpreter-exec dap-wrap { "seq": <n>, ... }
...
and let dap-wrap take care of adding a header with the correct size.

But this all might be overkill, I'm not sure.

Thanks,
- Tom
  
Tom Tromey May 5, 2023, 1:09 p.m. UTC | #3
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Sorry about the delay in replying to this.

Tom> What I'm proposing with this patch allows in addition:
Tom> ...
Tom> Content-Length: 119\r\n
Tom> \r\n
Tom> { "seq": <n>, ... }\r\n
Tom> Content-Length: 119\r\n
Tom> \r\n
Tom> { "seq": <n>, ... }
Tom> ...

Tom> So the \r\n that terminates the header is still there.

Tom> And the blank line (^\r\n) that separates header and content is still there.

Tom> Whether this is still confirming DAP, I can't tell from the specification.

Yeah, the text says:

    Since both the last header field and the overall header itself are each
    terminated with \r\n, and since the header is mandatory, the content
    part of a message is always preceded (and uniquely identified) by two
    \r\n sequences.

which to me means that they didn't consider the possibility of a blank
line as the first line of the header part.  Oops.

Tom> So, perhaps we want to enable this selectively, say with a setting
Tom> dap-parse-strict, and perhaps have some "set dap-cmd-input gdb.in"
Tom> that automatically sets dap-parse-strict to 0.

I'd be fine with this.

Tom
  

Patch

diff --git a/gdb/python/lib/gdb/dap/io.py b/gdb/python/lib/gdb/dap/io.py
index 28f4d93ba46..fd9953a7aaa 100644
--- a/gdb/python/lib/gdb/dap/io.py
+++ b/gdb/python/lib/gdb/dap/io.py
@@ -28,7 +28,10 @@  def read_json(stream):
         line = stream.readline()
         line = line.strip()
         if line == b"":
-            break
+            if content_length != None:
+                break
+            else:
+                continue
         if line.startswith(b"Content-Length:"):
             line = line[15:].strip()
             content_length = int(line)