Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet

[Luc Michel]

Hi,

On 2/1/19 3:25 PM, Lucien Anti-Spam wrote:
> Hi Luc,
> 
> Great feedback from both you and Eric - im blown away, I dont think
> theres a single 1byte ascii character left unturned there :)
> My coding style snuck in erywhere, as did my Scottish English!
> 
> On Friday, February 1, 2019, 10:33:18 PM GMT+9, Luc Michel
> <address@hidden> wrote:
>> > +    GDBThreadIdKind vcontThreadType ;
>> The coding style ... CODING_STYLE ...
> Ah I didnt pick up on this whilst reading the Wiki for coding style,
> thank you!
> 
>> > +            vcontThreadType = GDB_ALL_THREAD ;> +            pid = 1 ;
>> The spec is not clear but I would opt for GDB_ALL_PROCESSES instead of
>> GDB_ALL_THREAD here. pid = 1; is clearly wrong since you don't know if
>> this PID exists or is currently attached.
> 
> I was in a quandary here, and I did see your previous email too.
> However, I looked at how the call to read_thread_id() behaved and
>  copied over the same behavior as if the command was vCont;c:-1
> which I understand to mean continue all threads?
This is where the spec lacks details. The doc says that the vCont packet
is used to "Resume the inferior, specifying different actions for each
thread."

So it seems to target only one inferior (i.e. one process).

When multiprocess extension is in use, this packet must be supported,
and used in place of 'Hc' (which modifies s->c_cpu) and other action
packets (s, S, ...) which are deprecated.

So the question is: when the vCont packet does not specifies a
thread-id, what _process_ we should choose?

Going for all process is probably ok because:
  - we don't have to bother choosing a PID,
  - it will cover the mono-process case correctly (main use-case of this
form of vCont packet I think).

Otherwise, we can go for GDB_ALL_THREAD and take the PID of the first
attached CPU. I'm not sure sure which one is better...

Thanks.

Luc

> 
> So maybe we should look at altering read_thread_id() which sets pid=1
> when there
> is no "ppid." part to the thread-id description, and if you set "tid=-1"
> then it performs a GDB_ALL_THREAD
> 
> From the vCont spec I also got;
> "p-1 ... means all processes"
> "An action with no thread-id matches all threads."
> "tid=-1 ... matches all threads"
> 
> But it is not very clear what happens if you omit ppid and supply tid as -1.
> 
> What do you think?
> 
> Cheers,
> 
> Luc
> 
>

signature.asc
Description: OpenPGP digital signature