Re: [PATCH] Upload TSVs in extend-remote mode

[Pedro Alves <palves@redhat.com>]

On 06/22/2013 03:23 AM, Yao Qi wrote:
> On 06/21/2013 02:34 AM, Pedro Alves wrote:
>>> The fix to this problem is to upload TSV in
>>>> extended_remote_create_inferior_1.
>> This fixes "run", but I imagine it leaves "attach" broken?
>>
>
> You are right, :(.  I add a test that tsvs are uploaded after attach,
> to make sure this patch handles "attach" as well.

Thanks.

>
>>>> Another fix could be that move the code uploading TSVs earlier in
>>>> remote_start_remote before sending packet '?'.  I don't choose this
>>>> approach because I'd like GDB to upload TSVs when inferior starts run,
>>>> instead of when GDB connects to the stub.
>> Why would you like that?  (Not saying it might not make sense).
>>
>
> Both of them were right to me at that moment and I spent some time on
> choosing one of them.  I thought it might be "better" to postpone the
> uploading from the point of connect to the point of run.  In the
> updated patch, I move the code uploading tsvs earlier in
> remote_start_remote before handling the reply of '?'.

Yeah, I agree the choice isn't obvious, and I wasn't objecting to
your choice.  For now, I was just looking at recording the reasons
for our preferences.  I suspect we'll need to revisit this once we have
multi-inferior-aware tracing, or when gdb supports multi-target.

> Thanks for the pointer.  When I was moving this code, I had a question
> in my mind that how to test non-gdbserver stubs if they also have
> predefined tsvs.  Some stubs don't have predefined tsvs, while some
> stubs have different ones.  We can extend the testsuite to read
> predefined tsvs from the board info, and the different predefined tsvs
> can be set in different board files for different stubs.  In the
> updated patch below, the following line is added in all gdbserver board
> files,

Good idea.

>
> +  /* Upload TSVs regardless the target is running or not.  The remote

... regardless of whether the ...

> +     stub, such as GDBserver, may have some predefined or builtin TSVs,
> +     even if the target is not running.  GDB has to upload them too when
> +     connects to the remote stub.  */

"when it connects", "when connecting", or better "GDB has to upload them
too in that case".

But I'd drop that last sentence entirely.

> +  if (remote_get_trace_status (current_trace_status ()) != -1)
> +    {
> +      struct uploaded_tsv *uploaded_tsvs = NULL;
> +
> +      remote_upload_trace_state_variables (&uploaded_tsvs);
> +      merge_uploaded_trace_state_variables (&uploaded_tsvs);
> +    }
> +

> +# If there are predefined TSVs, test these predefined TSVs are correctly
> +# uploaded.
> +if [target_info exists gdb,predefined_tsv] {
> +    set tsv [target_info gdb,predefined_tsv]
> +
> +    # Restart.
> +    clean_restart ${binfile}
> +
> +    if ![runto_main] then {
> +	fail "Can't run to main"
> +	return
> +    }
> +
> +    # Test predefined TSVs are uploaded.
> +    gdb_test "info tvariables" ".*${tsv}.*" "check uploaded tsv"
> +}

I think there could be an else branch here, that did "info tvariables"
and made sure nothing comes out.  Whoever tests against a target that
has builtin tsvs that is not GDBserver, would then notice the FAIL and
update the board file.  I'd suggest in the same vein, making ".*${tsv}.*"
less lax.  Otherwise, I'm afraid this is the sort of board setting that
will just go silently unnoticed.

WDYT?

The patch is okay with or without that change.

In the future, we should be thinking of coming up with a new
boards/gdbserver.exp file with these GDBserver specific
settings that native-gdbserver.exp, native-extended-gdbserver.exp,
etc. would source, instead of duplicating these bits across
several boards.

-- 
Pedro Alves