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

[Pedro Alves <palves@redhat.com>]

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.

BTW, do we print the not-actually-collected-in-fast-tracepoint registers
as <unavailable>, or as zero's?

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.

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?

I noticed several new functions with missing entry comment.

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

> +
>    agent_look_up_symbols (NULL);
>  }
>  

Thanks,
Pedro Alves