[mips] Introduce '.set noase' to the mips assembler.

Message ID 6c8f145a-9203-f4a3-8243-fc99919d92b0@oracle.com
State New
Headers show
Series
  • [mips] Introduce '.set noase' to the mips assembler.
Related show

Commit Message

Egeyar Bagcioglu Sept. 26, 2019, 5:51 p.m.
Hello,

I would like to propose the attached tiny patch, together with the 
lengthy explanation of its use case. Please review.

Currently, the mips assembler keeps, as a part of its runtime state, 
information regarding the accepted ISA and ASEs (Application Specific 
Extensions). It is possible to change the ISA or allowed extensions on 
the fly. It is also possible to use ".set push" to save the state 
regarding the accepted instructions, change them, and pop the previously 
pushed state afterwards.

Linux kernel takes advantage of this situation in its mips inline 
assembly functions. The assembler is asked to push into the stack the 
information of the target platform. The ISA is then set to mips3, which 
is the strictest, to make sure that the assembler generates an error if 
the inline assembler is not accepted by all mips platforms. At the end 
of the inline assembly, the previous state of the assembler is restored 
with a ".set pop":

.set push
.set mips3      #the base ISA
<assembly>
.set pop

The above usage works mostly, when no ASEs are enabled or when the 
enabled ASEs are accepted in mips3 due other settings (such as micromips 
being enabled, etc.). If, however, a conflicting ASE is enabled, the 
switch to mips3 causes the assembler to generate a warning per file 
because mips3 does not support that given ASE. While building a big 
project such as the kernel, one warning per file  with inline assembly 
means several hundreds of warnings in total.

I have considered different ways to solve this issue. If we implicitly 
disable the unsupported ASEs upon switching to a new ISA, we lose 
backwards compatibility. Any project which is -for whatever reason- 
generating some extension instructions that are not supported by the 
given ISA would start getting errors from the assembler. If we decide to 
turn off the warnings under whatever condition, the assembler would 
start accepting unsupported extension instructions without even a 
warning. This would, for example, undermine the kernel's use-case. A 
more-user-friendly option could be delaying the warning. We would not 
warn when a conflicting ISA is set. Instead, we would store this 
information, check the following instructions and only warn if/when we 
see the first instruction using a conflicting ASE. However, I believe 
the overhead of this approach could not be justified by the need.

The change I propose is to add a new ".set noase" directive to disable 
all the ASEs. This pseudo-op would be beneficial primarily within .set 
push/pop pairs of inline assembly functions as used by the kernel, by 
explicitly disabling all 19 extensions of mips architecture and avoiding 
the related warnings. An example usage of it would transform the inline 
assembly functions into the following format:

.set push
.set noase
.set mips3    #the base ISA
<assembly>
.set pop


As .set push/pop pairs also saves and restores the state of enabled 
ASEs, this would indeed serve the purpose of limiting the generation of 
extension instructions within that pair, without permanently changing 
which instructions are allowed. Having said that, the execution of this 
directive does not depend on the .set push/pop pairs. Therefore, it can 
also be used separately.

I do not know how likely it is for the kernel or other projects to use 
this new directive as it would depend on a very new binutils patch. 
Although, it might be applied together with a macro controlling its 
usage. Either way, I still see value in assembler supporting such a usage.

As it is new, this directive does not interfere with the existing 
regression tests. Therefore, you'll see a new test case in the patch as 
well.

The suggested gas/ChangeLog is
2019-09-26  Egeyar Bagcioglu  <egeyar.bagcioglu@oracle.com>

         * config/tc-mips.c (parse_code_option): Introduce ".set/.module
         noase" pseudo-op to disable all Application Specific Extensions.
         * gas/testsuite/gas/mips/ase-errors-5.l: New test.
         * gas/testsuite/gas/mips/ase-errors-5.s: New test source.
         * gas/testsuite/gas/mips/mips.exp: Run the new test.


Please review and let me know what you think.

Thanks,
Egeyar

Comments

Egeyar Bagcioglu Oct. 3, 2019, 2:07 p.m. | #1
ping.

I have corrected the gas/ChangeLog below.
Adding mips maintainers Maciej W. Rozycki and Chenghua Xu.

Regards,
Egeyar


On 9/26/19 7:51 PM, Egeyar Bagcioglu wrote:
> Hello,

>

> I would like to propose the attached tiny patch, together with the 

> lengthy explanation of its use case. Please review.

>

> Currently, the mips assembler keeps, as a part of its runtime state, 

> information regarding the accepted ISA and ASEs (Application Specific 

> Extensions). It is possible to change the ISA or allowed extensions on 

> the fly. It is also possible to use ".set push" to save the state 

> regarding the accepted instructions, change them, and pop the 

> previously pushed state afterwards.

>

> Linux kernel takes advantage of this situation in its mips inline 

> assembly functions. The assembler is asked to push into the stack the 

> information of the target platform. The ISA is then set to mips3, 

> which is the strictest, to make sure that the assembler generates an 

> error if the inline assembler is not accepted by all mips platforms. 

> At the end of the inline assembly, the previous state of the assembler 

> is restored with a ".set pop":

>

> .set push

> .set mips3      #the base ISA

> <assembly>

> .set pop

>

> The above usage works mostly, when no ASEs are enabled or when the 

> enabled ASEs are accepted in mips3 due other settings (such as 

> micromips being enabled, etc.). If, however, a conflicting ASE is 

> enabled, the switch to mips3 causes the assembler to generate a 

> warning per file because mips3 does not support that given ASE. While 

> building a big project such as the kernel, one warning per file  with 

> inline assembly means several hundreds of warnings in total.

>

> I have considered different ways to solve this issue. If we implicitly 

> disable the unsupported ASEs upon switching to a new ISA, we lose 

> backwards compatibility. Any project which is -for whatever reason- 

> generating some extension instructions that are not supported by the 

> given ISA would start getting errors from the assembler. If we decide 

> to turn off the warnings under whatever condition, the assembler would 

> start accepting unsupported extension instructions without even a 

> warning. This would, for example, undermine the kernel's use-case. A 

> more-user-friendly option could be delaying the warning. We would not 

> warn when a conflicting ISA is set. Instead, we would store this 

> information, check the following instructions and only warn if/when we 

> see the first instruction using a conflicting ASE. However, I believe 

> the overhead of this approach could not be justified by the need.

>

> The change I propose is to add a new ".set noase" directive to disable 

> all the ASEs. This pseudo-op would be beneficial primarily within .set 

> push/pop pairs of inline assembly functions as used by the kernel, by 

> explicitly disabling all 19 extensions of mips architecture and 

> avoiding the related warnings. An example usage of it would transform 

> the inline assembly functions into the following format:

>

> .set push

> .set noase

> .set mips3    #the base ISA

> <assembly>

> .set pop

>

>

> As .set push/pop pairs also saves and restores the state of enabled 

> ASEs, this would indeed serve the purpose of limiting the generation 

> of extension instructions within that pair, without permanently 

> changing which instructions are allowed. Having said that, the 

> execution of this directive does not depend on the .set push/pop 

> pairs. Therefore, it can also be used separately.

>

> I do not know how likely it is for the kernel or other projects to use 

> this new directive as it would depend on a very new binutils patch. 

> Although, it might be applied together with a macro controlling its 

> usage. Either way, I still see value in assembler supporting such a 

> usage.

>

> As it is new, this directive does not interfere with the existing 

> regression tests. Therefore, you'll see a new test case in the patch 

> as well.

>

> The suggested gas/ChangeLog is

> 2019-09-26  Egeyar Bagcioglu  <egeyar.bagcioglu@oracle.com>

>

>         * config/tc-mips.c (parse_code_option): Introduce ".set/.module

>         noase" pseudo-op to disable all Application Specific Extensions.

>         * testsuite/gas/mips/ase-errors-5.l: New test.

>         * testsuite/gas/mips/ase-errors-5.s: New test source.

>         * testsuite/gas/mips/mips.exp: Run the new test.

>

>

> Please review and let me know what you think.

>

> Thanks,

> Egeyar
Maciej W. Rozycki Oct. 3, 2019, 2:28 p.m. | #2
On Thu, 3 Oct 2019, Egeyar Bagcioglu wrote:

> I have corrected the gas/ChangeLog below.

> Adding mips maintainers Maciej W. Rozycki and Chenghua Xu.


 This has been discussed before (no mailing list archive pointer offhand, 
but I suppose you can try chasing it now that you know it is there) and I 
reckon the consensus was that we ought to suppress the warning when the 
ISA is demoted to one that does not support an ASE until an instruction 
that is a part of the said ASE is actually seen in input under the new ISA 
setting.  I'd recommend implementing this as it sounds like the best of 
both worlds: it keeps backwards compatibility while silencing the useless 
warning and it does trigger the warning where actually relevant.  You may 
have to reword the warning though.

 You may also check what older GAS versions did before the warning was 
introduced, i.e. whether they actually choked on such an ASE instruction 
under a demoted ISA or silently accepted it.  That would determine how far 
backwards compatibility needs to go, i.e. if it was a working feature back 
then, then we probably want to keep it, but if the bug was only introduced 
with the warning message, then we don't and we just need to bail out on 
the offending instruction rather than warn.

 HTH,

  Maciej
Egeyar Bagcioglu Oct. 7, 2019, 12:57 p.m. | #3
On 10/3/19 4:28 PM, Maciej W. Rozycki wrote:
> On Thu, 3 Oct 2019, Egeyar Bagcioglu wrote:

>

>> I have corrected the gas/ChangeLog below.

>> Adding mips maintainers Maciej W. Rozycki and Chenghua Xu.

>   This has been discussed before (no mailing list archive pointer offhand,

> but I suppose you can try chasing it now that you know it is there) and I

> reckon the consensus was that we ought to suppress the warning when the

> ISA is demoted to one that does not support an ASE until an instruction

> that is a part of the said ASE is actually seen in input under the new ISA

> setting.  I'd recommend implementing this as it sounds like the best of

> both worlds: it keeps backwards compatibility while silencing the useless

> warning and it does trigger the warning where actually relevant.  You may

> have to reword the warning though.

>

>   You may also check what older GAS versions did before the warning was

> introduced, i.e. whether they actually choked on such an ASE instruction

> under a demoted ISA or silently accepted it.  That would determine how far

> backwards compatibility needs to go, i.e. if it was a working feature back

> then, then we probably want to keep it, but if the bug was only introduced

> with the warning message, then we don't and we just need to bail out on

> the offending instruction rather than warn.

>

>   HTH,

>

>    Maciej


Thanks Maciej for the clear explanation.

I was not able to find the discussion you mentioned, unless you meant 
the following two:
https://sourceware.org/bugzilla/show_bug.cgi?id=16288
https://sourceware.org/ml/binutils/2014-05/msg00179.html

The .module directive is only allowed before generating code. The usage 
that I would like the assembler to allow --without warnings-- is .set 
push/pop pairs to be used to encapsulate inline assembly functions in 
order to prevent the usage of higher level assembly instructions within 
the source code of a project, *while also allowing* the generated .s 
files to use the ASEs permitted outside of such pairs.

Whether this is what you meant by the previous discussion or not, I'll 
take your reply above as reference and start implementing the 
functionality you described as soon as I can.

Regards,
Egeyar

Patch

commit d8c1a1be8f1919198cdbc1d63d3f0a4efdb9886f
Author: Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com>
Date:   Tue Sep 24 15:52:47 2019 +0000

    Introduce '.set noase' to the mips assembler.

diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
index b2e4973..69c3fb1 100644
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -205,8 +205,8 @@  struct mips_set_options
      -mipsN command line option, and the default CPU.  */
   int isa;
   /* Enabled Application Specific Extensions (ASEs).  Changed by `.set
-     <asename>', by command line options, and based on the default
-     architecture.  */
+     <asename>', by `.set noase', by command line options, and based on
+     the default architecture.  */
   int ase;
   /* Whether we are assembling for the mips16 processor.  0 if we are
      not, 1 if we are, and -1 if the value has not been initialized.
@@ -16716,6 +16716,8 @@  parse_code_option (char * name)
     mips_opts.sym32 = TRUE;
   else if (strcmp (name, "nosym32") == 0)
     mips_opts.sym32 = FALSE;
+  else if (strcmp (name, "noase") == 0)
+    mips_opts.ase = 0;
   else
     return OPTION_TYPE_BAD;
 
diff --git a/gas/testsuite/gas/mips/ase-errors-5.l b/gas/testsuite/gas/mips/ase-errors-5.l
new file mode 100644
index 0000000..06c60ff
--- /dev/null
+++ b/gas/testsuite/gas/mips/ase-errors-5.l
@@ -0,0 +1,14 @@ 
+.*Assembler messages:
+.*:6: Error: opcode not supported.* `aclr 4,100\(\$4\)'
+# ----------------------------------------------------------------------------
+.*:11: Error: opcode not supported.* `aclr 4,100\(\$4\)'
+# ----------------------------------------------------------------------------
+.*:13: Error: opcode not supported.* `lbux \$4,\$5\(\$6\)'
+.*:14: Error: opcode not supported.* `ldx \$4,\$5\(\$6\)'
+.*:15: Error: opcode not supported.* `aclr 4,100\(\$4\)'
+# ----------------------------------------------------------------------------
+.*:17: Error: opcode not supported.* `lbux \$4,\$5\(\$6\)'
+.*:18: Error: opcode not supported.* `ldx \$4,\$5\(\$6\)'
+# ----------------------------------------------------------------------------
+.*:24: Error: opcode not supported.* `aclr 4,100\(\$4\)'
+# ----------------------------------------------------------------------------
diff --git a/gas/testsuite/gas/mips/ase-errors-5.s b/gas/testsuite/gas/mips/ase-errors-5.s
new file mode 100644
index 0000000..f419c16
--- /dev/null
+++ b/gas/testsuite/gas/mips/ase-errors-5.s
@@ -0,0 +1,24 @@ 
+	.set nomicromips
+	.set mips64r2
+	.set dsp		# OK
+	lbux $4,$5($6)		# OK
+	ldx $4,$5($6)		# OK
+	aclr 4,100($4)		# ERROR: mcu not enabled
+
+	.set push
+	lbux $4,$5($6)		# OK
+	ldx $4,$5($6)		# OK
+	aclr 4,100($4)		# ERROR: mcu not enabled
+	.set noase
+	lbux $4,$5($6)		# ERROR: dsp not enabled
+	ldx $4,$5($6)		# ERROR: dsp not enabled
+	aclr 4,100($4)		# ERROR: mcu not enabled
+	.set mcu
+	lbux $4,$5($6)		# ERROR: dsp not enabled
+	ldx $4,$5($6)		# ERROR: dsp not enabled
+	aclr 4,100($4)		# OK
+	.set pop
+
+	lbux $4,$5($6)		# OK
+	ldx $4,$5($6)		# OK
+	aclr 4,100($4)		# ERROR: mcu not enabled
diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
index 2084ee0..9bb8886 100644
--- a/gas/testsuite/gas/mips/mips.exp
+++ b/gas/testsuite/gas/mips/mips.exp
@@ -1581,6 +1581,7 @@  if { [istarget mips*-*-vxworks*] } {
     run_list_test "ase-errors-2" "-mabi=o64 -march=mips3" "ASE errors (2)"
     run_list_test "ase-errors-3" "-mabi=32 -march=mips1" "ASE errors (3)"
     run_list_test "ase-errors-4" "-mabi=o64 -march=mips3" "ASE errors (4)"
+    run_list_test "ase-errors-5" "-mabi=o64 -march=mips3" "ASE errors (5)"
 
     run_dump_test_arches "la-reloc"	[mips_arch_list_matching mips1]
     run_list_test "dla-warn" "-mabi=32 -march=mips3" \