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

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

[Matthew Fortune]

Hi,

MIPS GAS allows an architecture have a set of ASEs implicitly enabled
if they are guaranteed to be available.  Some architectures are
represented as unique CPUs in the GAS internals and some are just
baseline architecture with some ASEs on top.  In the latter case the
default ASEs fail to be applied leading to issues like PR/21219.

This was caused some time ago by:

commit 919731affbef19fcad8dddb0a595bb05755cb345
Author: mfortune <matthew.fortune@imgtec.com>
Date:   Tue May 20 13:28:20 2014 +0100

    Add MIPS .module directive

Proposed fix is to store the ASEs explicitly along with the
architecture in file_mips_opts so that the second case identified
above works. 

Regression tested on mips-mti-elf. No regressions.

Thanks,
Matthew

gas/

	* 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.
	(mips_after_parse_args): Set init_ases.
	(parse_code_option): Likewise.
	(s_mipsset): Likewise.
	* testsuite/gas/mips/34kc-mt.d: New file.
	* testsuite/gas/mips/34kc-mt.d: Likewise.
	* testsuite/gas/mips/mips.exp: Run 34kc-mt test.
---
 gas/config/tc-mips.c             | 22 +++++++++++++++-------
 gas/testsuite/gas/mips/34kc-mt.d | 12 ++++++++++++
 gas/testsuite/gas/mips/34kc-mt.s |  5 +++++
 gas/testsuite/gas/mips/mips.exp  |  1 +
 4 files changed, 33 insertions(+), 7 deletions(-)
 create mode 100644 gas/testsuite/gas/mips/34kc-mt.d
 create mode 100644 gas/testsuite/gas/mips/34kc-mt.s

diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
index 54b94be..f0689c1 100644
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -262,6 +262,12 @@ struct mips_set_options
   /* 1 if single-precision operations on odd-numbered registers are
      allowed.  */
   int oddspreg;
+
+  /* The set of ASEs that should be enabled for the user specified
+     architecture.  This cannot be inferred from 'arch' for all cores
+     as processors only have a unique 'arch' if they add architecture
+     specific instructions (UDI).  */
+  int init_ase;
 };
 
 /* Specifies whether module level options have been checked yet.  */
@@ -283,7 +289,8 @@ static struct mips_set_options file_mips_opts =
   /* noreorder */ 0,  /* at */ ATREG, /* warn_about_macros */ 0,
   /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
   /* gp */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
-  /* soft_float */ FALSE, /* single_float */ FALSE, /* oddspreg */ -1
+  /* soft_float */ FALSE, /* single_float */ FALSE, /* oddspreg */ -1,
+  /* init_ase */ 0
 };
 
 /* This is similar to file_mips_opts, but for the current set of options.  */
@@ -294,7 +301,8 @@ static struct mips_set_options mips_opts =
   /* noreorder */ 0,  /* at */ ATREG, /* warn_about_macros */ 0,
   /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
   /* gp */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
-  /* soft_float */ FALSE, /* single_float */ FALSE, /* oddspreg */ -1
+  /* soft_float */ FALSE, /* single_float */ FALSE, /* oddspreg */ -1,
+  /* init_ase */ 0
 };
 
 /* Which bits of file_ase were explicitly set or cleared by ASE options.  */
@@ -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);
 
   /* Set up the current options.  These may change throughout assembly.  */
   mips_opts = file_mips_opts;
@@ -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;
 
   /* Set up initial mips_opts state.  */
   mips_opts = file_mips_opts;
@@ -16133,6 +16138,7 @@ parse_code_option (char * name)
 	    {
 	      mips_opts.arch = p->cpu;
 	      mips_opts.isa = p->isa;
+	      mips_opts.init_ase = p->ase;
 	      isa_set = TRUE;
 	    }
 	}
@@ -16147,6 +16153,7 @@ parse_code_option (char * name)
 	    {
 	      mips_opts.arch = p->cpu;
 	      mips_opts.isa = p->isa;
+	      mips_opts.init_ase = p->ase;
 	      isa_set = TRUE;
 	    }
 	}
@@ -16221,6 +16228,7 @@ s_mipsset (int x ATTRIBUTE_UNUSED)
     {
       mips_opts.isa = file_mips_opts.isa;
       mips_opts.arch = file_mips_opts.arch;
+      mips_opts.init_ase = file_mips_opts.init_ase;
       mips_opts.gp = file_mips_opts.gp;
       mips_opts.fp = file_mips_opts.fp;
     }
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
+#as: -32 -march=34kc
+
+# Test the abs macro.
+
+.*: +file format .*mips.*
+
+Disassembly of section .text:
+0+0000 <[^>]*> mttc0	\$4,\$25,1
+0+0004 <[^>]*> mftc0	\$25,\$4
+	...
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.
+foo:
+	mttc0	$4,$25,1
+	mftc0	$25,$4,0
+	.space	8
diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
index f0c6c34..086ba2a 100644
--- a/gas/testsuite/gas/mips/mips.exp
+++ b/gas/testsuite/gas/mips/mips.exp
@@ -1347,6 +1347,7 @@ if { [istarget mips*-*-vxworks*] } {
     run_dump_test_arches "mips32-dspr3"	[mips_arch_list_matching mips32r6]
     run_dump_test "mips64-dsp"
     run_dump_test "mips32-mt"
+    run_dump_test "34kc-mt"
 
     run_dump_test "mips16-dwarf2"
     if $has_newabi {
-- 
2.2.1

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

[Maciej W. Rozycki]

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

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

[Maciej W. Rozycki]

On Wed, 22 Mar 2017, Maciej W. Rozycki wrote:

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

 Hmm, I missed how `mips_set_ase' infers the ASE list set on the command 
line.  Apologies for misleading you.

 I still think we don't want to have an extra `init_ase' member only used 
once.  I think we should reinstate the global `file_ase' variable removed 
with commit 919731affbef (and complementing `file_ase_explicit' still 
present there) and adjust the API of `mips_set_ase' so that it operates on 
`unsigned int *ase_flags' instead of `struct mips_set_options *opts', and 
then adjust call sites accordingly to refer to `&file_ase' or 
`&mips_opts.ase' as appropriate.  Then in `file_mips_check_options':

   file_mips_opts.ase &= ~file_ase_explicit;
   file_mips_opts.ase |= file_ase;

will do.  Can you implement this?

  Maciej

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

[Matthew Fortune]

Maciej Rozycki <Maciej.Rozycki@imgtec.com> writes:
> On Wed, 22 Mar 2017, Maciej W. Rozycki wrote:
> 
> > > @@ -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.
> 
>  Hmm, I missed how `mips_set_ase' infers the ASE list set on the command
> line.  Apologies for misleading you.
> 
>  I still think we don't want to have an extra `init_ase' member only used
> once.  I think we should reinstate the global `file_ase' variable removed
> with commit 919731affbef (and complementing `file_ase_explicit' still
> present there) and adjust the API of `mips_set_ase' so that it operates on
> `unsigned int *ase_flags' instead of `struct mips_set_options *opts', and
> then adjust call sites accordingly to refer to `&file_ase' or
> `&mips_opts.ase' as appropriate.  Then in `file_mips_check_options':
> 
>    file_mips_opts.ase &= ~file_ase_explicit;
>    file_mips_opts.ase |= file_ase;
> 
> will do.  Can you implement this?

I'll need to think about this more as although initially I thought your
proposal was going to clean up the solution it has some issues. The main one
being that file_ase, if re-introduced, must be used only to track the ASEs
coming from the selected arch not the explicitly selected ASEs.  This is
because the logic needed in file_mips_check_options is to set up the
overall ASEs to be anything that was explicitly selected and bits from the
architecture ASEs that were not explicitly disabled.  In the long run, and
to some extent just to solve this issue, we are going to want to track all
of:

1) Which ASEs have been explicitly enabled/disabled
2) Which ASEs are enabled overall
3) Which ASEs the chosen architecture enables by default

This data is needed on a global and per .push stack level to calculate the
appropriate ASEs at any given time. (1) needs a new mips_set_options field
and the global file_ase_explicit moving to file_mips_opts.ase_explicit. (2)
is mips_set_options 'ase' field. (3) would be a new field but could actually
be implemented by removing 'arch' and 'isa' from mips_set_options and instead
just keeping a reference to the mips_cpu_info entry for the selected arch
which would then get the ASE list for free. (this also solves the name issue
for warnings and errors).

I believe this scheme would also directly help with having .set arch affect
the ASEs as you indicated should be done with follow up work.

Hope that makes some sense; I can try and elaborate or do a draft implementation
if not.

Matthew

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

[Maciej W. Rozycki]

On Wed, 22 Mar 2017, Matthew Fortune wrote:

> >  I still think we don't want to have an extra `init_ase' member only used
> > once.  I think we should reinstate the global `file_ase' variable removed
> > with commit 919731affbef (and complementing `file_ase_explicit' still
> > present there) and adjust the API of `mips_set_ase' so that it operates on
> > `unsigned int *ase_flags' instead of `struct mips_set_options *opts', and
> > then adjust call sites accordingly to refer to `&file_ase' or
> > `&mips_opts.ase' as appropriate.  Then in `file_mips_check_options':
> > 
> >    file_mips_opts.ase &= ~file_ase_explicit;
> >    file_mips_opts.ase |= file_ase;
> > 
> > will do.  Can you implement this?
> 
> I'll need to think about this more as although initially I thought your
> proposal was going to clean up the solution it has some issues. The main one
> being that file_ase, if re-introduced, must be used only to track the ASEs
> coming from the selected arch not the explicitly selected ASEs.  This is
> because the logic needed in file_mips_check_options is to set up the
> overall ASEs to be anything that was explicitly selected and bits from the
> architecture ASEs that were not explicitly disabled.

 I've meant the opposite arrangement, that is `file_ase' to hold the 
explicitly selected ASEs (further masked by `file_ase_explicit' so that 
both binary states are recorded), not any taken from the architecture.  
That would be achieved by making `mips_set_ase' operate on `&file_ase' in 
`md_parse_option' and on `&mips_opts.ase' elsewhere.

>  In the long run, and
> to some extent just to solve this issue, we are going to want to track all
> of:
> 
> 1) Which ASEs have been explicitly enabled/disabled
> 2) Which ASEs are enabled overall
> 3) Which ASEs the chosen architecture enables by default
> 
> This data is needed on a global and per .push stack level to calculate the
> appropriate ASEs at any given time. (1) needs a new mips_set_options field
> and the global file_ase_explicit moving to file_mips_opts.ase_explicit. (2)
> is mips_set_options 'ase' field. (3) would be a new field but could actually
> be implemented by removing 'arch' and 'isa' from mips_set_options and instead
> just keeping a reference to the mips_cpu_info entry for the selected arch
> which would then get the ASE list for free. (this also solves the name issue
> for warnings and errors).
> 
> I believe this scheme would also directly help with having .set arch affect
> the ASEs as you indicated should be done with follow up work.

 I think any further work on `.set arch=...' should make it just override 
all ASEs as per the architecture selected, ignoring any command-line 
selection.  Any other ASEs required or unwanted by the piece covered by 
`.set arch=...' would have to be controlled by further `.set' pseudo-ops 
that follow.  That goes along with the purpose of `.set arch=...', that is 
to have a piece of code assembled for the architecture selected rather 
than the global settings.  Of course `.set pop' will then restore any 
previous ASE selection.

 AFAICT the only reason why we need the extra complication for ASEs 
explicitly selected on the command line is because we don't process 
command-line options fully in the order they appear on the command line, 
and instead have a complex set of rules, where e.g.: `-mmt -mno-mt' keeps 
the MT ASE disabled as one would expect, but also both `-mno-mt 
-march=34kc' and `-march=34kc -mno-mt' do (barring this regression) while 
one might expect only the latter to, and say `-march=4kc -march=34kc' 
issues a warning rather than silently assuming the `4kc' architecture has 
simply been overridden with `34kc'.

 We don't have this issue with `.module' or `.set' pseudo-ops as they are 
simply interpreted in the order they appear in the source.

  Maciej