Re: [RFA] xtensa-tdep.c ARI fixes

Re: [RFA] xtensa-tdep.c ARI fixes

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   The only non-trivial change is in xtensa_register_type:
Pierre> this is mainly because I do not understand the old code:
Pierre> it was using both xmalloc and strdup, which should
Pierre> create a memory leak, no?
Pierre>   But maybe I miss something?

I agree.

Pierre> -		  char *name = xmalloc (16);
Pierre> +		  char *name = xstrprintf ("int%d", size * 8);

Looks better.

Pierre> +		    = arch_integer_type (gdbarch, size * 8, 1, name);

arch_integer_type calls arch_type, which calls xstrdup(name).
So I think you need to xfree name here.

Tom

Re: [RFA] xtensa-tdep.c ARI fixes

[Maxim Grigoriev]

Thanks for noticing this. xfree () is necessary here.

-- Maxim

On 04/14/2011 01:31 PM, Tom Tromey wrote:
>>>>>> "Pierre" == Pierre Muller<pierre.muller@ics-cnrs.unistra.fr>  writes:
>>>>>>              
> Pierre>    The only non-trivial change is in xtensa_register_type:
> Pierre>  this is mainly because I do not understand the old code:
> Pierre>  it was using both xmalloc and strdup, which should
> Pierre>  create a memory leak, no?
> Pierre>    But maybe I miss something?
>
> I agree.
>
> Pierre>  -		  char *name = xmalloc (16);
> Pierre>  +		  char *name = xstrprintf ("int%d", size * 8);
>
> Looks better.
>
> Pierre>  +		    = arch_integer_type (gdbarch, size * 8, 1, name);
>
> arch_integer_type calls arch_type, which calls xstrdup(name).
> So I think you need to xfree name here.
>
> Tom
>    

[RFA-v2] xtensa-tdep.c ARI fixes

[Pierre Muller]

> Pierre>   The only non-trivial change is in xtensa_register_type:
> Pierre> this is mainly because I do not understand the old code:
> Pierre> it was using both xmalloc and strdup, which should
> Pierre> create a memory leak, no?
> Pierre>   But maybe I miss something?
> 
> I agree.
> 
> Pierre> -		  char *name = xmalloc (16);
> Pierre> +		  char *name = xstrprintf ("int%d", size * 8);
> 
> Looks better.
> 
> Pierre> +		    = arch_integer_type (gdbarch, size * 8, 1,
name);
> 
> arch_integer_type calls arch_type, which calls xstrdup(name).
> So I think you need to xfree name here.

  Here is a new version that adds this xfree call.
Could someone test this patch out?
Again, I have no way to trigger that code, so I can't really test it.

Pierre Muller
as ARI maintainer.


2011-04-15  Pierre Muller  <muller@ics.u-strasbg.fr>

	ARI cleanup.
	* xtensa-tdep.c (xtensa_register_type): Use xstrprintf instead of
	sprintf. Simplify code and avoid loosing memory.
	(xtensa_register_reggroup_p): Extract assignment out of IF clause.
	(call0_frame_cache): Remove && operator from end of line.

Index: xtensa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/xtensa-tdep.c,v
retrieving revision 1.57
diff -u -p -r1.57 xtensa-tdep.c
--- xtensa-tdep.c	18 Mar 2011 18:52:32 -0000	1.57
+++ xtensa-tdep.c	15 Apr 2011 07:05:30 -0000
@@ -318,15 +318,14 @@ xtensa_register_type (struct gdbarch *gd
 
 	      if (tp == NULL)
 		{
-		  char *name = xmalloc (16);
+		  char *name = xstrprintf ("int%d", size * 8);
 		  tp = xmalloc (sizeof (struct ctype_cache));
 		  tp->next = tdep->type_entries;
 		  tdep->type_entries = tp;
 		  tp->size = size;
-
-		  sprintf (name, "int%d", size * 8);
 		  tp->virtual_type
-		    = arch_integer_type (gdbarch, size * 8, 1, xstrdup
(name));
+		    = arch_integer_type (gdbarch, size * 8, 1, name);
+		  xfree (name);
 		}
 
 	      reg->ctype = tp->virtual_type;
@@ -843,7 +842,8 @@ xtensa_register_reggroup_p (struct gdbar
   if (group == restore_reggroup)
     return (regnum < gdbarch_num_regs (gdbarch)
 	    && (reg->flags & SAVE_REST_FLAGS) == SAVE_REST_VALID);
-  if ((cp_number = xtensa_coprocessor_register_group (group)) >= 0)
+  cp_number = xtensa_coprocessor_register_group (group);
+  if (cp_number >= 0)
     return rg & (xtRegisterGroupCP0 << cp_number);
   else
     return 1;
@@ -2715,9 +2715,9 @@ call0_frame_cache (struct frame_info *th
 	 too bad.  */
 
       int i;
-      for (i = 0; 
-	   (i < C0_NREGS) &&
-	     (i == C0_RA || cache->c0.c0_rt[i].fr_reg != C0_RA);
+      for (i = 0;
+	   (i < C0_NREGS)
+	   &&  (i == C0_RA || cache->c0.c0_rt[i].fr_reg != C0_RA);
 	   ++i);
       if (i >= C0_NREGS && cache->c0.c0_rt[C0_RA].fr_reg == C0_RA)
 	i = C0_RA;

Re: [RFA-v2] xtensa-tdep.c ARI fixes

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   Here is a new version that adds this xfree call.
Pierre> Could someone test this patch out?
Pierre> Again, I have no way to trigger that code, so I can't really test it.

The patch looks good to me.
But I agree, testing would be nice.

Pierre> +	   &&  (i == C0_RA || cache->c0.c0_rt[i].fr_reg != C0_RA);

Extra space after the "&&" :-)

Tom

Re: [RFA-v2] xtensa-tdep.c ARI fixes

[Maxim Grigoriev]

I tested the patch. No problems have been detected.
Thanks much for improving Xtensa code.

-- Maxim

On 04/15/2011 11:21 AM, Tom Tromey wrote:
>>>>>> "Pierre" == Pierre Muller<pierre.muller@ics-cnrs.unistra.fr>  writes:
>>>>>>              
> Pierre>    Here is a new version that adds this xfree call.
> Pierre>  Could someone test this patch out?
> Pierre>  Again, I have no way to trigger that code, so I can't really test it.
>
> The patch looks good to me.
> But I agree, testing would be nice.
>
> Pierre>  +	&&   (i == C0_RA || cache->c0.c0_rt[i].fr_reg != C0_RA);
>
> Extra space after the "&&" :-)
>
> Tom
>    

RE: [RFA-v2] xtensa-tdep.c ARI fixes

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Maxim Grigoriev
> Envoyé : vendredi 15 avril 2011 20:37
> À : Tom Tromey
> Cc : Pierre Muller; gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] xtensa-tdep.c ARI fixes
> 
> I tested the patch. No problems have been detected.
> Thanks much for improving Xtensa code.

  May I take this as an approval?


> On 04/15/2011 11:21 AM, Tom Tromey wrote:
> >>>>>> "Pierre" == Pierre Muller<pierre.muller@ics-cnrs.unistra.fr>
writes:
> >>>>>>
> > Pierre>    Here is a new version that adds this xfree call.
> > Pierre>  Could someone test this patch out?
> > Pierre>  Again, I have no way to trigger that code, so I can't really
test
> it.
> >
> > The patch looks good to me.
> > But I agree, testing would be nice.
> >
> > Pierre>  +	&&   (i == C0_RA || cache->c0.c0_rt[i].fr_reg != C0_RA);
> >
> > Extra space after the "&&" :-)

  I am lucky this is not yet tested inside ARI web page...
Otherwise my attempt of getting rid of xtensa-tdep.c source
in the ARI listing would have failed.

  Thanks for noticing,

Pierre

Re: [RFA-v2] xtensa-tdep.c ARI fixes

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

>> I tested the patch. No problems have been detected.
>> Thanks much for improving Xtensa code.

Pierre>   May I take this as an approval?

Yes, please check this in.
Thanks.

Tom

RE: [RFA-v2] xtensa-tdep.c ARI fixes

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : lundi 18 avril 2011 19:07
> À : Pierre Muller
> Cc : 'Maxim Grigoriev'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] xtensa-tdep.c ARI fixes
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> >> I tested the patch. No problems have been detected.
> >> Thanks much for improving Xtensa code.
> 
> Pierre>   May I take this as an approval?
> 
> Yes, please check this in.
> Thanks.

  Patch applied,
thanks to both of you.

Pierre