[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

On 11/20/21 01:42, Rémi Bernon wrote:
> On 11/19/21 17:33, Jinoh Kang wrote:
>> On 11/19/21 23:54, Rémi Bernon wrote:
>>> Hi Jinoh!
>>>
>>> A few comments:
>>>
>>> On 11/19/21 14:41, Jinoh Kang wrote:
>>>> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
>>>> ---
>>>
>>> Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?
>>
>> De-duplicating the buffer management code sounds like a good idea.
>>
>> My intention was to minimise touching existing code and do refactoring in
>> a separate patch serie.  Turns out this extra step is unnecessary, I guess.
>>
> 
> I think winegdb in general and gdbproxy in particular aren't used very much

Wait, there were any alternatives?  Bare GDB was always cumbersome to me, since
it never detects all the DLL images automatically.

Maybe I should take a look at MinGW build of gdbserver.

> and definitely need more love.

Especially since its use is infrequent, test coverage should be what needs most
improvement.  We can even get free conformance tests if we get rid of all
Wine-dependent code from WineDbg (sounds weird, I know).

(The major part of Wine dependence is the Unix path resolution.  Removing this
 would mean that gdbproxy shall implement the file I/O protocol, so that
 GDB can read files from Windows paths.  Sounds like a lot of work, though.)

> Hence I'd not be too worried about changing the code, I don't think anything actually depends on it, and improvements are probably welcome.
> 
>>>
>>> At least it'd be nice to have the escaping done separately from the qxfer buffer addition, if possible.
>>
>> I agree.  In this case I would say that the qXfer buffer addition logically
>> precedes the XML escaping.  The reason is that the qXfer buffer is required to
>> correctly calculate the offset and length of the reply data slice that should
>> be transmitted to the debugger client.
>>
>> Currently gdbproxy reuses the same buffer for both qXfer reply and actual GDB
>> packet reply.  This works well since a character in the qXfer reply buffer
>> matches 1:1 to the actual GDB reply packet.  However, if special characters are
>> escaped, this property will no longer hold and a single byte in qXfer reply will
>> take up to two bytes in the GDB reply packet.  This causes offsets to shift,
>> preventing the offset/length response slicing from working correctly.
>>
>> I'll copy the above in the commit message.
>>
> 
> Sounds good.
> 
>>>
>>>>    +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
>>>> +{
>>>> +    reply->alloc = alloc;
>>>> +    reply->base = realloc(reply->base, reply->alloc);
>>>> +}
>>>> +
>>>> +static void reply_buffer_empty(struct reply_buffer* reply)
>>>> +{
>>>> +    reply->len = 0;
>>>> +    reply_buffer_resize(reply, 0);
>>>> +}
>>>> +
>>>
>>> Do we need to realloc when emptying? I think the less alloc call we do the better.
>>
>> I just noticed that gdbproxy does not free the old packet reply after transmission.
>> There's no reason reply_buffer shall deviate from this behaviour.  Thanks!
>>
> 
> Yes, I think it's just cleaned up on exit, there's no real use to do the cleanup properly here.
> 
>>> I don't think we should worry too much about memory consumption here?
>>
>> In fact, if memory consumption isn't a concern, why don't we straight out just
>> pre-allocate buffers in fixed sizes?  This way we don't have to worry about
>> realloc() entirely (we would still need to look out for overflows though).
>> This is also what some GDB stubs (e.g. the Linux kernel) does.
>> Perhaps fifteen pages followed by a guard to catch overlooked memory corruption
>> issues would suffice?
>>
> 
> I guess we could, or at least allocate a bigger buffer and reallocate it by *3/2 steps when needed. I'm not completely sure why I only added small increments, maybe it was already like this before.

Good.

> 
> It could be static too, of course if we can assume that gdb will only ever ask limited size chunks (even if you try to dump large portion of memory for instance).

GDB seems to do just that.  For LLDB (which uses a compatible, albeit subtly different protocol), I believe it doesn't.

> 

-- 
Sincerely,
Jinoh Kang