[PATCH] xtensa: Properly strdup string when building reggroup

[PATCH] xtensa: Properly strdup string when building reggroup

[Stafford Horne]

I noticed this while looking at the reggroup intializations.  It seems
for xtensa the "cpN" reggroup->name is getting assigned to the same text
pointer for each iteration of XTENSA_MAX_COPROCESSOR.

Note 1, internally reggroup_new() does not do any xstrdup().

Note 2, I could not test this.

gdb/ChangeLog:

2017-06-10  Stafford Horne  <shorne@gmail.com>

	* xtensa-tdep.c (xtensa_init_reggroups): Use xstrdup for cpname.
---
 gdb/xtensa-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
index f9e8584..cfbddf2 100644
--- a/gdb/xtensa-tdep.c
+++ b/gdb/xtensa-tdep.c
@@ -746,7 +746,7 @@ xtensa_init_reggroups (void)
   for (i = 0; i < XTENSA_MAX_COPROCESSOR; i++)
     {
       cpname[2] = '0' + i;
-      xtensa_cp[i] = reggroup_new (cpname, USER_REGGROUP);
+      xtensa_cp[i] = reggroup_new (xstrdup(cpname), USER_REGGROUP);
     }
 }
 
-- 
2.9.4

Re: [PATCH] xtensa: Properly strdup string when building reggroup

[Yao Qi]

Stafford Horne <shorne@gmail.com> writes:

Patch is good to me, a nit,

> -      xtensa_cp[i] = reggroup_new (cpname, USER_REGGROUP);
> +      xtensa_cp[i] = reggroup_new (xstrdup(cpname), USER_REGGROUP);

"xstrdup (cpname)".

Before your patch is applied, GDB prints some garbage data,

(gdb) set architecture xtensa
The target architecture is assumed to be xtensa
(gdb) maintenance print reggroups 
 Group      Type      
 all        user      
 save       internal  
 restore    internal  
 system     user      
 vector     user      
 general    user      
 float      user      
 ar         user      
 user       user      
 vectra     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user      
 P�^\U     user
 P�^\U     user

with your patch applied, the output looks "right",

(gdb) maintenance print reggroups
 Group      Type      
 all        user      
 save       internal  
 restore    internal  
 system     user      
 vector     user      
 general    user      
 float      user      
 ar         user      
 user       user      
 vectra     user      
 cp0        user      
 cp1        user      
 cp2        user      
 cp3        user      
 cp4        user      
 cp5        user      
 cp6        user      
 cp7        user      
 cp8        user      
 cp9        user      
 cp:        user      
 cp;        user      
 cp<        user      
 cp=        user      
 cp>        user      
 cp?        user

This exposes another bug, IMO, here,

  for (i = 0; i < XTENSA_MAX_COPROCESSOR; i++)
    {
      cpname[2] = '0' + i;
      xtensa_cp[i] = reggroup_new (cpname, USER_REGGROUP);
    }

and XTENSA_MAX_COPROCESSOR is 0x10, so we can see "cp:", "cp;", which
looks odd.

-- 
Yao (齐尧)

Re: [PATCH] xtensa: Properly strdup string when building reggroup

[Simon Marchi]

On 2017-06-12 10:22, Yao Qi wrote:
> This exposes another bug, IMO, here,
> 
>   for (i = 0; i < XTENSA_MAX_COPROCESSOR; i++)
>     {
>       cpname[2] = '0' + i;
>       xtensa_cp[i] = reggroup_new (cpname, USER_REGGROUP);
>     }
> 
> and XTENSA_MAX_COPROCESSOR is 0x10, so we can see "cp:", "cp;", which
> looks odd.

Heh, looks like the patch here

   https://sourceware.org/ml/gdb-patches/2011-03/msg00571.html

did not take that into account :)

Re: [PATCH] xtensa: Properly strdup string when building reggroup

[Stafford Horne]

On Mon, Jun 12, 2017 at 10:38:09AM +0200, Simon Marchi wrote:
> On 2017-06-12 10:22, Yao Qi wrote:
> > This exposes another bug, IMO, here,
> > 
> >   for (i = 0; i < XTENSA_MAX_COPROCESSOR; i++)
> >     {
> >       cpname[2] = '0' + i;
> >       xtensa_cp[i] = reggroup_new (cpname, USER_REGGROUP);
> >     }
> > 
> > and XTENSA_MAX_COPROCESSOR is 0x10, so we can see "cp:", "cp;", which
> > looks odd.
> 
> Heh, looks like the patch here
> 
>   https://sourceware.org/ml/gdb-patches/2011-03/msg00571.html
> 
> did not take that into account :)

Right, I might was well resend the patch with an sprintf().

-Stafford