[PATCH, MIPS] Ensure default ASEs are applied for an architecture

Maciej W. Rozycki macro@imgtec.com
Wed Mar 22 11:15:00 GMT 2017


Hi Matthew,

 Thank you for your submission.

 Your proposed change looks mostly good to me, except I think we don't 
actually need a separate `init_ase' member; see below.  See a couple of 
minor nits too.

> gas/
> 

 Please remember to quote "PR gas/21219" along with the ChangeLog entry, 
both in the commit description and the actual file, and preferably also in 
the commit heading (e-mail subject).

> 	* config/tc-mips.c (struct mips_set_options): Add init_ase
> 	field.
> 	(file_mips_opts): Zero init_ase.
> 	(mips_opts): Likewise.
> 	(file_mips_check_options): Use file_mips_opts.init_ase to
> 	set enabled ases.

 Please capitalise ASEs correctly.

> @@ -3918,8 +3926,6 @@ mips_check_options (struct mips_set_options *opts, bfd_boolean abi_checks)
>  static void
>  file_mips_check_options (void)
>  {
> -  const struct mips_cpu_info *arch_info = 0;
> -
>    if (file_mips_opts_checked)
>      return;
>  
> @@ -3962,8 +3968,6 @@ file_mips_check_options (void)
>  	file_mips_opts.fp = 32;
>      }
>  
> -  arch_info = mips_cpu_info_from_arch (file_mips_opts.arch);
> -
>    /* Disable operations on odd-numbered floating-point registers by default
>       when using the FPXX ABI.  */
>    if (file_mips_opts.oddspreg < 0)
> @@ -4007,7 +4011,7 @@ file_mips_check_options (void)
>  
>    /* If the user didn't explicitly select or deselect a particular ASE,
>       use the default setting for the CPU.  */
> -  file_mips_opts.ase |= (arch_info->ase & ~file_ase_explicit);
> +  file_mips_opts.ase |= (file_mips_opts.init_ase & ~file_ase_explicit);

 This is the first place where `file_mips_opts.ase' is ever set AFAICT, so 
I think all that you need here is:

  file_mips_opts.ase &= ~file_ase_explicit;

(and `arch_info' can of course go, as you did), with the hunk below 
changed accordingly:

> @@ -14769,6 +14773,7 @@ mips_after_parse_args (void)
>  
>    file_mips_opts.arch = arch_info->cpu;
>    file_mips_opts.isa = arch_info->isa;
> +  file_mips_opts.init_ase = arch_info->ase;

  file_mips_opts.ase = arch_info->ase;

The rest of the change can then go.  Please verify that this does what's 
intended.

 As a follow-up update I think `.set arch=' should set `mips_opts.ase' 
accordingly too (possibly with `file_ase_explicit' applied), which would 
be consistent and should resolve some issues we have seen with the Linux 
kernel.

 Also as another followup I think we should record `arch_info->name' too 
for reporting, so that rather than say:

Error: opcode not supported on this processor: mips32r2 (mips32r2)

a more meaningful:

Error: opcode not supported on this processor: 34kc (mips32r2)

message is produced, reporting the actual architecture chosen even for 
those that do not have a dedicated BFD architecture.  Then 
`mips_cpu_info_from_arch' can go altogether.

 I can handle the two followups myself unless you're keen to.

 For further nits see below.

> diff --git a/gas/testsuite/gas/mips/34kc-mt.d b/gas/testsuite/gas/mips/34kc-mt.d
> new file mode 100644
> index 0000000..f5ac635
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/34kc-mt.d
> @@ -0,0 +1,12 @@
> +#objdump: -dr --prefix-addresses -M reg-names=numeric
> +#name: MIPS 34kc MT ASE default

 Please capitalise 34Kc correctly.

> +#as: -32 -march=34kc
> +
> +# Test the abs macro.

 Pasto here?

> +
> +.*: +file format .*mips.*
> +
> +Disassembly of section .text:
> +0+0000 <[^>]*> mttc0	\$4,\$25,1
> +0+0004 <[^>]*> mftc0	\$25,\$4
> +	...

 Please escape `.' characters to be matched literally here and with 
`.text'.  We do have special handling in the test framework for implicit 
escaping `.' in `.text', `.data' and `.bss' in dump patterns, but not for 
other section names and certainly not for other cases, so please keep our 
test cases consistent.

> diff --git a/gas/testsuite/gas/mips/34kc-mt.s b/gas/testsuite/gas/mips/34kc-mt.s
> new file mode 100644
> index 0000000..ab68fdb
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/34kc-mt.s
> @@ -0,0 +1,5 @@
> +# Source file used to test that the MT ase is enabled by default for 34kc.

 34Kc again, and also ASE.

 Please rework and resubmit.

  Maciej



More information about the Binutils mailing list