This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]