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

["Marcin Kościelnicki" <koriakin@0x04.net>]

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
>