Re: [PATCH] [PR gdb/13808] gdb.trace: Pass tdesc selected in gdbserver to IPA.

Marcin Kościelnicki
2016-02-25T13:39:01Z

On 25/02/16 14:08, Pedro Alves wrote:
> On 02/21/2016 09:17 PM, Marcin KoÅ›cielnicki wrote:
>> If gdbserver and IPA are using different tdesc, they will disagree
>> about 'R' trace packet size. This results in mangled traces.
>>
>> To make sure they pick the same tdesc, gdbserver pokes the tdesc
>> (specified as an index in a target-specific list) into a global
>> variable in IPA. In theory, IPA could find out the tdesc on its
>> own, but that may be complex (in particular, I don't know how to
>> tell whether we have LAST_BREAK on s390 without messing with ptrace),
>> and we'd have to duplicate the logic.
>>
>> Tested on i386 and x86_64. On i386, it fixes two FAILs in ftrace.exp.
>> On x86_64, these failures have been KFAILed - one of them works now,
>> but the other now fails due to an unrelated reason (ugh).
>
> Thanks.
>
> I realized this grows each traceframe's size, probably several
> times fold on x86, given the size/number of vector registers.
> It'd be nice to avoid this.
>
> - One way would be to actually respect the ax's register masks.
>
> - Another way would be to trim recorded regblock sizes up until the
> last recordable register in the tdesc. So if when reading back the
> traceframe, the regblock is shorter than expected, just treat the
> missing registers as unavailable.

Both of these require changing trace frame format - currently the
regblock size is shared for all tracepoints in file (ie. both trap and
fast tracepoints), but #1 sounds like the way to go.
>
> BTW, do we print the not-actually-collected-in-fast-tracepoint registers
> as <unavailable>, or as zero's?

Zeros - there's no way in the trace frame format to mark them unavailable.
>
> It'd also be nice to be able to actually collect the now tdesc-accessible
> registers, but that runs into the register size problems discussed earlier.
> Plus, we probably wouldn't want to _always_ collect everything.

Well, they do work for trap tracepoints... but yeah.
>
> I haven't analyzed the actual traceframe size impact, but I guess we can
> consider those orthogonal optimization problems, as without this fix, things
> just don't work.
>
> One thing that wasn't super obvious in the patch is why do several different
> target descriptions in gdbserver map to a single description in the IPA?

They're basically the same tdesc's for different ABIs (i386 vs x32 vs
x64). Since IPA is single-ABI, it's not necessary for the enum to
encode the ABI. I'll add a comment to linux-x86-tdesc.h.
>
> I noticed several new functions with missing entry comment.

OK, will fix these.
>
>> +const struct target_desc *
>> +get_ipa_tdesc (int idx)
>> +{
>> + switch (idx) {
>> + case X86_TDESC_MMX: /* Should not happen. */
>
> ...
>
>> +const struct target_desc *
>> +get_ipa_tdesc (int idx)
>> +{
>> + switch (idx) {
>
> '{' goes on next line (and then reindent).
>
>
>
>> @@ -328,6 +332,14 @@ tracepoint_look_up_symbols (void)
>> }
>> }
>>
>> + /* Tell IPA about the correct tdesc. */
>> + if (write_inferior_integer (ipa_sym_addrs.addr_ipa_tdesc_idx,
>> + target_get_ipa_tdesc_idx ()))
>> + {
>> + internal_error (__FILE__, __LINE__,
>> + "Error setting ipa_tdesc_idx variable in lib");
>> + }
>
> Failure to write to the inferior should never be an internal error.
> The inferior might vanish, e.g., because it was SIGKILL'ed from
> outside gdbserver.

Fair enough. The file is littered with internal errors in such cases
though (I just copied one of them), what should be done for these?
>
>> +
>> agent_look_up_symbols (NULL);
>> }
>>
>
> Thanks,
> Pedro Alves
>