[PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history.

[Metzger, Markus T]

Hello Felix,

>> >Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
>> >is encountered in the instruction-history.  Printing is active by default,
>> >it can be silenced with the /a modifier.
>>
>> I find it a bit strange that /a disables some output instead of enabling it.
>> Should we change the default?  I'd probably always use it, but then, I
>> also always supply /cli to 'record function-call-history'.
>
>I don't find it strange. /p and /f already disables some output:
>
>(gdb) help record instruction-history
>Print disassembled instructions stored in the execution log.
>With a /m or /s modifier, source lines are included (if available).
>With a /r modifier, raw instructions in hex are included.
>With a /f modifier, function names are omitted.
>With a /p modifier, current position markers are omitted.
>
>I think we should make the default the thing GDB users "would want".

Hmmm, OK, it does fit.  /p was added later so I had to make it omit things
to preserve backwards compatibility.  But /f was there from the beginning.

I'm OK either way; let's hear what Eli thinks.


>> >+      else if (insn->iclass == BTRACE_INSN_AUX)
>> >+	{
>> >+	  if ((flags & DISASSEMBLY_OMIT_AUX_INSN) != 0)
>> >+	    continue;
>> >+
>> >+	  uiout->field_fmt ("insn-number", "%u", btrace_insn_number (&it));
>> >+	  uiout->text ("\t");
>> >+	  uiout->spaces (3);
>> >+	  uiout->text ("[");
>> >+	  uiout->field_fmt (
>> >+	      "aux-data", "%s",
>> >+	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());
>>
>> The formatting is a bit unusual.
>
>Agreed. How should I format it instead? The last line is the problem.
>
>Should it be this:
>
>	  uiout->field_fmt
>	     ("aux-data", "%s",
>	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());
>
>Or should it be something like this:
>
>	  uiout->field_fmt ("aux-data", "%s",
>			    it.btinfo->aux_data.at
>				(insn->aux_data_index).c_str ());
>
>Or I can also add a temporary std:string:
>
>	  std::string data
>	    = it.btinfo->aux_data.at (insn->aux_data_index).c_str ();
>	  uiout->field_fmt ("aux-data", "%s", data);

No need to construct a std::string; you may just use a const char * or
a reference to the std::string that at () returns.

All of the above are acceptable, I think.

regards,
markus.

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