[PATCH v5 4/7] start/stop btrace with coresight etm and collect etm buffer on linux os

[Zied Guermazi]

hi Markus

here is the current status of the modifications, they will be published 
in the next version of the patchset

/Zied

On 30.04.21 11:01, Metzger, Markus T wrote:
> Hello Zied,
>
>> All that perf_event setup code looks very similar to PT.  Could we share
>> all that?
>> [Zied] Yes, there is a lot of similarity in getting the file descriptor out of the syscall and the mmaped file etc.. the difference is in the attributes settings and later on the in setting btrace_tinfo_pt or btrace_tinfo_etm etc... which requires passing a lot of parameters. Therefore I opted to have two dedicated functions.
> They agree on the fields related to perf so we can capture those
> in a separate structure.
>
> And we could have the caller fill in the perf attributes.  The rest
> should be identical.
>
>> +  conf->etm_trace_params = new std::vector<cs_etm_trace_params>;
>> +  for (int i = 0; i < conf->cpu_count; i++)
>> +    {
>> +      fill_etm_trace_params (&etm_trace_params,i);
>>
>> Can this throw?  We just allocated memory.
>> [Zied] Yes, new can throw a std::bad_alloc. If we would like to have a graceful continuation of gdb when we fail in new, then we will have to catch it. Otherwise we ignore it and we let it crash gdb.
>> Which behaviour do we want to have here?
> I was asking about fill_etm_trace_params ().  We're not using unique_ptr
> to store the vector so it would be leaked on an exception.
[Zied] yes, it may be leaked,
> Do we need to allocate that vector on the heap, at all?

[Zied] this is vector of structure with union. I opted to allocate it on 
the heap instead of the stack to avoid c++ restrictions. in fact in C++, 
unions may not contain classes with (non-trivial) constructors or 
destructors, which is the case here.

gcc fires compilation errors if I put the vector in the data structure 
itself (std::vector<struct cs_etm_trace_params> etm_trace_params;), even 
if I define the constructor, destructor and the= operator.

unique_ptr or shared_ptr made the situation even worst.

>> +/* Branch trace target information for ARM CoreSight ETM Trace.  */
>> +struct btrace_tinfo_etm
>> +{
>> +  /* The Linux perf_event configuration for collecting the branch trace.  */
>> +  struct perf_event_attr attr;
>> +
>> +  /* The perf event file.  */
>> +  int file;
>> +
>> +  /* The perf event configuration page.  */
>> +  volatile struct perf_event_mmap_page *header;
>> +
>> +  /* The trace perf event buffer.  */
>> +  struct perf_event_buffer etm;
>> +};
>>
>> Maybe we can share that, too.
>> [Zied] Yes. This should be basically possible, we have to rename "struct perf_event_buffer etm/pt"  to "struct perf_event_buffer buffer", and change its occurrences. shall we go for it in this patch set or in a different one?
> I think BTS is using the same fields, too, so we could change this
> also in a separate patch.  The configuration is different between
> PT and BTS, though.  BTS is collected in the perf buffer, whereas
> PT is collected in the AUX buffer and we ignore the perf buffer.unique
> ETM seems similar to PT in this regard.
>
> I'm fine either way.
>
> 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