This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[committed v2] PR gas/21762: MIPS: Fix .stabs directive marking labels as MIPS16


From: James Cowgill <James.Cowgill@imgtec.com>

If a .stabs directive was used before another .set directive in a MIPS
source file, s_mips_stab would call mips_mark_labels without having
initialized the mips_opts structure yet.  Fix this by calling
file_mips_check_options which will initialize mips_opts if necessary.

gas/
	PR gas/21762
	* config/tc-mips.c (s_mips_stab): Insert call to
	file_mips_check_options.
	* testsuite/gas/mips/micromips@stabs-symbol-type.d: New test.
	* testsuite/gas/mips/mips.exp: Run the new tests.
	* testsuite/gas/mips/mips16@stabs-symbol-type.d: New test.
	* testsuite/gas/mips/stabs-symbol-type.d: New test.
	* testsuite/gas/mips/stabs-symbol-type.s: New test source.
---
Hi James,

 Thank you for your submission.

 It had several technical issues as noted below, however I have decided to 
correct them on this occasion and apply your change regardless.  Next time 
please take extra care and avoid such problems or I will reject such a 
submission and request a resend with corrections applied.

 Also you didn't explain how you verified your change, and my own testing 
has actually shown that your newly added case triggers several failures 
with 64-bit MIPS targets (which I have also corrected).

 See below for the details of the issues.

> If a .stabs directive was used before another .set directive in a MIPS
> source file, s_mips_stab would call mips_mark_labels without having
> initialized the mips_opts structure yet. Fix this by calling

 Double space is required after a full stop by the GNU Coding Standard.

> file_mips_check_options which will initialize mips_opts if necessary.
> 
> gas/
> 	PR gas/21762
> 	* config/tc-mips.c (s_mips_stab): insert call to
> 	file_mips_check_options.

 s/insert/Insert/

> diff --git a/gas/testsuite/gas/mips/micromips@stabs-symbol-type.d b/gas/testsuite/gas/mips/micromips@stabs-symbol-type.d
> new file mode 100644
> index 0000000000..0b00a3ef8c
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/micromips@stabs-symbol-type.d
> @@ -0,0 +1,10 @@
> +#PROG: readelf
> +#readelf: -s
> +#name: MIPS .stab symbol type
> +#source: stabs-symbol-type.s

 This fails with 64-bit targets such as `mips64-linux', `mips64-freebsd', 
etc., e.g.:

Assembler messages:
Error: -march=mips1 is not compatible with the selected ABI 
.../gas/testsuite/gas/mips/stabs-symbol-type.s:3: Error: `gp=32' used with a 64-bit ABI
.../gas/testsuite/gas/mips/stabs-symbol-type.s:3: Warning: `fp=32' used with a 64-bit ABI

Fixed by adding:

#as: -32

> +# Verify the symbol type when emitting a .stab directive
> +#  In this case, it should be MICROMIPS

 Full stop is required at the end of a sentence by the GNU Coding 
Standard.  I also removed extraneous indentation on the second line.

> diff --git a/gas/testsuite/gas/mips/mips.exp b/gas/testsuite/gas/mips/mips.exp
> index c71dca4351..1a73cfff7b 100644
> --- a/gas/testsuite/gas/mips/mips.exp
> +++ b/gas/testsuite/gas/mips/mips.exp
> @@ -2047,4 +2047,6 @@ if { [istarget mips*-*-vxworks*] } {
>  
>      run_list_test_arches "r6-branch-constraints"  "-32" \
>  			[mips_arch_list_matching mips32r6]
> +
> +    run_dump_test_arches "stabs-symbol-type" [mips_arch_list_all]

 I moved this test invocation ahead of the R6 block, so that the block 
stays last (until we have R7 or suchlike, that is).

> diff --git a/gas/testsuite/gas/mips/mips16@stabs-symbol-type.d b/gas/testsuite/gas/mips/mips16@stabs-symbol-type.d
> new file mode 100644
> index 0000000000..63070b6a65
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/mips16@stabs-symbol-type.d
> @@ -0,0 +1,10 @@
> +#PROG: readelf
> +#readelf: -s
> +#name: MIPS .stab symbol type
> +#source: stabs-symbol-type.s

 Same issue with 64-bit targets.

> +# Verify the symbol type when emitting a .stab directive
> +#  In this case, it should be MIPS16

 Same issue with full stops and indentation.

> diff --git a/gas/testsuite/gas/mips/stabs-symbol-type.d b/gas/testsuite/gas/mips/stabs-symbol-type.d
> new file mode 100644
> index 0000000000..66a664f5c2
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/stabs-symbol-type.d
> @@ -0,0 +1,9 @@
> +#PROG: readelf
> +#readelf: -s
> +#name: MIPS .stab symbol type

 Same issue with 64-bit targets.

> +# Verify the symbol type when emitting a .stab directive
> +#  In this case, it should not be mips16 or micromips

 Same issue with full stops and indentation.  Also capitalisation of 
MIPS16 and MICROMIPS is inconsistent with the other test variants (and 
tool's output, if these markings did actually appear).

 For the record here's the final version actually applied.

  Maciej

---

 gas/config/tc-mips.c                                 |    1 +
 gas/testsuite/gas/mips/micromips@stabs-symbol-type.d |   11 +++++++++++
 gas/testsuite/gas/mips/mips.exp                      |    2 ++
 gas/testsuite/gas/mips/mips16@stabs-symbol-type.d    |   11 +++++++++++
 gas/testsuite/gas/mips/stabs-symbol-type.d           |   10 ++++++++++
 gas/testsuite/gas/mips/stabs-symbol-type.s           |    3 +++
 6 files changed, 38 insertions(+)
 create mode 100644 gas/testsuite/gas/mips/micromips@stabs-symbol-type.d
 create mode 100644 gas/testsuite/gas/mips/mips16@stabs-symbol-type.d
 create mode 100644 gas/testsuite/gas/mips/stabs-symbol-type.d
 create mode 100644 gas/testsuite/gas/mips/stabs-symbol-type.s

binutils-jamesc-mips-gas-stabs-check-options.diff
Index: binutils/gas/config/tc-mips.c
===================================================================
--- binutils.orig/gas/config/tc-mips.c	2017-09-20 21:59:28.098357123 +0100
+++ binutils/gas/config/tc-mips.c	2017-09-21 21:01:58.281587695 +0100
@@ -17149,6 +17149,7 @@ s_nan (int ignore ATTRIBUTE_UNUSED)
 static void
 s_mips_stab (int type)
 {
+  file_mips_check_options ();
   mips_mark_labels ();
   s_stab (type);
 }
Index: binutils/gas/testsuite/gas/mips/micromips@stabs-symbol-type.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/micromips@stabs-symbol-type.d	2017-09-21 23:16:42.408481600 +0100
@@ -0,0 +1,11 @@
+#PROG: readelf
+#readelf: -s
+#name: MIPS .stab symbol type
+#as: -32
+#source: stabs-symbol-type.s
+
+# Verify the symbol type when emitting a .stab directive.
+# In this case, it should be MICROMIPS.
+#...
+ *[0-9]+: +[0-9]+ +[0-9]+ +NOTYPE +LOCAL +DEFAULT +\[MICROMIPS\] +[0-9]+ foo
+#pass
Index: binutils/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils.orig/gas/testsuite/gas/mips/mips.exp	2017-09-20 22:42:28.852946891 +0100
+++ binutils/gas/testsuite/gas/mips/mips.exp	2017-09-21 21:04:30.503694346 +0100
@@ -2035,6 +2035,8 @@ if { [istarget mips*-*-vxworks*] } {
     run_dump_test "org-11"
     run_dump_test "org-12"
 
+    run_dump_test_arches "stabs-symbol-type" [mips_arch_list_all]
+
     run_dump_test_arches "r6"		[mips_arch_list_matching mips32r6]
     if $has_newabi {
 	run_dump_test_arches "r6-n32"	[mips_arch_list_matching mips64r6]
Index: binutils/gas/testsuite/gas/mips/mips16@stabs-symbol-type.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/mips16@stabs-symbol-type.d	2017-09-21 23:16:51.988688549 +0100
@@ -0,0 +1,11 @@
+#PROG: readelf
+#readelf: -s
+#name: MIPS .stab symbol type
+#as: -32
+#source: stabs-symbol-type.s
+
+# Verify the symbol type when emitting a .stab directive.
+# In this case, it should be MIPS16.
+#...
+ *[0-9]+: +[0-9]+ +[0-9]+ +NOTYPE +LOCAL +DEFAULT +\[MIPS16\] +[0-9]+ foo
+#pass
Index: binutils/gas/testsuite/gas/mips/stabs-symbol-type.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/stabs-symbol-type.d	2017-09-21 23:16:56.446962760 +0100
@@ -0,0 +1,10 @@
+#PROG: readelf
+#readelf: -s
+#name: MIPS .stab symbol type
+#as: -32
+
+# Verify the symbol type when emitting a .stab directive.
+# In this case, it should not be MIPS16 or MICROMIPS.
+#...
+ *[0-9]+: +[0-9]+ +[0-9]+ +NOTYPE +LOCAL +DEFAULT +[0-9]+ foo
+#pass
Index: binutils/gas/testsuite/gas/mips/stabs-symbol-type.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/gas/testsuite/gas/mips/stabs-symbol-type.s	2017-09-21 21:01:58.382285709 +0100
@@ -0,0 +1,3 @@
+	.text
+foo:
+	.stabd 0, 0, 0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]