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

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

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