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

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

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 28. Juni 2022 11:10
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH v5 02/10] btrace: Enable auxiliary instructions in record
> instruction-history.
> 
> 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".

> 
> >This patch is in preparation for the new ptwrite feature, which is based on
> >auxiliary instructions.
> >---
> > gdb/disasm-flags.h  |  1 +
> > gdb/doc/gdb.texinfo |  3 +++
> > gdb/record-btrace.c | 14 ++++++++++++++
> > gdb/record.c        |  5 +++++
> > 4 files changed, 23 insertions(+)
> 
> >+      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);

I can't really find a good way.

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