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

["Maciej W. Rozycki" <macro at imgtec dot com>]

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?

 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.

> @@ -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.

> 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?

> 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.

  Maciej