Re: [PATCH/RFC] coffread.c: delete param

Re: [PATCH/RFC] coffread.c: delete param

[Michael Elizabeth Chastain]

Nickc writes:
> Given Andrew's comment in the code, I would be rather wary of this
> patch.  Presumably there is some good reason for passing the
> cs->c_sclass field in the (void *) pointer argument slot, or otherwise
> Andrew would not have gone to all that trouble of casting it.

I figured out some of the history:

Back in gdb 4.17, arm_pc_is_thumb in arm-tdep.c used this c_sclass
information to determine whether a symbol is a Thumb or Arm function.
gdb 4.18 rewrote arm_pc_is_thumb with a new mechanism based on
COFF_MAKE_MSYMBOL_SPECIAL, which sets a flag bit instead of storing the
sclass and reading it later.  That enables other readers besides the
coff reader to mark functions as special (ELF_MAKE_MSYMBOL_SPECIAL
in particular).

The change from 4.17 to 4.18 added the new COFF_MAKE_MSYMBOL_SPECIAL,
and changed arm-tdep.c to use the new COFF_MAKE_MSYMBOL_SPECIAL,
but did not delete the old dangling c_sclass code.

Someone with access to the old Cygnus CVS repository might be able
to say more about the exact change some time between 4.17 and 4.18.

Andrew C came along a few years later and added a cast just to make
the compiler happy -- just an innocent janitor.

It's very unlikely that Elena's change will change the behavior of gdb.
This c_sclass info has been dead since 4.18, replaced by
COFF_MAKE_MSYMBOL_SPECIAL.

It would be nice if someone could build and test arm-coff to verify
this line of reasoning.  But, it's also nice to know the history,
which explains why c_sclass is stored, but never read.

> Does it improve things ?  :-)  If so, what ?

It's part of a cleanup for the msymbol.info field.

There are three different places in gdb which use msymbol.info.
This is one of them.  The improvement is that getting rid
of dead code makes it simpler to talk about the other two places.

Michael C

Re: [PATCH/RFC] coffread.c: delete param

[Elena Zannoni]

Michael Elizabeth Chastain writes:
 > Nickc writes:
 > > Given Andrew's comment in the code, I would be rather wary of this
 > > patch.  Presumably there is some good reason for passing the
 > > cs->c_sclass field in the (void *) pointer argument slot, or otherwise
 > > Andrew would not have gone to all that trouble of casting it.
 > 
 > I figured out some of the history:
 > 
 > Back in gdb 4.17, arm_pc_is_thumb in arm-tdep.c used this c_sclass
 > information to determine whether a symbol is a Thumb or Arm function.
 > gdb 4.18 rewrote arm_pc_is_thumb with a new mechanism based on
 > COFF_MAKE_MSYMBOL_SPECIAL, which sets a flag bit instead of storing the
 > sclass and reading it later.  That enables other readers besides the
 > coff reader to mark functions as special (ELF_MAKE_MSYMBOL_SPECIAL
 > in particular).
 > 
 > The change from 4.17 to 4.18 added the new COFF_MAKE_MSYMBOL_SPECIAL,
 > and changed arm-tdep.c to use the new COFF_MAKE_MSYMBOL_SPECIAL,
 > but did not delete the old dangling c_sclass code.
 > 
 > Someone with access to the old Cygnus CVS repository might be able
 > to say more about the exact change some time between 4.17 and 4.18.

Did you see my posting with the exact diffs (from 1998) I posted
yesterday?
http://sources.redhat.com/ml/gdb-patches/2003-10/msg00470.html

That explains in detail what happened, which is pretty much how you
describe it here.

elena


 > 
 > Andrew C came along a few years later and added a cast just to make
 > the compiler happy -- just an innocent janitor.
 > 
 > It's very unlikely that Elena's change will change the behavior of gdb.
 > This c_sclass info has been dead since 4.18, replaced by
 > COFF_MAKE_MSYMBOL_SPECIAL.
 > 
 > It would be nice if someone could build and test arm-coff to verify
 > this line of reasoning.  But, it's also nice to know the history,
 > which explains why c_sclass is stored, but never read.
 > 
 > > Does it improve things ?  :-)  If so, what ?
 > 
 > It's part of a cleanup for the msymbol.info field.
 > 
 > There are three different places in gdb which use msymbol.info.
 > This is one of them.  The improvement is that getting rid
 > of dead code makes it simpler to talk about the other two places.
 > 
 > Michael C

Re: [PATCH/RFC] coffread.c: delete param

[Andrew Cagney]

>  > The change from 4.17 to 4.18 added the new COFF_MAKE_MSYMBOL_SPECIAL,
>  > and changed arm-tdep.c to use the new COFF_MAKE_MSYMBOL_SPECIAL,
>  > but did not delete the old dangling c_sclass code.
>  > 
>  > Someone with access to the old Cygnus CVS repository might be able
>  > to say more about the exact change some time between 4.17 and 4.18.
> 
> Did you see my posting with the exact diffs (from 1998) I posted
> yesterday?
> http://sources.redhat.com/ml/gdb-patches/2003-10/msg00470.html
> 
> That explains in detail what happened, which is pretty much how you
> describe it here.

BTW, if COFF's minimal symbol pointed at the bfd symbol, GDB could share 
the logic:

>   if (!is_thumb && info->symbols != NULL)
>     {
>       if (bfd_asymbol_flavour (*info->symbols) == bfd_target_coff_flavour)
>         {
>           coff_symbol_type * cs;
> 
>           cs = coffsymbol (*info->symbols);
>           is_thumb = (   cs->native->u.syment.n_sclass == C_THUMBEXT
>                       || cs->native->u.syment.n_sclass == C_THUMBSTAT
>                       || cs->native->u.syment.n_sclass == C_THUMBLABEL
>                       || cs->native->u.syment.n_sclass == C_THUMBEXTFUNC
>                       || cs->native->u.syment.n_sclass == C_THUMBSTATFUNC);
>         }
>       else if (bfd_asymbol_flavour (*info->symbols) == bfd_target_elf_flavour)
>         {
>           elf_symbol_type *  es;
>           unsigned int       type;
> 
>           es = *(elf_symbol_type **)(info->symbols);
>           type = ELF_ST_TYPE (es->internal_elf_sym.st_info);
> 
>           is_thumb = (type == STT_ARM_TFUNC) || (type == STT_ARM_16BIT);
>         }
>     }
> 

found in opcodes/arm-dis.c with bfd/opcodes.  Similar for MIPS, SH-5, 
M68HC11, ...

Andrew

[PATCH/RFC] coffread.c: delete param

[Elena Zannoni]

Based on the discussion in this thread:
http://sources.redhat.com/ml/gdb-patches/2003-10/msg00405.html

I don't have a set up to test this, though. It does build, that's all
I can say.

[Richard, Nick, this does affect arm-coff]

elena

2003-10-14  Elena Zannoni  <ezannoni@redhat.com>

	* coffread.c (coff_symtab_read): Remove passing of info parameter
	to prim_record_minimal_symbol_and_info.

Index: coffread.c
===================================================================
RCS file: /cvs/uberbaum/gdb/coffread.c,v
retrieving revision 1.45
diff -u -p -r1.45 coffread.c
--- coffread.c	21 Sep 2003 01:26:44 -0000	1.45
+++ coffread.c	14 Oct 2003 21:37:31 -0000
@@ -926,15 +926,8 @@ coff_symtab_read (long symtab_offset, un
 	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
 	      {
 		struct minimal_symbol *msym;
-
- 		/* FIXME: cagney/2001-02-01: The nasty (int) -> (long)
-                   -> (void*) cast is to ensure that that the value of
-                   cs->c_sclass can be correctly stored in a void
-                   pointer in MSYMBOL_INFO.  Better solutions
-                   welcome. */
-		gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
 		msym = prim_record_minimal_symbol_and_info
-		  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
+		  (cs->c_name, tmpaddr, ms_type, NULL,
 		   sec, NULL, objfile);
 		if (msym)
 		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);

Re: [PATCH/RFC] coffread.c: delete param

[Richard Earnshaw]

> 
> Based on the discussion in this thread:
> http://sources.redhat.com/ml/gdb-patches/2003-10/msg00405.html
> 
> I don't have a set up to test this, though. It does build, that's all
> I can say.
> 
> [Richard, Nick, this does affect arm-coff]
> 

I really couldn't say.  I don't think I've ever built an arm-coff target.  
It was something added for one of Red Hat's customers.

R.

Re: [PATCH/RFC] coffread.c: delete param

[Andrew Cagney]

>> 
>> Based on the discussion in this thread:
>> http://sources.redhat.com/ml/gdb-patches/2003-10/msg00405.html
>> 
>> I don't have a set up to test this, though. It does build, that's all
>> I can say.
>> 
>> [Richard, Nick, this does affect arm-coff]
>> 
> 
> 
> I really couldn't say.  I don't think I've ever built an arm-coff target.  
> It was something added for one of Red Hat's customers.

As an aside, arm-pe is alive and kicking (in a moblile phone near you 
...).  However, I don't think that should be used as an excuse to not 
clean this up.

Andrew

Re: [PATCH/RFC] coffread.c: delete param

[Nick Clifton]

Hi Elena,

> Based on the discussion in this thread:
> http://sources.redhat.com/ml/gdb-patches/2003-10/msg00405.html
>
> I don't have a set up to test this, though. It does build, that's
> all I can say.

Given Andrew's comment in the code, I would be rather wary of this
patch.  Presumably there is some good reason for passing the
cs->c_sclass field in the (void *) pointer argument slot, or otherwise
Andrew would not have gone to all that trouble of casting it.

> -
> - 		/* FIXME: cagney/2001-02-01: The nasty (int) -> (long)
> -                   -> (void*) cast is to ensure that that the value of
> -                   cs->c_sclass can be correctly stored in a void
> -                   pointer in MSYMBOL_INFO.  Better solutions
> -                   welcome. */
> -		gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
>  		msym = prim_record_minimal_symbol_and_info
> -		  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
> +		  (cs->c_name, tmpaddr, ms_type, NULL,
>  		   sec, NULL, objfile);


> [Richard, Nick, this does affect arm-coff]

Does it improve things ?  :-)  If so, what ?

Cheers
        Nick
        

Re: [PATCH/RFC] coffread.c: delete param

[Andrew Cagney]

> Given Andrew's comment in the code, I would be rather wary of this
> patch.  Presumably there is some good reason for passing the
> cs->c_sclass field in the (void *) pointer argument slot, or otherwise
> Andrew would not have gone to all that trouble of casting it.

No.  I went to the trouble of casing it so that it got past GCC -Werror.

Andrew


>> -
>> - 		/* FIXME: cagney/2001-02-01: The nasty (int) -> (long)
>> -                   -> (void*) cast is to ensure that that the value of
>> -                   cs->c_sclass can be correctly stored in a void
>> -                   pointer in MSYMBOL_INFO.  Better solutions
>> -                   welcome. */
>> -		gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
>>  		msym = prim_record_minimal_symbol_and_info
>> -		  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
>> +		  (cs->c_name, tmpaddr, ms_type, NULL,
>>  		   sec, NULL, objfile);
> 
> 
> 
>> [Richard, Nick, this does affect arm-coff]
> 
> 
> Does it improve things ?  :-)  If so, what ?
> 
> Cheers
>         Nick
>         
> 
> 

Re: [PATCH/RFC] coffread.c: delete param

[Elena Zannoni]

Nick Clifton writes:
 > Hi Elena,
 > 
 > > Based on the discussion in this thread:
 > > http://sources.redhat.com/ml/gdb-patches/2003-10/msg00405.html
 > >
 > > I don't have a set up to test this, though. It does build, that's
 > > all I can say.
 > 
 > Given Andrew's comment in the code, I would be rather wary of this
 > patch.  Presumably there is some good reason for passing the
 > cs->c_sclass field in the (void *) pointer argument slot, or otherwise
 > Andrew would not have gone to all that trouble of casting it.

Ah, not really. I think he was just compiling with -Werror. It's that
you added that parameter there, several years ago. So I was wondering
if you still had any interest in this code, and had a way of testing it.

 > 
 > > -
 > > - 		/* FIXME: cagney/2001-02-01: The nasty (int) -> (long)
 > > -                   -> (void*) cast is to ensure that that the value of
 > > -                   cs->c_sclass can be correctly stored in a void
 > > -                   pointer in MSYMBOL_INFO.  Better solutions
 > > -                   welcome. */
 > > -		gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
 > >  		msym = prim_record_minimal_symbol_and_info
 > > -		  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
 > > +		  (cs->c_name, tmpaddr, ms_type, NULL,
 > >  		   sec, NULL, objfile);
 > 
 > 
 > > [Richard, Nick, this does affect arm-coff]
 > 
 > Does it improve things ?  :-)  If so, what ?
 > 

It certainly opens the way for more cleanups.
Is arm-coff still alive and of interest?

elena

 > Cheers
 >         Nick
 >         

Re: [PATCH/RFC] coffread.c: delete param

[Elena Zannoni]

Elena Zannoni writes:
 > 
 > Based on the discussion in this thread:
 > http://sources.redhat.com/ml/gdb-patches/2003-10/msg00405.html
 > 
 > I don't have a set up to test this, though. It does build, that's all
 > I can say.
 > 
 > [Richard, Nick, this does affect arm-coff]
 > 
 > elena
 > 

committed.

elena

 > 2003-10-14  Elena Zannoni  <ezannoni@redhat.com>
 > 
 > 	* coffread.c (coff_symtab_read): Remove passing of info parameter
 > 	to prim_record_minimal_symbol_and_info.
 > 
 > Index: coffread.c
 > ===================================================================
 > RCS file: /cvs/uberbaum/gdb/coffread.c,v
 > retrieving revision 1.45
 > diff -u -p -r1.45 coffread.c
 > --- coffread.c	21 Sep 2003 01:26:44 -0000	1.45
 > +++ coffread.c	14 Oct 2003 21:37:31 -0000
 > @@ -926,15 +926,8 @@ coff_symtab_read (long symtab_offset, un
 >  	    if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
 >  	      {
 >  		struct minimal_symbol *msym;
 > -
 > - 		/* FIXME: cagney/2001-02-01: The nasty (int) -> (long)
 > -                   -> (void*) cast is to ensure that that the value of
 > -                   cs->c_sclass can be correctly stored in a void
 > -                   pointer in MSYMBOL_INFO.  Better solutions
 > -                   welcome. */
 > -		gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
 >  		msym = prim_record_minimal_symbol_and_info
 > -		  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
 > +		  (cs->c_name, tmpaddr, ms_type, NULL,
 >  		   sec, NULL, objfile);
 >  		if (msym)
 >  		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);