Re: [PATCH v6 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant

[Zied Guermazi <zied.guermazi@trande.de>]

Hello Markus,

please see the comments below.

/Zied

On 13.04.22 09:00, Metzger, Markus T wrote:
>
> Hello Zied,
>
> +
> +static void
> +ftrace_remove_last_insn (struct btrace_thread_info *btinfo)
> +{
> +  /* If we didn't have a function, we return.  */
> +  if (btinfo->functions.empty ())
> +    return;
> Should this be an error?
> +
> +  struct btrace_function *bfun = &btinfo->functions.back ();
> +  /* If we had a gap before, we return.  */
> +  if (bfun->errcode != 0)
> +    return;
> In which case can we have a gap?
>
> [Zied] this is to make sure that following calls are safe to be executed.
>
> +
> +  if (!bfun->insn.empty ())
> +    bfun->insn.pop_back ();
> +  else
> +    {
> +      /* A valid function must have at least one instruction.  */
> +      internal_error (__FILE__, __LINE__,
> +                 _("Attempt to remove last instruction"
> +                   "from an empty function"));
> We just removed an instruction in the then statement, which
> could well result in BFUN->INSN to become empty().
> The statement about valid functions above may be a bit too
> generic.  Also, ftrace_new_* () may create empty function
> segments.
> Isn't it rather that in our use-case, the caller knows that the
> function segment cannot be empty?
>
> [Zied] yes, we are sure that we have at least the last executed 
> instruction that raised the breakpoint execption.
>
> Would it make more sense to simply do ‘bfun->insn.pop_back ()’ inside 
> the caller?
>
> Apparently, the caller knows that there must be an instruction so it 
> could safely remove it.  If we turn this into a separate function, we 
> need to handle the general case.
>
[Zied] what will happens in the case  of a gap proceeding current 
instruction, the function is still not created at this point, isn't it?

to be sure that none tries to use the function as a generic one, we can 
remove it and put the code in the caller function. It is called only 
once. will this be fine from your point of view?

> +    case ocsd_isa_custom:
> +      return BTRACE_INSN_FLAG_ISA_CUSTOM;
> +
> +    case ocsd_isa_unknown:
> +      return BTRACE_INSN_FLAG_ISA_UNKNOWN;
> +
> +    default:
> +      internal_error (__FILE__, __LINE__,
> +                 _("Undefined elem->isa value returned by OpenCsd."));
> This internal error kills GDB.  Should this be a normal error that just
> stops trace processing for this unknown ISA?
>
> [Zied] since I listed all possible values that may get returned by the 
> decoder, the default case means that gdb and the decoder are doing 
> something wrong and this should be handled as a fatal error.
>
> The error is fatal for trace decode but not for GDB.  I think we 
> should use error () instead of internal_error () for all those cases.
>
[Zied] done.
> Even for normal trace off/on pairs, we'd want a gap if PC is moving.
>
> [Zied] yes, but I do not have here a mean for checking if the PC has 
> moved. The pc field is not valid in this kind of elements
>
> In libipt, I compare the IPs of the stop and the resume trace packets. 
> If something like that doesn’t work in your case I won’t insist.
>
[Zied] unfortunately such a feature is not available in OpenCsd
>
> 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
>
-- 

*Zied Guermazi*
founder

Trande GmbH
Leuschnerstraße 2
69469 Weinheim/Germany

Mobile: +491722645127
mailto:zied.guermazi@trande.de <mailto:zied.guermazi@trande.de>

*Trande GmbH*
Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209 
- Geschäftsführung: Zied Guermazi

*Confidentiality Note*
This message is intended only for the use of the named recipient(s) and 
may contain confidential and/or privileged information. If you are not 
the intended recipient, please contact the sender and delete the 
message. Any unauthorized use of the information contained in this 
message is prohibited.