This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[committed v2] PR gas/21762: MIPS: Fix .stabs directive marking labels as MIPS16
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: James Cowgill <James dot Cowgill at imgtec dot com>
- Cc: <binutils at sourceware dot org>
- Date: Fri, 22 Sep 2017 01:01:06 +0100
- Subject: [committed v2] PR gas/21762: MIPS: Fix .stabs directive marking labels as MIPS16
- Authentication-results: sourceware.org; auth=none
- References: <20170913102756.5842-1-James.Cowgill@imgtec.com>
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