RE: [PATCH 03/10] btrace: Enable auxiliary instructions in record function-call-history.

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

On 6/4/19 2:35 PM, Metzger, Markus T wrote:
>Hello Felix,
>
>> Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX is 
>> encountered in the function-call-history.  Printing is active by 
>> default, it can be silenced with the /s modifier.

Markus> The /s modifier is used in 'record instruction-history' to mean 'interleave sources'.  I guess we'd want this behavior there, as well.  And we'd also want the same modifier for both commands.

Done

>> +static void
>> +btrace_print_aux_insn (struct ui_out *uiout,
>> +		       const struct btrace_function *bfun,
>> +		       const struct btrace_thread_info *btinfo) {
>> +  for (const auto &i : bfun->insn)
> >+    {
> >+      if (i.iclass == BTRACE_INSN_AUX)
> >+	{
> >+	  uiout->text ("\t\t[");

Markus> We print the number and a single tab for gaps.  I'd do the same here.

I don't think we want that. I had that in an earlier revision, but we decided against it back then.
AUX insns are not function segments like gaps. There could be plenty of AUX insns in one function,
increasing the number by a lot. We would need to manipulate the bfun->number, as the aux
insn is a insn inside bfun, but a gap is an empty bfun.

Currently it looks like this:

record function-call-history 1,4
1       main
2       ptwrite64
                [payload: 0x42]
3       main
4       ptwrite32
               [payload: 0x43]
               [payload: 0x33]
5       main

This makes it consistent with the silenced version:

record function-call-history /a 1,4
1       main
2       ptwrite64
3       main
4       ptwrite32
5       main

>
>> +	  uiout->field_fmt ("aux-insn", "%s",
>
>Should this be spelled "aux-data" to align with the naming of the field?
>>
>>+			    btinfo->aux_data[i.aux_data_index].c_str ());
>>+	  uiout->text ("]\n");
>>+	}
>>+    }
>>+}
>>+
>> /* Disassemble a section of the recorded function trace.  */
>>
>> static void
>> @@ -1214,6 +1231,10 @@ btrace_call_history (struct ui_out *uiout,
>> 	}
>> 
>>        uiout->text ("\n");
>> +
>> +      if (((flags & RECORD_DONT_PRINT_AUX) == 0)
>> +	  && ((bfun->flags & BFUN_AUX_DECODED) != 0))
>> +	btrace_print_aux_insn(uiout, bfun, btinfo);

Markus> This ignores the 'record function-call-history-size' setting.


Isn't the size handled by the loop? The current behaviour looks fine to me:

(gdb) set record function-call-history-size 2
(gdb) record function-call-history 1
1    	main
2    	ptwrite64
		[payload: 0x42]
(gdb) set record function-call-history-size 3
(gdb) record function-call-history 1         
1	main
2	ptwrite64
		[payload: 0x42]
3	main

I don't think we want to start cutting the auxiliary data from a function.

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928