RE: [PATCH 10/10] btrace: Extend event decoding for ptwrite.

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

On 6/4/19 2:35 PM, Metzger, Markus T wrote:
>> @@ -1244,6 +1244,53 @@ handle_pt_insn_events (struct 
>> btrace_thread_info *btinfo,
>> 		   bfun->insn_offset - 1, offset);
>> 
>>  	  break;
>> +
>> +	case ptev_ptwrite:
>> +	  {
>> +	    uint64_t *ip = nullptr;
>> +	    gdb::unique_xmalloc_ptr<char> ptw_string = nullptr;
>> +	    struct btrace_insn ptw_insn;
>> +
>> +	    /* Lookup the ip.  */
>> +	    if (event.ip_suppressed == 0)
>> +	      ip = &event.variant.ptwrite.ip;
>> +	    else if (!btinfo->functions.empty ()
>> +		     && !btinfo->functions.back ().insn.empty ())
>> +	      ip = &btinfo->functions.back ().insn.back ().pc;

Markus> Libipt will provide the PTWRITE instruction's IP.  We may get PTW events without IP but those wouldn't bind to the previous instruction - or libipt would have filled in the event IP.
Markus > If EVENT.IP_SUPPRESSED is set, there is no IP.

This is a fallback to get the IP from somewhere. In the case that ip is suppressed - but recording worked fine - this should still provide the user with the right IP. That said, I am fine with removing it, it is probably not a common occurrence.

>> +	    else
>> +	      /* This should never happen.  */
>> +	      error (_("Failed to obtain the PC of the current 
>> +instruction."));

Markus> We explicitly allow a nullptr IP in the callback function.

Right removed.

>> +
>> +	    try
>> +	      {
>> +		if (btinfo->ptw_callback_fun != nullptr)
>> +		  ptw_string = btinfo->ptw_callback_fun (
>> +					  &event.variant.ptwrite.payload, ip,
>> +					  btinfo->ptw_listener);
>> +	      }
>> +	    catch (const gdb_exception_error &error)
>> +	      {
>> +		warning (_("Failed to call ptwrite listener."));

Markus> This indicates a bug in the user's ptwrite-listener object.  Should we print a backtrace and fail with an error?

Python code errors are actually already caught and printed in py-record-btrace.c already. I think the idea was some protection against possible errors with the callback registration or other weird GDB problems. I removed the try/catch, as I think that shouldn't really happen, it is all GDB internal.

>> +	      }
>> +
>> +	    if (ptw_string == nullptr)
>> +	      break;
>> +
>> +	    btinfo->aux_data.emplace_back (ptw_string.get ());
>> +
>> +	    /* Update insn list with ptw payload insn.  */
>> +	    ptw_insn.aux_data_index = btinfo->aux_data.size () - 1;
>> +	    ptw_insn.flags = 0;

Markus> Can we preserve the SPECULATIVE flag?

Yes, done now.

>> +When recording multithreaded programs, @value{GDBN} creates a new 
>> +copy of

Markus>Also when recording a single threaded program.  Maybe just remove this part.

Done

>> the
>> +listener function for each thread to allow for independent internal states.
>> +There is currently no support for registering different listeners for 
>> +different threads. The listener can however distinguish between 
>> +multiple threads

Markus> You write 'currently'.  Are there plans to support different listeners for different threads?

I removed 'currently'.

>> +with the help of @code{gdb.selected_thread().global_num} or similar.
>> +
>> +@exdent For example:
>> +@smallexample
>> +(gdb) python-interactive
>> +>>> def my_listener(payload, ip):
>> +...     if gdb.selected_thread().global_num == 1:
>> +...         return "payload: @{0@}, ip: @{:#x@}".format(payload, ip)
>> +...     else:
>> +...         return None
>> +...

Markus> An example where the listener accumulates payloads would be nice.

I rewrote this one to include a counter that is incremented. For the accumulated payloads you would really need
to show the payload values and therefore the source code for it to make sense. This made the example too long.

>> +++ b/gdb/testsuite/gdb.btrace/ptwrite.c
>> @@ -0,0 +1,40 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2018 Free Software Foundation, Inc.

Markus> It's 2019, meanwhile.

Done

> +int
> +main (void)
> +{
> +

>Spurious empty line.

Done

>> +
>> +if { [skip_btrace_ptw_tests] } {
>> +    unsupported "Hardware does not support ptwrite instructions."
>> +    return -1
>> +}
>> +
>> +set comp_flags "additional_flags=-I${srcdir}/.."

Markus> If we need this only in the COMPILE case, I'd move it there; or just inline it.

Removed, was a left over from earlier revisions.

>> +if [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] {
>> +    unsupported "Compiler does not support ptwrite."

Markus> I don't think we need an 'unsupported' here.  We should get 'failed to prepare', already.

Fixed

> +### 1. Default testrun
> +
> +# Setup recording

Markus> You can use with_test_prefix to avoid repeating the prefix in each test.

Using with_test_prefix would have resulted in some ugly formatting with 
the multi-line test statements. I preferred to remove the prefix from the 
tests that don't need it to have a unique name.


>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp 
>> index c703a7e6338..ba7ccfd46ba 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -2874,6 +2874,98 @@ gdb_caching_proc skip_btrace_pt_tests {
>>      return $skip_btrace_tests
>>  }
>> 
>> +# Run a test on the target to see if it supports ptwrite instructions.
>> +# Return 0 if so, 1 if it does not.  Based on 'check_vmx_hw_available'
>> +# from the GCC testsuite.
>> +
>> +gdb_caching_proc skip_btrace_ptw_tests {
>> +    global srcdir subdir gdb_prompt inferior_exited_re
>> +
>> +    set me "skip_ptw_tests"
>> +
>> +    # Set up, compile, and execute a test program.
>> +    # Include the current process ID in the file names to prevent conflicts
>> +    # with invocations for multiple testsuites.
>> +    set src [standard_temp_file ptw[pid].c]
>> +    set exe [standard_temp_file ptw[pid].x]
>> +
>> +    gdb_produce_source $src {
>> +	#include <stdbool.h>
>> +	#include <stdint.h>
>> +	#include <stdio.h>
>> +	#include "x86-cpuid.h"
>> +
>> +	#define bit_PTW (1 << 4) 
>> +
>> +	bool
>> +	have_ptw ()
>> +	{
>> +	  unsigned int eax, ebx, ecx, edx;
>> +
>> +	  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
>> +	      return false;
>> +
>> +	  if ((ecx & bit_OSXSAVE) != bit_OSXSAVE)
>> +	      return false;
>> +
>> +	  if (__get_cpuid_max (0, NULL) < 0x14)
>> +	      return false;
>> +
>> +	  __cpuid_count (0x14, 0, eax, ebx, ecx, edx);
>> +
>> +	  return (ebx & bit_PTW) == bit_PTW;
>> +	}
>> +
>> +	int
>> +	main ()
>> +	{
>> +	  return have_ptw ();
>> +	}
>> +
>> +    }
>> +
>> +    verbose "$me:  compiling testfile $src" 2
>> +
>> +    set test_flags [list additional_flags=-I${srcdir}/../nat]
>> +
>> +    set compile_flags [concat {debug nowarnings quiet} ${test_flags}]
>> +    set lines [gdb_compile $src $exe executable $compile_flags]
>> +
>> +    if ![string match "" $lines] then {
>> +	verbose "$me:  testfile compilation failed, returning 1" 2
>> +	file delete $src
>> +	return 1
>> +    }
>> +
>> +    # No error message, compilation succeeded so now run it via gdb.
>> +    gdb_exit
>> +    gdb_start
>> +    gdb_reinitialize_dir $srcdir/$subdir
>> +    gdb_load $exe
>> +    if ![runto_main] {
>> +	file delete $src
>> +	return 1
>> +    }
>> +    file delete $src
>> +
>> +    # In case of an unexpected output, we return 2 as a fail value.
>> +    set skip_ptw_tests 2
>> +    gdb_test_multiple "print (int) have_ptw ()" "check ptw support" {
>> +	-re ".. = 1\r\n$gdb_prompt $" {
>> +	    set skip_ptw_tests 0
>> +	}
>> +	-re ".. = 0\r\n$gdb_prompt $" {
>> +	    set skip_ptw_tests 1
>> +	}
>> +    }
>> +
>> +    gdb_exit
>> +    remote_file build delete $exe
>> +
>> +    verbose "$me:  returning $skip_ptw_tests" 2
>> +    return $skip_ptw_tests

Markus> We still don't know if the OS supports it, do we?

True, I rewrote the whole function to do it similar to other functions we added in the meanwhile,
and added checks whether "/sys/bus/event_source/devices/intel_pt/caps/ptwrite" is set.

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