Re: ARM mapping symbols

[Richard Earnshaw <rearnsha at arm dot com>]

> On Mon, 8 Mar 2004, Richard Earnshaw wrote:
> 
> > > Hi Bruno,
> > > 
> > > >> > The last assembled section is often a data section, so the mapping
> > > >> > symbol is $d. Then arm_cleanup is called and the literal pool(s)
> > > >> > is(/are) written. As the last symbol already was $d, no new symbol
> > > >> > is added... 
> > > >> 
> > > >> Ok - but this is easily fixed by calling arm_elf_change_section()
> > > >> after calling subseg_set() in arm_cleanup().  Then the correct-for-
> > > >> that-section mapping symbol will be emitted, which can then be changed
> > > >> by your patch to s_ltorg().
> > > >
> > > > I might be wrong again, but I believe this will add both a $d and a 
> > > > $a symbol? 
> > > 
> > > Yes - is this a problem ?  I know that it is not optimal, but I
> > > thought that it might be a simpler "quick hack" that adding a 'forced'
> > > argument to mapping_state().
> > 
> > Please lets not talk about "quick hacks".  Quick hacks have a tendency to 
> > breed, and before you know what's happening you have serious spaghetti to 
> > untangle.
> > 
> > Instead, lets work out what needs doing to fix this properly.
> 
> Agreed, 
> 
> the attached patch solves the problem in a more clean fashion:
> 
> * I kept the last mapping symbol for a segment in TC_SEGMENT_INFO_TYPE as 
> suggested by Nick
> * Added a call to arm_elf_change_section in arm_cleanup as suggested by 
> Nick
> * I added a mapping state MAP_UNDEFINED as suggested by Richard (this 
> avoids duplicate mapping symbols at the start of a section)
> * moved the enum to tc-arm.h
> 
> * changed
> #define md_elf_section_change_hook() arm_elf_change_section
> to 
> #define md_elf_section_change_hook() arm_elf_change_section()
> in tc-arm.h ???????????????? Weird.....
> 
> * moved mapstate out of the function so we can change it when changing the 
> section
> 
> It tested the example given by Richard and literal pools (both explicit 
> and implicit) and they appear to work correct.... 
> 
> 
> Hope this is acceptable....
> 

This is a *vast* improvement.  There's a couple of minor technical 
niggles, where you haven't conformed to the GNU coding standards, and 
there's no ChangeLog entry.  But other than that, I think this is OK now.

It would also be good if there were test cases added to the regression 
suite to check ensure that we don't break things again in future.

> Bruno
> 
> PS:
> 
> I applied for a copyright assignment (but the patch is really small, even 
> smaller than the first one)

This is Nick's call.  I'd err on the side of caution and ask for one, but 
as lead maintainer, Nick might be prepared to let this one through.

Please can you fix up the niggles, add a ChangeLog entry and re-send the 
diff using "diff -p" format.


General: in all cases there should be a space between the function name 
and the opening parenthesis for the arguments/parameters.

	foo ()
never
	foo()


>  
> -static void
> -mapping_state (enum mstate state)
> +static enum mstate mapstate = MAP_UNDEFINED;
> +
> +static void mapping_state (enum mstate state)

Function name on a new line.

>  {
> -  static enum mstate mapstate = MAP_DATA;
>    symbolS * symbolP;
>    const char * symname;
>    int type;
> @@ -2933,10 +2926,15 @@
>        symname = "$t";
>        type = BSF_FUNCTION;
>        break;
> +    case MAP_UNDEFINED:
> +      return;
> +      

blank line before new case statement.

> @@ -3111,6 +3100,9 @@
>  
>    /* Align pool as you have word accesses.
>       Only make a frag if we have to.  */
> +
> +
> +  mapping_state(MAP_DATA);

excess blank lines between comment and code.  In fact, this comment is now 
divorced from the code it refers to.  Please rearrange that.

>    if (!need_pass_2)
>      frag_align (2, 0, 0);
>  
> +enum mstate
> +{
> +  MAP_UNDEFINED=0, /* Must be zero, for seginfo in new sections */

White space around '='

R.