Re: [PATCH 2/5] btrace: support Intel(R) Processor Trace

[Pedro Alves <palves@redhat.com>]

On 06/30/2015 03:54 PM, Metzger, Markus T wrote:

>>> +  decoder = pt_insn_alloc_decoder (&config);
>>> +  if (decoder == NULL)
>>> +    error (_("Failed to allocate the Intel(R) Processor Trace decoder."));
>>> +
>>> +  TRY
>>> +    {
>>> +      struct pt_image *image;
>>> +
>>> +      image = pt_insn_get_image(decoder);
>>> +      if (image == NULL)
>>> +	error (_("Failed to configure the Intel(R) Processor Trace decoder."));
>>> +
>>> +      errcode = pt_image_set_callback(image,
>> btrace_pt_readmem_callback, NULL);
>>> +      if (errcode < 0)
>>> +	error (_("Failed to configure the Intel(R) Processor Trace decoder: "
>>> +		 "%s."), pt_errstr (pt_errcode (errcode)));
>>> +
>>> +      ftrace_add_pt (decoder, &btinfo->begin, &btinfo->end, &level,
>>> +		     &btinfo->ngaps);
>>> +    }
>>> +  CATCH (error, RETURN_MASK_ALL)
>>> +    {
>>> +      /* Indicate a gap in the trace if we quit trace processing.  Errors were
>>> +	 already logged before.  */
>>
>> What does this "already logged before" mean?  AFAICS, the errors thrown
>> in the TRY branch are just swallowed here.  Did you mean to rethrow them?
>> Otherwise I'm not seeing the point in throwing them in the first place.
> 
> This means that decode errors are already represented as gaps in the trace.
> When the trace is printed, the error at a trace gap is printed.
> 
> This code is now handling a user interrupt, which is also represented
> as a gap at the very end of the trace.
> 
> This reference to decode errors is maybe more confusing than helpful.
> I'll remove it.

I still don't get why throw the errors in the TRY branch:

      if (image == NULL)
	error (_("Failed to configure the Intel(R) Processor Trace decoder."));

      errcode = pt_image_set_callback(image, btrace_pt_readmem_callback, NULL);
      if (errcode < 0)
	error (_("Failed to configure the Intel(R) Processor Trace decoder: "
		 "%s."), pt_errstr (pt_errcode (errcode)));

... if they're just dropped on the catch brock.  Shouldn't those
be rethrown?  The CATCH block you had does not do that.  And it
neither rethrows the ctrl-c that generates the RETURN_QUIT:

+  CATCH (error, RETURN_MASK_ALL)
+    {
+      /* Indicate a gap in the trace if we quit trace processing.  Errors were
+	 already logged before.  */
+      if (error.reason == RETURN_QUIT && btinfo->end != NULL)
+	{
+	  btinfo->end = ftrace_new_gap (btinfo->end, BDE_PT_USER_QUIT);
+	  btinfo->ngaps++;
+	}
+    }

So shouldn't that be:

  CATCH (error, RETURN_MASK_ALL)
    {
      /* Indicate a gap in the trace if we quit trace processing.  Errors were
	 already logged before.  */
      if (error.reason == RETURN_QUIT && btinfo->end != NULL)
	{
	  btinfo->end = ftrace_new_gap (btinfo->end, BDE_PT_USER_QUIT);
	  btinfo->ngaps++;
	}

+     throw_exception (error);
    }

?

> I'm just adding new elements and attributes.  I thought I'd bump the version
> since there are new features.  Should I leave it at version 1.0?

Yes.  That way record bts with old gdb should still work with
new gdbserver.  (Please give that a try to make sure it actually
still works.)

Thanks,
Pedro Alves