[PATCH 1/1] gdb, btrace: Fix clang build

[Simon Marchi]

On 8/20/24 10:46 AM, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Dienstag, 20. August 2024 15:47
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
>> Subject: Re: [PATCH 1/1] gdb, btrace: Fix clang build
>>
>> On 8/20/24 4:20 AM, Felix Willgerodt wrote:
>>> @@ -1233,7 +1234,7 @@ handle_pt_insn_events (struct btrace_thread_info
>> *btinfo,
>>>  #if defined (HAVE_PT_INSN_EVENT)
>>>    while (status & pts_event_pending)
>>>      {
>>> -      struct btrace_function *bfun;
>>> +      struct btrace_function *bfun = nullptr;
>>
>> In my version, I just removed the bfun parameter from the
>> handle_pt_aux_insn function, because it appears to be unused.  We pass a
>> bfun value to handle_pt_aux_insn, but it immediately overwrites it.
>>
>> Initializing bfun here is unnecessary, because other users do assign it
>> properly.  In fact, the declaration could be moved to each point where bfun is
>> assigned (i.e. have two separate declarations).  That helps show the
>> uses of bfun are unrelated.
>>
>> Simon
> 
> Hi Simon,
> 
> I did see that you did that, but I didn't like the unused declaration in case of
> ptwrite. We already have a patch series lined up for upstreaming that will
> add a lot more cases like ptwrite here.
> 
> I would go with what you proposed and make each case use their own
> declaration:
> 
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 95ff27cc4fe..7d8be3314e8 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1200,7 +1200,8 @@ pt_btrace_insn_flags (const struct pt_insn &insn)
>  static btrace_insn
>  pt_btrace_insn (const struct pt_insn &insn)
>  {
> -  return {(CORE_ADDR) insn.ip, (gdb_byte) insn.size,
> +  return {{static_cast<CORE_ADDR> (insn.ip)},
> +	  static_cast<gdb_byte> (insn.size),
>  	  pt_reclassify_insn (insn.iclass),
>  	  pt_btrace_insn_flags (insn)};
>  }
> @@ -1209,13 +1210,13 @@ pt_btrace_insn (const struct pt_insn &insn)
>  /* Helper for events that will result in an aux_insn.  */
>  
>  static void
> -handle_pt_aux_insn (btrace_thread_info *btinfo, btrace_function *bfun,
> -		    std::string &aux_str, CORE_ADDR ip)
> +handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
> +		    CORE_ADDR ip)
>  {
>    btinfo->aux_data.emplace_back (std::move (aux_str));
> -  bfun = ftrace_update_function (btinfo, ip);
> +  struct btrace_function *bfun = ftrace_update_function (btinfo, ip);
>  
> -  btrace_insn insn {btinfo->aux_data.size () - 1, 0,
> +  btrace_insn insn {{btinfo->aux_data.size () - 1}, 0,
>  		    BTRACE_INSN_AUX, 0};
>  
>    ftrace_update_insns (bfun, insn);
> @@ -1233,7 +1234,6 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>  #if defined (HAVE_PT_INSN_EVENT)
>    while (status & pts_event_pending)
>      {
> -      struct btrace_function *bfun;
>        struct pt_event event;
>        uint64_t offset;
>  
> @@ -1247,12 +1247,14 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>  	  break;
>  
>  	case ptev_enabled:
> +	{
>  	  if (event.status_update != 0)
>  	    break;
>  
>  	  if (event.variant.enabled.resumed == 0 && !btinfo->functions.empty ())
>  	    {
> -	      bfun = ftrace_new_gap (btinfo, BDE_PT_DISABLED, gaps);
> +	      struct btrace_function *bfun
> +		= ftrace_new_gap (btinfo, BDE_PT_DISABLED, gaps);
>  
>  	      pt_insn_get_offset (decoder, &offset);
>  
> @@ -1261,9 +1263,12 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>  	    }
>  
>  	  break;
> +	}
>  
>  	case ptev_overflow:
> -	  bfun = ftrace_new_gap (btinfo, BDE_PT_OVERFLOW, gaps);
> +	{
> +	  struct btrace_function *bfun
> +	    = ftrace_new_gap (btinfo, BDE_PT_OVERFLOW, gaps);
>  
>  	  pt_insn_get_offset (decoder, &offset);
>  
> @@ -1271,6 +1276,7 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>  		   bfun->insn_offset - 1, offset);
>  
>  	  break;
> +	}
>  #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
>  	case ptev_ptwrite:
>  	  {
> @@ -1320,7 +1326,7 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>  	    if (!ptw_string.has_value ())
>  	      *ptw_string = hex_string (event.variant.ptwrite.payload);
>  
> -	    handle_pt_aux_insn (btinfo, bfun, *ptw_string, pc);
> +	    handle_pt_aux_insn (btinfo, *ptw_string, pc);
>  
>  	    break;
>  	  }
> 
> Is that ok?

This version LGTM, thanks.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon