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

[Rémi Bernon]

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 and definitely need more love. 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.

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).

-- 
Rémi Bernon <rbernon at codeweavers.com>