Re: SPARC GDB Failure

Re: SPARC GDB Failure

[Jan Kratochvil]

On Mon, 07 Sep 2009 16:18:05 +0200, Joel Sherrill wrote:
> But sparc/sis core dumps in gdb instantly.

It looks as the ia64 crash:
	http://sourceware.org/ml/gdb-patches/2009-08/msg00221.html

I grepped it before but not well enough, now used:
	$ grep -il 'malloc.*tdep' *.c|xargs grep -il '! *tdep'
	m68k-tdep.c
	sparc-tdep.c

I think the patch should go in nonetheless and I even hope it fixes it.

No testing was made.


Thanks,
Jan


gdb/
2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix start crash on unitialized memory on m68k and sparc.
	* m68k-tdep.c (m68k_gdbarch_init): Allocate TDEP as cleared.
	* sparc-tdep.c (sparc32_gdbarch_init): Allocate TDEP as cleared.
	Remove explicit clearing of TDEP fields.

--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -1160,7 +1160,7 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       break;
     }
 
-  tdep = xmalloc (sizeof (struct gdbarch_tdep));
+  tdep = xzalloc (sizeof (struct gdbarch_tdep));
   gdbarch = gdbarch_alloc (&info, tdep);
   tdep->fpregs_present = has_fp;
   tdep->flavour = flavour;
--- a/gdb/sparc-tdep.c
+++ b/gdb/sparc-tdep.c
@@ -1377,16 +1377,11 @@ sparc32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
     return arches->gdbarch;
 
   /* Allocate space for the new architecture.  */
-  tdep = XMALLOC (struct gdbarch_tdep);
+  tdep = XZALLOC (struct gdbarch_tdep);
   gdbarch = gdbarch_alloc (&info, tdep);
 
   tdep->pc_regnum = SPARC32_PC_REGNUM;
   tdep->npc_regnum = SPARC32_NPC_REGNUM;
-  tdep->gregset = NULL;
-  tdep->sizeof_gregset = 0;
-  tdep->fpregset = NULL;
-  tdep->sizeof_fpregset = 0;
-  tdep->plt_entry_size = 0;
   tdep->step_trap = sparc_step_trap;
 
   set_gdbarch_long_double_bit (gdbarch, 128);

Re: SPARC GDB Failure

[Doug Evans]

On Mon, Sep 7, 2009 at 9:45 AM, Jan Kratochvil<jan.kratochvil@redhat.com> wrote:
> On Mon, 07 Sep 2009 16:18:05 +0200, Joel Sherrill wrote:
>> But sparc/sis core dumps in gdb instantly.
>
> It looks as the ia64 crash:
>        http://sourceware.org/ml/gdb-patches/2009-08/msg00221.html
>
> I grepped it before but not well enough, now used:
>        $ grep -il 'malloc.*tdep' *.c|xargs grep -il '! *tdep'
>        m68k-tdep.c
>        sparc-tdep.c
>
> I think the patch should go in nonetheless and I even hope it fixes it.
>
> No testing was made.
>
>
> Thanks,
> Jan
>
>
> gdb/
> 2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        Fix start crash on unitialized memory on m68k and sparc.
>        * m68k-tdep.c (m68k_gdbarch_init): Allocate TDEP as cleared.
>        * sparc-tdep.c (sparc32_gdbarch_init): Allocate TDEP as cleared.
>        Remove explicit clearing of TDEP fields.
>
> --- a/gdb/m68k-tdep.c
> +++ b/gdb/m68k-tdep.c
> @@ -1160,7 +1160,7 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>       break;
>     }
>
> -  tdep = xmalloc (sizeof (struct gdbarch_tdep));
> +  tdep = xzalloc (sizeof (struct gdbarch_tdep));
>   gdbarch = gdbarch_alloc (&info, tdep);
>   tdep->fpregs_present = has_fp;
>   tdep->flavour = flavour;
> --- a/gdb/sparc-tdep.c
> +++ b/gdb/sparc-tdep.c
> @@ -1377,16 +1377,11 @@ sparc32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     return arches->gdbarch;
>
>   /* Allocate space for the new architecture.  */
> -  tdep = XMALLOC (struct gdbarch_tdep);
> +  tdep = XZALLOC (struct gdbarch_tdep);
>   gdbarch = gdbarch_alloc (&info, tdep);
>
>   tdep->pc_regnum = SPARC32_PC_REGNUM;
>   tdep->npc_regnum = SPARC32_NPC_REGNUM;
> -  tdep->gregset = NULL;
> -  tdep->sizeof_gregset = 0;
> -  tdep->fpregset = NULL;
> -  tdep->sizeof_fpregset = 0;
> -  tdep->plt_entry_size = 0;
>   tdep->step_trap = sparc_step_trap;
>
>   set_gdbarch_long_double_bit (gdbarch, 128);
>

It seems like all alloc's of gdbarch_tdep should be zalloc'd.
[But I wouldn't make that a requirement of this patch.]

The patch is fine with me.
I think, though, the changelog shouldn't claim it fixes something
unless that's been verified.

Re: SPARC GDB Failure

[Jan Kratochvil]

On Mon, 07 Sep 2009 19:44:03 +0200, Doug Evans wrote:
> The patch is fine with me.

Checked-in.


> I think, though, the changelog shouldn't claim it fixes something
> unless that's been verified.

I agree, fixed.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2009-09/msg00028.html

--- src/gdb/ChangeLog	2009/09/07 11:09:33	1.10846
+++ src/gdb/ChangeLog	2009/09/07 17:52:38	1.10847
@@ -1,3 +1,9 @@
+2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* m68k-tdep.c (m68k_gdbarch_init): Allocate TDEP as cleared.
+	* sparc-tdep.c (sparc32_gdbarch_init): Allocate TDEP as cleared.
+	Remove explicit clearing of TDEP fields.
+
 2009-09-06  Hui Zhu  <teawater@gmail.com>
 
 	* i386-tdep.c (i386_record_check_override): Deleted.
--- src/gdb/m68k-tdep.c	2009/07/02 17:25:55	1.144
+++ src/gdb/m68k-tdep.c	2009/09/07 17:52:41	1.145
@@ -1160,7 +1160,7 @@
       break;
     }
 
-  tdep = xmalloc (sizeof (struct gdbarch_tdep));
+  tdep = xzalloc (sizeof (struct gdbarch_tdep));
   gdbarch = gdbarch_alloc (&info, tdep);
   tdep->fpregs_present = has_fp;
   tdep->flavour = flavour;
--- src/gdb/sparc-tdep.c	2009/07/02 17:25:58	1.208
+++ src/gdb/sparc-tdep.c	2009/09/07 17:52:41	1.209
@@ -1377,16 +1377,11 @@
     return arches->gdbarch;
 
   /* Allocate space for the new architecture.  */
-  tdep = XMALLOC (struct gdbarch_tdep);
+  tdep = XZALLOC (struct gdbarch_tdep);
   gdbarch = gdbarch_alloc (&info, tdep);
 
   tdep->pc_regnum = SPARC32_PC_REGNUM;
   tdep->npc_regnum = SPARC32_NPC_REGNUM;
-  tdep->gregset = NULL;
-  tdep->sizeof_gregset = 0;
-  tdep->fpregset = NULL;
-  tdep->sizeof_fpregset = 0;
-  tdep->plt_entry_size = 0;
   tdep->step_trap = sparc_step_trap;
 
   set_gdbarch_long_double_bit (gdbarch, 128);

Re: SPARC GDB Failure

[Doug Evans]

On Mon, Sep 7, 2009 at 10:54 AM, Jan
Kratochvil<jan.kratochvil@redhat.com> wrote:
> On Mon, 07 Sep 2009 19:44:03 +0200, Doug Evans wrote:
>> The patch is fine with me.
>
> Checked-in.
> [...]
> http://sourceware.org/ml/gdb-cvs/2009-09/msg00028.html
>
> --- src/gdb/ChangeLog   2009/09/07 11:09:33     1.10846
> +++ src/gdb/ChangeLog   2009/09/07 17:52:38     1.10847
> @@ -1,3 +1,9 @@
> +2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
> +
> +       * m68k-tdep.c (m68k_gdbarch_init): Allocate TDEP as cleared.
> +       * sparc-tdep.c (sparc32_gdbarch_init): Allocate TDEP as cleared.
> +       Remove explicit clearing of TDEP fields.
> +
> [...]

Long-term-wise, maybe the thing to do is have all allocs of
gdbarch_tdep go through a function (gdbarch_tdep_alloc or some such).
That would make it clear how one *should* alloc them.

Re: SPARC GDB Failure

[Joel Brobecker]

> Long-term-wise, maybe the thing to do is have all allocs of
> gdbarch_tdep go through a function (gdbarch_tdep_alloc or some such).
> That would make it clear how one *should* alloc them.

Not sure if it is possible or not without having to pass the size
explicitly. This structure is opaque to most of the code in GDB,
and the size is usually only known inside the specific -tdep.c file.
We might be able to work around that by defining a macro instead of
having a function, but it's not clear whether it'd be a real
improvement or not over a call to XZALLOC...

-- 
Joel

Re: SPARC GDB Failure

[Jan Kratochvil]

On Mon, 07 Sep 2009 20:15:46 +0200, Doug Evans wrote:
> Long-term-wise, maybe the thing to do is have all allocs of
> gdbarch_tdep go through a function (gdbarch_tdep_alloc or some such).
> That would make it clear how one *should* alloc them.

I do not find any functions outside of the target file(s) to depend on its
fields (they cannot, the structure is opaque to them, as Joel has said).

I would find such change the same as changing xmalloc to always do xzalloc.
At least for mn10300 such change would be excessive.

(I would find OK to put xzalloc there but I do not find it right by design.)


Thanks,
Jan

Re: SPARC GDB Failure

[Joel Sherrill]

Doug Evans wrote:
> On Mon, Sep 7, 2009 at 10:54 AM, Jan
> Kratochvil<jan.kratochvil@redhat.com> wrote:
>   
>> On Mon, 07 Sep 2009 19:44:03 +0200, Doug Evans wrote:
>>     
>>> The patch is fine with me.
>>>       
>> Checked-in.
>> [...]
>> http://sourceware.org/ml/gdb-cvs/2009-09/msg00028.html
>>
>> --- src/gdb/ChangeLog   2009/09/07 11:09:33     1.10846
>> +++ src/gdb/ChangeLog   2009/09/07 17:52:38     1.10847
>> @@ -1,3 +1,9 @@
>> +2009-09-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
>> +
>> +       * m68k-tdep.c (m68k_gdbarch_init): Allocate TDEP as cleared.
>> +       * sparc-tdep.c (sparc32_gdbarch_init): Allocate TDEP as cleared.
>> +       Remove explicit clearing of TDEP fields.
>> +
>> [...]
>>     
>
> Long-term-wise, maybe the thing to do is have all allocs of
> gdbarch_tdep go through a function (gdbarch_tdep_alloc or some such).
> That would make it clear how one *should* alloc them.
>   
Thanks for the quick response.  I updated and the
head now works for sparc-rtems4.10-gdb.

I suspect Ralf will be cranking up a build shortly. :)

More feedback as we get it.

--joel

Re: SPARC GDB Failure

[Ralf Corsepius]

On 09/07/2009 08:39 PM, Joel Sherrill wrote:
> Thanks for the quick response.  I updated and the
> head now works for sparc-rtems4.10-gdb.
> I suspect Ralf will be cranking up a build shortly. :)
I updated our sparc and m68k-rtems4.10-gdb rpms to gdb-6.8.50.20090908 [1].

Ralf

[1] ftp://ftp.rtems.org/pub/rtems/linux/4.10