diff mbox series

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

Message ID 20240820082010.1129073-1-felix.willgerodt@intel.com
State New
Headers
Series [1/1] gdb, btrace: Fix clang build |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Felix Willgerodt Aug. 20, 2024, 8:20 a.m. UTC 
  Simon pointed out to me that there are some failures when building with clang,
that were caused by my commit

commit d894edfcc40e63be9b6efa0950c1752f249f16e5
Author: Felix Willgerodt <felix.willgerodt@intel.com>
Date:   Mon Feb 18 13:49:25 2019 +0100

    btrace: Introduce auxiliary instructions.

The errors are:

  CXX    btrace.o
gdb/btrace.c:1203:11: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
 1203 |   return {(CORE_ADDR) insn.ip, (gdb_byte) insn.size,
      |           ^~~~~~~~~~~~~~~~~~~
      |           {                  }
gdb/btrace.c:1218:21: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
 1218 |   btrace_insn insn {btinfo->aux_data.size () - 1, 0,
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                     {                           }
gdb/btrace.c:1323:34: error: variable 'bfun' is uninitialized when used here [-Werror,-Wuninitialized]
 1323 |             handle_pt_aux_insn (btinfo, bfun, *ptw_string, pc);
      |                                         ^~~~
gdb/btrace.c:1236:35: note: initialize the variable 'bfun' to silence this warning
 1236 |       struct btrace_function *bfun;
      |                                   ^
      |                                    = nullptr
3 errors generated.
make[1]: *** [Makefile:1961: btrace.o] Error 1

This fixes those errors.  And switches two casts to C++ casts while we
are at it.
---
 gdb/btrace.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Simon Marchi Aug. 20, 2024, 1:47 p.m. UTC | #1 
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
Felix Willgerodt Aug. 20, 2024, 2:46 p.m. UTC | #2 
> -----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?

Felix


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Simon Marchi Aug. 20, 2024, 3:07 p.m. UTC | #3 
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
Felix Willgerodt Aug. 21, 2024, 7:30 a.m. UTC | #4 
> > Is that ok?
> 
> This version LGTM, thanks.
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon

Thanks, I was just about to push this when I noticed that I messed
up the indentation of the two extra scopes I added.
Can you take another look? I can also post a proper v2 if that is easier
to read.


diff --git a/gdb/btrace.c b/gdb/btrace.c
index 95ff27cc4fe..ff9612dd664 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,30 +1247,38 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 	  break;
 
 	case ptev_enabled:
-	  if (event.status_update != 0)
-	    break;
+	  {
+	    if (event.status_update != 0)
+	      break;
 
-	  if (event.variant.enabled.resumed == 0 && !btinfo->functions.empty ())
-	    {
-	      bfun = ftrace_new_gap (btinfo, BDE_PT_DISABLED, gaps);
+	    if (event.variant.enabled.resumed == 0
+		&& !btinfo->functions.empty ())
+	      {
+		struct btrace_function *bfun
+		  = ftrace_new_gap (btinfo, BDE_PT_DISABLED, gaps);
 
-	      pt_insn_get_offset (decoder, &offset);
+		pt_insn_get_offset (decoder, &offset);
 
-	      warning (_("Non-contiguous trace at instruction %u (offset = 0x%"
-			 PRIx64 ")."), bfun->insn_offset - 1, offset);
-	    }
+		warning
+		  (_("Non-contiguous trace at instruction %u (offset = 0x%"
+		     PRIx64 ")."), bfun->insn_offset - 1, offset);
+	      }
 
-	  break;
+	    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);
+	    pt_insn_get_offset (decoder, &offset);
 
-	  warning (_("Overflow at instruction %u (offset = 0x%" PRIx64 ")."),
-		   bfun->insn_offset - 1, offset);
+	    warning (_("Overflow at instruction %u (offset = 0x%" PRIx64 ")."),
+		     bfun->insn_offset - 1, offset);
 
-	  break;
+	    break;
+	  }
 #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
 	case ptev_ptwrite:
 	  {
@@ -1320,7 +1328,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;
 	  }


Sorry for that,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Simon Marchi Aug. 23, 2024, 5 p.m. UTC | #5 
On 8/21/24 3:30 AM, Willgerodt, Felix wrote:
> @@ -1247,30 +1247,38 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>  	  break;
>  
>  	case ptev_enabled:
> -	  if (event.status_update != 0)
> -	    break;
> +	  {
> +	    if (event.status_update != 0)
> +	      break;

I don't think you need to introduce this scope, the new local variable
is inside the "if" below.  But it's just for consistency with the other
cases, it's fine with me.

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

Simon
Felix Willgerodt Aug. 26, 2024, 7:09 a.m. UTC | #6 
> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 23. August 2024 19:01
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, btrace: Fix clang build
> 
> On 8/21/24 3:30 AM, Willgerodt, Felix wrote:
> > @@ -1247,30 +1247,38 @@ handle_pt_insn_events (struct
> btrace_thread_info *btinfo,
> >  	  break;
> >
> >  	case ptev_enabled:
> > -	  if (event.status_update != 0)
> > -	    break;
> > +	  {
> > +	    if (event.status_update != 0)
> > +	      break;
> 
> I don't think you need to introduce this scope, the new local variable
> is inside the "if" below.  But it's just for consistency with the other
> cases, it's fine with me.
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon

Thanks, I pushed this.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox series

Patch

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 95ff27cc4fe..d62cb63dc46 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)};
 }
@@ -1215,7 +1216,7 @@  handle_pt_aux_insn (btrace_thread_info *btinfo, btrace_function *bfun,
   btinfo->aux_data.emplace_back (std::move (aux_str));
   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,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;
       struct pt_event event;
       uint64_t offset;