RE: [RFA] [MIPS] Don't use $gp if abicalls are in effect

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

> -----Original Message-----
> From: Maciej W. Rozycki [mailto:macro@imgtec.com]
> Sent: Saturday, January 16, 2016 10:42 AM
> To: Moore, Catherine
> Cc: binutils@sourceware.org
> Subject: Re: [RFA] [MIPS] Don't use $gp if abicalls are in effect
> 
> Hi Catherine,
> 
> > This patch prevents the assembler from using $gp if abicalls are in
> > effect.  OK to install?
> 
>  Please use a more accurate patch headline for the GIT commit, e.g.
> "Disallow gp-relative addressing if abicalls are in effect" or "Disallow the use
> of -G if...", etc. -- at first I thought: why would you want to stop GAS using
> that register?
> 

Okay, sure.

>  The change conceptually looks good to me, however I see a few small issues
> with the patch itself:
> 
> > diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c index
> > 859ddc6..eec725b 100644
> > --- a/gas/config/tc-mips.c
> > +++ b/gas/config/tc-mips.c
> > @@ -3466,7 +3466,13 @@ md_begin (void)
> >  	as_bad (_("-G may not be used in position-independent code"));
> >        g_switch_value = 0;
> >      }
> > -
> > +  else if (mips_abicalls)
> > +    {
> > +      if (g_switch_seen && g_switch_value != 0)
> > +	as_bad (_("-G may not be used with abicalls"));
> > +      g_switch_value = 0;
> > +    }
> > +
> 
>  Trailing space here, please remove.

Done.
> 
> > @@ -14318,6 +14324,8 @@ md_parse_option (int c, char *arg)
> >      case OPTION_CALL_SHARED:
> >        mips_pic = SVR4_PIC;
> >        mips_abicalls = TRUE;
> > +      if (!g_switch_seen)
> > +	g_switch_value = 0;
> >        break;
> 
>  This change makes code depend on the ordering between -G and -
> call_shared on the command line, and also affects the situation where GAS is
> called like this:
> 
> $ as -call_shared -non_shared ...
> 
> i.e. where a later option cancels an earlier one -- one would expect the
> setting of -G to remain at its default then.
> 
>  However this change is also not needed AFAICT, because with your patch in
> place `g_switch_value' will be reset as required in `md_begin'.  So please
> either just drop this part or otherwise tell me why it is needed.

I've dropped this part of the patch.  As you pointed out, it's not necessary.

> 
> > diff --git a/gas/testsuite/gas/mips/mips.exp
> > b/gas/testsuite/gas/mips/mips.exp index 6645e83..c0a65f1 100644
> > --- a/gas/testsuite/gas/mips/mips.exp
> > +++ b/gas/testsuite/gas/mips/mips.exp
> > @@ -741,6 +741,7 @@ if { [istarget mips*-*-vxworks*] } {
> >      run_dump_test_arches "rol64-hw"	[mips_arch_list_matching
> gpr64 ror]
> >
> >      run_dump_test "sb"
> > +    run_dump_test_arches "sdata-gp"	[mips_arch_list_matching
> mips3]
> 
>  Why is running this test limited to MIPS III+ ISAs only?

I was avoiding errors from the mips64-linux configuration:

Error: -march=mips1 is not compatible with the selected ABI
/scratch/cmoore/9101/patched2/binutils-gdb/gas/testsuite/gas/mips/sdata-gp.s:7: Error: `gp=32' used with a 64-bit ABI
/scratch/cmoore/9101/patched2/binutils-gdb/gas/testsuite/gas/mips/sdata-gp.s:7: Warning: `fp=32' used with a 64-bit ABI

This version of the patch adds -32 to the assembler command and runs the test for MIPS1 +
I take it you prefer this?

> 
> > diff --git a/gas/testsuite/gas/mips/sdata-gp.s
> > b/gas/testsuite/gas/mips/sdata-gp.s
> > new file mode 100755
> > index 0000000..fa6fc08
> > --- /dev/null
> > +++ b/gas/testsuite/gas/mips/sdata-gp.s
> > @@ -0,0 +1,7 @@
> > +	.sdata
> > +c0101:	.word	0xabcd
> > +
> > +	.text
> > +	.align	4
> > +test:
> > +        lw      $2, c0101
> 
>  Wrong indentation in the last line, please replace spaces with tabs.
> 
Done.

Okay, now?
Thanks,
Catherine

Attachment: gprel.cl
Description: gprel.cl

Attachment: gprel.patch
Description: gprel.patch