RE: [PATCH 02/11] btrace: Change parameters to use btrace_thread_info.

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Wiederhake, Tim
> Sent: Friday, February 17, 2017 2:26 PM
> To: gdb-patches@sourceware.org
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH 02/11] btrace: Change parameters to use btrace_thread_info.

Hello Tim,

> gdb/ChangeLog:
> 	* btrace.c (ftrace_new_function, ftrace_fixup_caller, ftrace_new_call,
> 	ftrace_new_tailcall, ftrace_find_caller, ftrace_find_call,
> 	ftrace_new_return, ftrace_new_switch, ftrace_new_gap,
> 	ftrace_update_function, ftrace_update_insns, ftrace_connect_bfun,
> 	ftrace_connect_backtrace, ftrace_bridge_gap,
> btrace_compute_ftrace_bts,
> 	ftrace_add_pt, btrace_compute_ftrace_pt): Changed to use struct
> 	btrace_thread_info * as parameter. Adjusted comments where

Two spaces after '.'.


>    bfun->msym = mfun;
> @@ -258,7 +259,8 @@ ftrace_update_caller (struct btrace_function *bfun,
>  /* Fix up the caller for all segments of a function.  */
> 
>  static void
> -ftrace_fixup_caller (struct btrace_function *bfun,
> +ftrace_fixup_caller (struct btrace_thread_info *btinfo,
> +		     struct btrace_function *bfun,
>  		     struct btrace_function *caller,
>  		     enum btrace_function_flag flags)
>  {

I assume the new argument will be used in later patches.


> @@ -275,18 +277,17 @@ ftrace_fixup_caller (struct btrace_function *bfun,
>  }
> 
>  /* Add a new function segment for a call.
> -   CALLER is the chronologically preceding function segment.
>     MFUN and FUN are the symbol information we have for this function.  */

Please add 'at the end of the trace' like you did for ftrace_new_function.


>    ftrace_debug (bfun, "new call");
> @@ -295,18 +296,17 @@ ftrace_new_call (struct btrace_function *caller,
>  }
> 
>  /* Add a new function segment for a tail call.
> -   CALLER is the chronologically preceding function segment.
>     MFUN and FUN are the symbol information we have for this function.  */

Same here.  More instances below.

 
> -/* Add the instruction at PC to BFUN's instructions.  */
> +/* Add the instruction at PC to the instructions of the current function call
> +   segment at the end of the trace.  */

Just "function segment" without the "call".


> @@ -1017,7 +1029,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
>  	      break;
>  	    }
> 
> -	  btinfo->end = ftrace_update_function (btinfo->end, pc);
> +	  btinfo->end = ftrace_update_function (btinfo, pc);
>  	  if (btinfo->begin == NULL)
>  	    btinfo->begin = btinfo->end;

It would be cleaner if we moved this BTINFO->BEGIN update into ftrace_new_function.

Patch 9 will remove BTINFO->BEGIN/END but this would allow us to get rid of most of
the btrace_function pointers before patch 5.  We may not need to squash everything,
which may help later bisects.

We really only need btrace_function pointers for maintaining the list of gaps and the
function level adjustment, and for fixing up callers.  The latter two don't generate new
functions, so we're safe.  And for gaps, we have to switch to indices, anyway.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928