This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFA] [MIPS] Don't use $gp if abicalls are in effect
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: "Moore, Catherine" <Catherine_Moore at mentor dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Sat, 16 Jan 2016 15:41:31 +0000
- Subject: Re: [RFA] [MIPS] Don't use $gp if abicalls are in effect
- Authentication-results: sourceware.org; auth=none
- References: <FD3DCEAC5B03E9408544A1E416F112420192D05F1C at NA-MBX-04 dot mgc dot mentorg 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