* [PATCH 4/4] MIPS/GAS: Propagate symbol attributes
@ 2010-07-26 10:48 Maciej W. Rozycki
2010-10-29 14:40 ` [PATCH 4.0/4 v2] " Maciej W. Rozycki
0 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2010-07-26 10:48 UTC (permalink / raw)
To: binutils; +Cc: Richard Sandiford, Catherine Moore, gnu-mips-sgxx
Hi,
There are several cases where ELF symbol attributes are not correctly set
for symbols leading to all kinds of odd side effects for MIPS16 code (and
with the upcoming change also for microMIPS code).
For example this program:
.text
foo:
xor $16, $17
.set fnord, . + 2
addu $2, $3, $4
xor $5, $6
bar:
subu $7, $16
assembles to this:
Disassembly of section .text:
00000000 <foo>:
0: e82e xor s0,s1
2: e389 addu v0,v1,a0
00000004 <fnord>:
4: edcee71f swc3 $14,-6369(t6)
00000006 <bar>:
6: e71f subu a3,s0
Notice how fnord's ELF attributes have not been set to indicate a MIPS16
symbol and the resulting confusion (this symbol, of course, if used as a
jump target will not set the ISA bit correctly). Similar symptoms are
seen with equated symbols (defined with .eqv) although the attributes are
lost at a different stage of assembly.
Here's a fix for these problems and a test case covering hopefully most
of them as well as those fixed by patches submitted previously in this series.
There are two new functions defined:
- mips_elf_copy_symbol_attributes() -- used for symbols defined with .set.
These, if calculated from a label that has been defined for the current
location (the "dot" special symbol being a prominent example, but any
label will actually do) will not have their ELF attributes set, because
the original label will only get them set once an instruction has been
emitted. A solution is to place the newly defined symbol on the list of
labels too.
- mips_elf_propagate_symbol_attributes() -- used for symbols defined with
.eqv. They are processed late and currently attributes are copied only
for exact copies of other symbols, i.e.:
.eqv foo, bar
but not:
.eqv foo, bar + 2
In the case of MIPS16 (and microMIPS) ELF attribute we want it to be
propagated even for offsetted symbols, hence the new hook in
resolve_symbol_value().
Finally I realised the uniqueness of the -mmips:16 option to `objdump'
that causes all the disassembled to be treated as MIPS16 code, regardless
of symbol annotations found. This actually covers the bugs addressed
here, hence I'm removing it. The implication is all MIPS16 tests need to
have a label at the beginning to set the ISA mode of the disassembler
correctly.
Perhaps the behaviour of the -mmips:16 option should be changed instead
so that it respects symbol annotations. I think it would make sense --
the option should normally only be needed for binary objects with no
sufficient symbol information. OTOH, the current behaviour is good in
case `objdump' gets confused because of a bug or a broken binary, hence I
have no strong preference actually towards making the change.
If we agree to make it after all, then the option can be added back. I
believe -mmips:micromips behaves the same. Additionally odd text symbols
with no ELF annotation are unconditionally treated as MIPS16 code. This
should probably be changed.
2010-07-26 Maciej W. Rozycki <macro@codesourcery.com>
gas/
* config/tc-mips.c (insn_label_recording): New variable.
(md_begin): Set insn_label_recording.
(md_mips_end): Clear insn_label_recording.
(mips_elf_copy_symbol_attributes): New function.
(mips_elf_propagate_symbol_attributes): New function.
* config/tc-mips.h (TC_COPY_SYMBOL_ATTRIBUTES): New macro.
(TC_PROPAGATE_SYMBOL_ATTRIBUTES): Likewise.
(mips_elf_copy_symbol_attributes): New prototype.
(mips_elf_propagate_symbol_attributes): Likewise.
* symbols.c (resolve_symbol_value): Call
TC_PROPAGATE_SYMBOL_ATTRIBUTES for symbols that are not exact
copies of originals.
gas/testsuite/
* gas/mips/branch-self.d: New test for various definitions of
labels.
* gas/mips/branch-self.s: Source for the new test.
* gas/mips/mips.exp: Run the new test. Remove -mmips:16 from
"mips16" architecture.
OK to apply?
Maciej
binutils-gas-propagate-attrs.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2010-07-25 03:52:42.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2010-07-25 23:49:09.000000000 +0100
@@ -1254,6 +1254,8 @@ struct insn_label_list
static struct insn_label_list *free_insn_labels;
#define label_list tc_segment_info_data.labels
+static int insn_label_recording;
+
static void mips_clear_insn_labels (void);
static inline void
@@ -2093,11 +2095,15 @@ md_begin (void)
if (mips_fix_vr4120)
init_vr4120_conflicts ();
+
+ insn_label_recording = 1;
}
void
md_mips_end (void)
{
+ insn_label_recording = 0;
+
if (! ECOFF_DEBUGGING)
md_obj_end ();
}
@@ -14770,9 +14776,38 @@ mips_symbol_new_hook (symbolS *sym)
&& S_GET_VALUE (sym) == frag_now_fix ())
mips_record_label (sym);
}
-\f
+
#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+/* Record copies of instruction labels made, for later MIPS16 symbol
+ annotation. */
+
+void
+mips_elf_copy_symbol_attributes (symbolS *dest, symbolS *src ATTRIBUTE_UNUSED)
+{
+ if (!IS_ELF || !insn_label_recording)
+ return;
+
+ if (S_GET_SEGMENT (src) == now_seg
+ && symbol_get_frag (src) == frag_now
+ && symbol_constant_p (src)
+ && S_GET_VALUE (src) == frag_now_fix ()
+ && symbol_constant_p (dest))
+ mips_record_label (dest);
+}
+
+/* Propagate MIPS16 symbol annotation. */
+
+void
+mips_elf_propagate_symbol_attributes (symbolS *dest, symbolS *src)
+{
+ if (!IS_ELF)
+ return;
+
+ if (ELF_ST_IS_MIPS16 (S_GET_OTHER (src)))
+ S_SET_OTHER (dest, ELF_ST_SET_MIPS16 (S_GET_OTHER (dest)));
+}
+\f
/* Some special processing for a MIPS ELF file. */
void
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.h 2010-07-25 03:35:59.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.h 2010-07-25 18:48:50.000000000 +0100
@@ -121,6 +121,14 @@ extern void mips_frob_file (void);
#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
#define tc_frob_file_after_relocs mips_frob_file_after_relocs
extern void mips_frob_file_after_relocs (void);
+
+#define TC_COPY_SYMBOL_ATTRIBUTES(dest, src) \
+ mips_elf_copy_symbol_attributes (dest, src)
+extern void mips_elf_copy_symbol_attributes (symbolS *, symbolS *);
+
+#define TC_PROPAGATE_SYMBOL_ATTRIBUTES(dest, src) \
+ mips_elf_propagate_symbol_attributes (dest, src)
+extern void mips_elf_propagate_symbol_attributes (symbolS *, symbolS *);
#endif
#define tc_fix_adjustable(fixp) mips_fix_adjustable (fixp)
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c 2010-07-25 03:35:59.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c 2010-07-25 18:48:50.000000000 +0100
@@ -1182,12 +1182,19 @@ resolve_symbol_value (symbolS *symp)
break;
}
- if (finalize_syms && final_val == 0)
+ if (finalize_syms)
{
- if (LOCAL_SYMBOL_CHECK (add_symbol))
- add_symbol = local_symbol_convert ((struct local_symbol *)
- add_symbol);
- copy_symbol_attributes (symp, add_symbol);
+ if (final_val == 0)
+ {
+ if (LOCAL_SYMBOL_CHECK (add_symbol))
+ add_symbol = local_symbol_convert ((struct local_symbol *)
+ add_symbol);
+ copy_symbol_attributes (symp, add_symbol);
+ }
+#ifdef TC_PROPAGATE_SYMBOL_ATTRIBUTES
+ else
+ TC_PROPAGATE_SYMBOL_ATTRIBUTES (symp, add_symbol);
+#endif
}
/* If we have equated this symbol to an undefined or common
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d 2010-07-25 03:56:22.000000000 +0100
@@ -0,0 +1,29 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self.
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s 2010-07-25 03:56:22.000000000 +0100
@@ -0,0 +1,30 @@
+# Source file used to test branches to self.
+ .text
+foo:
+ sw $2, ($3)
+ b .
+
+ sw $2, ($3)
+0:
+ b 0b
+
+ sw $2, ($3)
+bar:
+ b bar
+
+ sw $2, ($3)
+ .set frob, .
+ b frob
+
+ .eqv fnord, .
+ sw $2, ($3)
+ b fnord
+
+ .eqv foobar, fnord + 4
+ .eqv foobaz, foobar - 16
+ sw $2, ($3)
+ b foobaz + 12
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 8
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2010-07-25 03:35:59.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2010-07-25 18:48:50.000000000 +0100
@@ -368,7 +368,7 @@ mips_arch_create mips64r2 64 mips64 { mi
{ -mmips:isa64r2 } \
{ mipsisa64r2-*-* mipsisa64r2el-*-* }
mips_arch_create mips16 32 {} {} \
- { -march=mips1 -mips16 } { -mmips:16 }
+ { -march=mips1 -mips16 } {}
mips_arch_create r3000 32 mips1 {} \
{ -march=r3000 -mtune=r3000 } { -mmips:3000 }
mips_arch_create r3900 32 mips1 { gpr_ilocks } \
@@ -448,6 +448,7 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test_arches "branch-misc-2pic-64" [mips_arch_list_matching mips3]
run_dump_test "branch-misc-3"
run_dump_test "branch-swap"
+ run_dump_test_arches "branch-self" [mips_arch_list_all]
run_dump_test "div"
if { !$addr32 } {
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d 2010-07-25 03:56:22.000000000 +0100
@@ -0,0 +1,23 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+ \.\.\.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4.0/4 v2] MIPS/GAS: Propagate symbol attributes
2010-07-26 10:48 [PATCH 4/4] MIPS/GAS: Propagate symbol attributes Maciej W. Rozycki
@ 2010-10-29 14:40 ` Maciej W. Rozycki
2010-10-30 10:37 ` Richard Sandiford
0 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2010-10-29 14:40 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
Hi,
There are several cases where ELF symbol attributes are not correctly set
for symbols leading to all kinds of odd side effects for MIPS16 code (and
with the upcoming change also for microMIPS code).
For example this program:
.text
foo:
xor $16, $17
.set fnord, . + 2
addu $2, $3, $4
xor $5, $6
bar:
subu $7, $16
assembles to this:
Disassembly of section .text:
00000000 <foo>:
0: e82e xor s0,s1
2: e389 addu v0,v1,a0
00000004 <fnord>:
4: edcee71f swc3 $14,-6369(t6)
00000006 <bar>:
6: e71f subu a3,s0
Notice how fnord's ELF attributes have not been set to indicate a MIPS16
symbol and the resulting confusion (this symbol, of course, if used as a
jump target will not set the ISA bit correctly). Similar symptoms are
seen with equated symbols (defined with .eqv) although the attributes are
lost at a different stage of assembly.
Here's a fix for these problems and a test case covering hopefully most
of them as well as those fixed by patches submitted previously in this series.
There are two new functions defined:
- mips_elf_copy_symbol_attributes() -- used for symbols defined with .set.
These, if calculated from a label that has been defined for the current
location (the "dot" special symbol being a prominent example, but any
label will actually do) will not have their ELF attributes set, because
the original label will only get them set once an instruction has been
emitted. A solution is to place the newly defined symbol on the list of
labels too.
- mips_elf_propagate_symbol_attributes() -- used for symbols defined with
.eqv. They are processed late and currently attributes are copied only
for exact copies of other symbols, i.e.:
.eqv foo, bar
but not:
.eqv foo, bar + 2
In the case of MIPS16 (and microMIPS) ELF attribute we want it to be
propagated even for offsetted symbols, hence the new hook in
resolve_symbol_value().
Finally I realised the uniqueness of the -mmips:16 option to `objdump'
that causes all the disassembled to be treated as MIPS16 code, regardless
of symbol annotations found. This actually covers the bugs addressed
here, hence I'm removing it. The implication is all MIPS16 tests need to
have a label at the beginning to set the ISA mode of the disassembler
correctly.
Perhaps the behaviour of the -mmips:16 option should be changed instead
so that it respects symbol annotations. I think it would make sense --
the option should normally only be needed for binary objects with no
sufficient symbol information. OTOH, the current behaviour is good in
case `objdump' gets confused because of a bug or a broken binary, hence I
have no strong preference actually towards making the change.
If we agree to make it after all, then the option can be added back. I
believe -mmips:micromips behaves the same. Additionally odd text symbols
with no ELF annotation are unconditionally treated as MIPS16 code. This
should probably be changed.
Compared to the original version, this change has only been trivially
updated to take the new treatment of symbols equated to an expression
involving "." into account. The update is limited to the test case
according to the comment I'll just repeat here: "Move the location counter
away from the end of code to avoid the final values of symbols equated to
expressions involving the counter interfering with disassembly." Without
this change MIPS16 code would be disassembled incorrectly as the equated
symbols have the MIPS16 attribute clear (quite correctly, because the
final value of "." does not point to MIPS16 code). Dumps have been
adjusted accordingly.
2010-10-29 Maciej W. Rozycki <macro@codesourcery.com>
gas/
* config/tc-mips.c (insn_label_recording): New variable.
(md_begin): Set insn_label_recording.
(md_mips_end): Clear insn_label_recording.
(mips_elf_copy_symbol_attributes): New function.
(mips_elf_propagate_symbol_attributes): New function.
* config/tc-mips.h (TC_COPY_SYMBOL_ATTRIBUTES): New macro.
(TC_PROPAGATE_SYMBOL_ATTRIBUTES): Likewise.
(mips_elf_copy_symbol_attributes): New prototype.
(mips_elf_propagate_symbol_attributes): Likewise.
* symbols.c (resolve_symbol_value): Call
TC_PROPAGATE_SYMBOL_ATTRIBUTES for symbols that are not exact
copies of originals.
gas/testsuite/
* gas/mips/branch-self.d: New test for various definitions of
labels.
* gas/mips/branch-self.s: Source for the new test.
* gas/mips/mips.exp: Run the new test. Remove -mmips:16 from
"mips16" architecture.
OK to apply?
Maciej
binutils-gas-propagate-attrs.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2010-10-29 09:07:10.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2010-10-29 09:07:11.000000000 +0100
@@ -1256,6 +1256,8 @@ struct insn_label_list
static struct insn_label_list *free_insn_labels;
#define label_list tc_segment_info_data.labels
+static int insn_label_recording;
+
static void mips_clear_insn_labels (void);
static inline void
@@ -2095,11 +2097,15 @@ md_begin (void)
if (mips_fix_vr4120)
init_vr4120_conflicts ();
+
+ insn_label_recording = 1;
}
void
md_mips_end (void)
{
+ insn_label_recording = 0;
+
if (! ECOFF_DEBUGGING)
md_obj_end ();
}
@@ -14726,9 +14732,38 @@ mips_symbol_new_hook (symbolS *sym)
&& S_GET_VALUE (sym) == frag_now_fix ())
mips_record_label (sym);
}
-\f
+
#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+/* Record copies of instruction labels made, for later MIPS16 symbol
+ annotation. */
+
+void
+mips_elf_copy_symbol_attributes (symbolS *dest, symbolS *src ATTRIBUTE_UNUSED)
+{
+ if (!IS_ELF || !insn_label_recording)
+ return;
+
+ if (S_GET_SEGMENT (src) == now_seg
+ && symbol_get_frag (src) == frag_now
+ && symbol_constant_p (src)
+ && S_GET_VALUE (src) == frag_now_fix ()
+ && symbol_constant_p (dest))
+ mips_record_label (dest);
+}
+
+/* Propagate MIPS16 symbol annotation. */
+
+void
+mips_elf_propagate_symbol_attributes (symbolS *dest, symbolS *src)
+{
+ if (!IS_ELF)
+ return;
+
+ if (ELF_ST_IS_MIPS16 (S_GET_OTHER (src)))
+ S_SET_OTHER (dest, ELF_ST_SET_MIPS16 (S_GET_OTHER (dest)));
+}
+\f
/* Some special processing for a MIPS ELF file. */
void
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.h 2010-10-29 09:07:10.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.h 2010-10-29 09:07:11.000000000 +0100
@@ -121,6 +121,14 @@ extern void mips_frob_file (void);
#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
#define tc_frob_file_after_relocs mips_frob_file_after_relocs
extern void mips_frob_file_after_relocs (void);
+
+#define TC_COPY_SYMBOL_ATTRIBUTES(dest, src) \
+ mips_elf_copy_symbol_attributes (dest, src)
+extern void mips_elf_copy_symbol_attributes (symbolS *, symbolS *);
+
+#define TC_PROPAGATE_SYMBOL_ATTRIBUTES(dest, src) \
+ mips_elf_propagate_symbol_attributes (dest, src)
+extern void mips_elf_propagate_symbol_attributes (symbolS *, symbolS *);
#endif
#define tc_fix_adjustable(fixp) mips_fix_adjustable (fixp)
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c 2010-10-29 09:07:10.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c 2010-10-29 09:07:11.000000000 +0100
@@ -1176,12 +1176,19 @@ resolve_symbol_value (symbolS *symp)
break;
}
- if (finalize_syms && final_val == 0)
+ if (finalize_syms)
{
- if (LOCAL_SYMBOL_CHECK (add_symbol))
- add_symbol = local_symbol_convert ((struct local_symbol *)
- add_symbol);
- copy_symbol_attributes (symp, add_symbol);
+ if (final_val == 0)
+ {
+ if (LOCAL_SYMBOL_CHECK (add_symbol))
+ add_symbol = local_symbol_convert ((struct local_symbol *)
+ add_symbol);
+ copy_symbol_attributes (symp, add_symbol);
+ }
+#ifdef TC_PROPAGATE_SYMBOL_ATTRIBUTES
+ else
+ TC_PROPAGATE_SYMBOL_ATTRIBUTES (symp, add_symbol);
+#endif
}
/* If we have equated this symbol to an undefined or common
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d 2010-10-29 09:07:11.000000000 +0100
@@ -0,0 +1,30 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self.
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+ \.\.\.
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s 2010-10-29 09:07:11.000000000 +0100
@@ -0,0 +1,35 @@
+# Source file used to test branches to self.
+ .text
+foo:
+ sw $2, ($3)
+ b .
+
+ sw $2, ($3)
+0:
+ b 0b
+
+ sw $2, ($3)
+bar:
+ b bar
+
+ sw $2, ($3)
+ .set frob, .
+ b frob
+
+ .eqv fnord, .
+ sw $2, ($3)
+ b fnord
+
+ .eqv foobar, fnord + 4
+ .eqv foobaz, foobar - 16
+ sw $2, ($3)
+ b foobaz + 12
+
+# Move the location counter away from the end of code to avoid the final
+# values of symbols equated to expressions involving the counter interfering
+# with disassembly.
+ .space 16
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 8
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2010-10-29 09:07:11.000000000 +0100
@@ -390,7 +390,7 @@ mips_arch_create mips64r2 64 mips64 { mi
{ -mmips:isa64r2 } \
{ mipsisa64r2-*-* mipsisa64r2el-*-* }
mips_arch_create mips16 32 {} {} \
- { -march=mips1 -mips16 } { -mmips:16 }
+ { -march=mips1 -mips16 } {}
mips_arch_create r3000 32 mips1 {} \
{ -march=r3000 -mtune=r3000 } { -mmips:3000 }
mips_arch_create r3900 32 mips1 { gpr_ilocks } \
@@ -466,6 +466,7 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test_arches "branch-misc-2pic-64" [mips_arch_list_matching mips3]
run_dump_test "branch-misc-3"
run_dump_test "branch-swap"
+ run_dump_test_arches "branch-self" [mips_arch_list_all]
run_dump_test "div"
if { !$addr32 } {
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d 2010-10-29 09:07:11.000000000 +0100
@@ -0,0 +1,24 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+ \.\.\.
+ \.\.\.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.0/4 v2] MIPS/GAS: Propagate symbol attributes
2010-10-29 14:40 ` [PATCH 4.0/4 v2] " Maciej W. Rozycki
@ 2010-10-30 10:37 ` Richard Sandiford
2010-12-08 19:24 ` [PATCH 4.0/4 v3] " Maciej W. Rozycki
0 siblings, 1 reply; 7+ messages in thread
From: Richard Sandiford @ 2010-10-30 10:37 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Catherine Moore, binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> There are several cases where ELF symbol attributes are not correctly set
> for symbols leading to all kinds of odd side effects for MIPS16 code (and
> with the upcoming change also for microMIPS code).
>
> For example this program:
>
> .text
> foo:
> xor $16, $17
> .set fnord, . + 2
> addu $2, $3, $4
> xor $5, $6
> bar:
> subu $7, $16
>
> assembles to this:
>
> Disassembly of section .text:
>
> 00000000 <foo>:
> 0: e82e xor s0,s1
> 2: e389 addu v0,v1,a0
>
> 00000004 <fnord>:
> 4: edcee71f swc3 $14,-6369(t6)
>
> 00000006 <bar>:
> 6: e71f subu a3,s0
>
> Notice how fnord's ELF attributes have not been set to indicate a MIPS16
> symbol and the resulting confusion (this symbol, of course, if used as a
> jump target will not set the ISA bit correctly). Similar symptoms are
> seen with equated symbols (defined with .eqv) although the attributes are
> lost at a different stage of assembly.
>
> Here's a fix for these problems and a test case covering hopefully most
> of them as well as those fixed by patches submitted previously in this series.
>
> There are two new functions defined:
>
> - mips_elf_copy_symbol_attributes() -- used for symbols defined with .set.
> These, if calculated from a label that has been defined for the current
> location (the "dot" special symbol being a prominent example, but any
> label will actually do) will not have their ELF attributes set, because
> the original label will only get them set once an instruction has been
> emitted. A solution is to place the newly defined symbol on the list of
> labels too.
>
> - mips_elf_propagate_symbol_attributes() -- used for symbols defined with
> .eqv. They are processed late and currently attributes are copied only
> for exact copies of other symbols, i.e.:
>
> .eqv foo, bar
>
> but not:
>
> .eqv foo, bar + 2
>
> In the case of MIPS16 (and microMIPS) ELF attribute we want it to be
> propagated even for offsetted symbols, hence the new hook in
> resolve_symbol_value().
>
> Finally I realised the uniqueness of the -mmips:16 option to `objdump'
> that causes all the disassembled to be treated as MIPS16 code, regardless
> of symbol annotations found. This actually covers the bugs addressed
> here, hence I'm removing it. The implication is all MIPS16 tests need to
> have a label at the beginning to set the ISA mode of the disassembler
> correctly.
>
> Perhaps the behaviour of the -mmips:16 option should be changed instead
> so that it respects symbol annotations. I think it would make sense --
> the option should normally only be needed for binary objects with no
> sufficient symbol information. OTOH, the current behaviour is good in
> case `objdump' gets confused because of a bug or a broken binary, hence I
> have no strong preference actually towards making the change.
>
> If we agree to make it after all, then the option can be added back. I
> believe -mmips:micromips behaves the same. Additionally odd text symbols
> with no ELF annotation are unconditionally treated as MIPS16 code. This
> should probably be changed.
>
> Compared to the original version, this change has only been trivially
> updated to take the new treatment of symbols equated to an expression
> involving "." into account. The update is limited to the test case
> according to the comment I'll just repeat here: "Move the location counter
> away from the end of code to avoid the final values of symbols equated to
> expressions involving the counter interfering with disassembly." Without
> this change MIPS16 code would be disassembled incorrectly as the equated
> symbols have the MIPS16 attribute clear (quite correctly, because the
> final value of "." does not point to MIPS16 code). Dumps have been
> adjusted accordingly.
Sorry for not really getting to this patch when you posted the original
series. Do you actually have a "real world" use case for this though?
Why wouldn't you just put "fnord:" in the appropriate place?
I'm not certain that, in general, we can say that "mips16 symbol + offset"
should always be a mips16 symbol.
Richard
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4.0/4 v3] MIPS/GAS: Propagate symbol attributes
2010-10-30 10:37 ` Richard Sandiford
@ 2010-12-08 19:24 ` Maciej W. Rozycki
2010-12-09 17:25 ` Richard Sandiford
0 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2010-12-08 19:24 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
Hi Richard,
On Sat, 30 Oct 2010, Richard Sandiford wrote:
> > There are several cases where ELF symbol attributes are not correctly set
> > for symbols leading to all kinds of odd side effects for MIPS16 code (and
> > with the upcoming change also for microMIPS code).
> >
> > For example this program:
> >
> > .text
> > foo:
> > xor $16, $17
> > .set fnord, . + 2
> > addu $2, $3, $4
> > xor $5, $6
> > bar:
> > subu $7, $16
> >
> > assembles to this:
> >
> > Disassembly of section .text:
> >
> > 00000000 <foo>:
> > 0: e82e xor s0,s1
> > 2: e389 addu v0,v1,a0
> >
> > 00000004 <fnord>:
> > 4: edcee71f swc3 $14,-6369(t6)
> >
> > 00000006 <bar>:
> > 6: e71f subu a3,s0
> >
> > Notice how fnord's ELF attributes have not been set to indicate a MIPS16
> > symbol and the resulting confusion (this symbol, of course, if used as a
> > jump target will not set the ISA bit correctly). Similar symptoms are
> > seen with equated symbols (defined with .eqv) although the attributes are
> > lost at a different stage of assembly.
> >
> > Here's a fix for these problems and a test case covering hopefully most
> > of them as well as those fixed by patches submitted previously in this series.
> >
> > There are two new functions defined:
> >
> > - mips_elf_copy_symbol_attributes() -- used for symbols defined with .set.
> > These, if calculated from a label that has been defined for the current
> > location (the "dot" special symbol being a prominent example, but any
> > label will actually do) will not have their ELF attributes set, because
> > the original label will only get them set once an instruction has been
> > emitted. A solution is to place the newly defined symbol on the list of
> > labels too.
> >
> > - mips_elf_propagate_symbol_attributes() -- used for symbols defined with
> > .eqv. They are processed late and currently attributes are copied only
> > for exact copies of other symbols, i.e.:
> >
> > .eqv foo, bar
> >
> > but not:
> >
> > .eqv foo, bar + 2
> >
> > In the case of MIPS16 (and microMIPS) ELF attribute we want it to be
> > propagated even for offsetted symbols, hence the new hook in
> > resolve_symbol_value().
> >
> > Finally I realised the uniqueness of the -mmips:16 option to `objdump'
> > that causes all the disassembled to be treated as MIPS16 code, regardless
> > of symbol annotations found. This actually covers the bugs addressed
> > here, hence I'm removing it. The implication is all MIPS16 tests need to
> > have a label at the beginning to set the ISA mode of the disassembler
> > correctly.
> >
> > Perhaps the behaviour of the -mmips:16 option should be changed instead
> > so that it respects symbol annotations. I think it would make sense --
> > the option should normally only be needed for binary objects with no
> > sufficient symbol information. OTOH, the current behaviour is good in
> > case `objdump' gets confused because of a bug or a broken binary, hence I
> > have no strong preference actually towards making the change.
> >
> > If we agree to make it after all, then the option can be added back. I
> > believe -mmips:micromips behaves the same. Additionally odd text symbols
> > with no ELF annotation are unconditionally treated as MIPS16 code. This
> > should probably be changed.
As a side note I have actually fixed this oddity with the lastest
microMIPS patch -- odd text symbols with no ELF annotation are treated as
either MIPS16 or microMIPS code based on the ELF file header flags; if no
MIPS16 or microMIPS flag is set, then the MIPS16 ASE is assumed preserving
the current behaviour.
> > Compared to the original version, this change has only been trivially
> > updated to take the new treatment of symbols equated to an expression
> > involving "." into account. The update is limited to the test case
> > according to the comment I'll just repeat here: "Move the location counter
> > away from the end of code to avoid the final values of symbols equated to
> > expressions involving the counter interfering with disassembly." Without
> > this change MIPS16 code would be disassembled incorrectly as the equated
> > symbols have the MIPS16 attribute clear (quite correctly, because the
> > final value of "." does not point to MIPS16 code). Dumps have been
> > adjusted accordingly.
>
> Sorry for not really getting to this patch when you posted the original
> series. Do you actually have a "real world" use case for this though?
> Why wouldn't you just put "fnord:" in the appropriate place?
Please note this issue only affects disassembly -- I have not observed
any other unexpected symptoms. In particular all the relocated entities
referring to the symbols concerned get the ISA mode set regardless of this
change.
So it's not about whether we should treat these symbols as MIPS16 code
references or not, but to match the reality and not mislead the user into
thinking standard MIPS code is being concerned.
> I'm not certain that, in general, we can say that "mips16 symbol + offset"
> should always be a mips16 symbol.
Well, TBH I think it should be doing so by definition -- if you offset a
MIPS16 symbol (i.e. a text symbol with ".set mips16" in effect), then you
get a MIPS16 symbol (i.e. another text symbol carrying the ".set mips16"
property). If you don't want to get a MIPS16 symbol, then start with a
non-MIPS16 symbol in the first place.
For example this piece of code does the right thing (which may be good to
know to some in case a piece of MIPS16 code has to be referred to as data
for run-time relocation or something):
$ cat mips16.s
.text
.set mips16
.globl foo
.globl bar
foo:
.fill 0
bar:
jr $31
.data
.word foo
.word bar
$ mips-sde-elf-as -o mips16.o mips16.s
$ mips-sde-elf-ld -ebar -o mips16 mips16.o
$ mips-sde-elf-objdump -s -j .data mips16
mips16: file format elf32-tradbigmips
Contents of section .data:
410078 00400074 00400075 .@.t.@.u
[As it is `objdump -d' doesn't get the disassembly of bar() right, but I
believe it's fixed as a side effect of the fix made with the microMIPS
change side-noted above.] You can then offset "foo" or "bar" further as
required and get the desired result.
I have updated the change to include documentation in internals.texi and
regenerated it against the current shape of the code patched. No other
changes compared to the original and it still passes regression tests for
mips-sde-elf and mips-linux-gnu.
2010-12-08 Maciej W. Rozycki <macro@codesourcery.com>
gas/
* config/tc-mips.c (insn_label_recording): New variable.
(md_begin): Set insn_label_recording.
(md_mips_end): Clear insn_label_recording.
(mips_elf_copy_symbol_attributes): New function.
(mips_elf_propagate_symbol_attributes): New function.
* config/tc-mips.h (TC_COPY_SYMBOL_ATTRIBUTES): New macro.
(TC_PROPAGATE_SYMBOL_ATTRIBUTES): Likewise.
(mips_elf_copy_symbol_attributes): New prototype.
(mips_elf_propagate_symbol_attributes): Likewise.
* symbols.c (resolve_symbol_value): Call
TC_PROPAGATE_SYMBOL_ATTRIBUTES for symbols that are not exact
copies of originals.
* doc/internals.texi (CPU backend): Document
TC_PROPAGATE_SYMBOL_ATTRIBUTES.
gas/testsuite/
* gas/mips/branch-self.d: New test for various definitions of
labels.
* gas/mips/branch-self.s: Source for the new test.
* gas/mips/mips.exp: Run the new test. Remove -mmips:16 from
"mips16" architecture.
Maciej
binutils-gas-propagate-attrs.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2010-12-08 00:26:16.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2010-12-08 00:26:16.000000000 +0000
@@ -1265,6 +1265,8 @@ struct insn_label_list
static struct insn_label_list *free_insn_labels;
#define label_list tc_segment_info_data.labels
+static int insn_label_recording;
+
static void mips_clear_insn_labels (void);
static inline void
@@ -2104,11 +2106,15 @@ md_begin (void)
if (mips_fix_vr4120)
init_vr4120_conflicts ();
+
+ insn_label_recording = 1;
}
void
md_mips_end (void)
{
+ insn_label_recording = 0;
+
if (! ECOFF_DEBUGGING)
md_obj_end ();
}
@@ -14728,9 +14734,38 @@ mips_define_label (symbolS *sym)
dwarf2_emit_label (sym);
#endif
}
-\f
+
#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
+/* Record copies of instruction labels made, for later MIPS16 symbol
+ annotation. */
+
+void
+mips_elf_copy_symbol_attributes (symbolS *dest, symbolS *src ATTRIBUTE_UNUSED)
+{
+ if (!IS_ELF || !insn_label_recording)
+ return;
+
+ if (S_GET_SEGMENT (src) == now_seg
+ && symbol_get_frag (src) == frag_now
+ && symbol_constant_p (src)
+ && S_GET_VALUE (src) == frag_now_fix ()
+ && symbol_constant_p (dest))
+ mips_record_label (dest);
+}
+
+/* Propagate MIPS16 symbol annotation. */
+
+void
+mips_elf_propagate_symbol_attributes (symbolS *dest, symbolS *src)
+{
+ if (!IS_ELF)
+ return;
+
+ if (ELF_ST_IS_MIPS16 (S_GET_OTHER (src)))
+ S_SET_OTHER (dest, ELF_ST_SET_MIPS16 (S_GET_OTHER (dest)));
+}
+\f
/* Some special processing for a MIPS ELF file. */
void
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.h 2010-12-08 00:26:16.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.h 2010-12-08 00:26:16.000000000 +0000
@@ -124,6 +124,14 @@ extern void mips_frob_file (void);
#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
#define tc_frob_file_after_relocs mips_frob_file_after_relocs
extern void mips_frob_file_after_relocs (void);
+
+#define TC_COPY_SYMBOL_ATTRIBUTES(dest, src) \
+ mips_elf_copy_symbol_attributes (dest, src)
+extern void mips_elf_copy_symbol_attributes (symbolS *, symbolS *);
+
+#define TC_PROPAGATE_SYMBOL_ATTRIBUTES(dest, src) \
+ mips_elf_propagate_symbol_attributes (dest, src)
+extern void mips_elf_propagate_symbol_attributes (symbolS *, symbolS *);
#endif
#define tc_fix_adjustable(fixp) mips_fix_adjustable (fixp)
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c 2010-12-08 00:26:16.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/symbols.c 2010-12-08 00:26:16.000000000 +0000
@@ -1184,12 +1184,19 @@ resolve_symbol_value (symbolS *symp)
break;
}
- if (finalize_syms && final_val == 0)
+ if (finalize_syms)
{
- if (LOCAL_SYMBOL_CHECK (add_symbol))
- add_symbol = local_symbol_convert ((struct local_symbol *)
- add_symbol);
- copy_symbol_attributes (symp, add_symbol);
+ if (final_val == 0)
+ {
+ if (LOCAL_SYMBOL_CHECK (add_symbol))
+ add_symbol = local_symbol_convert ((struct local_symbol *)
+ add_symbol);
+ copy_symbol_attributes (symp, add_symbol);
+ }
+#ifdef TC_PROPAGATE_SYMBOL_ATTRIBUTES
+ else
+ TC_PROPAGATE_SYMBOL_ATTRIBUTES (symp, add_symbol);
+#endif
}
/* If we have equated this symbol to an undefined or common
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d 2010-12-08 00:26:16.000000000 +0000
@@ -0,0 +1,30 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self.
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+ \.\.\.
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s 2010-12-08 00:26:16.000000000 +0000
@@ -0,0 +1,36 @@
+# Source file used to test branches to self.
+ .text
+foo:
+ sw $2, ($3)
+ b .
+
+ sw $2, ($3)
+0:
+ b 0b
+
+ sw $2, ($3)
+bar:
+ b bar
+
+ sw $2, ($3)
+ .set frob, .
+ b frob
+
+ .eqv fnord, .
+ sw $2, ($3)
+ b fnord
+
+ .eqv foobar, fnord + 4
+ .eqv foobaz, foobar - 16
+ sw $2, ($3)
+ b foobaz + 12
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 8
+
+# Move the location counter away from the end of code to avoid the final
+# values of symbols equated to expressions involving the counter interfering
+# with disassembly.
+ .align 4
+ .space 16
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2010-12-08 00:26:16.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2010-12-08 00:26:16.000000000 +0000
@@ -390,7 +390,7 @@ mips_arch_create mips64r2 64 mips64 { mi
{ -mmips:isa64r2 } \
{ mipsisa64r2-*-* mipsisa64r2el-*-* }
mips_arch_create mips16 32 {} {} \
- { -march=mips1 -mips16 } { -mmips:16 }
+ { -march=mips1 -mips16 } {}
mips_arch_create r3000 32 mips1 {} \
{ -march=r3000 -mtune=r3000 } { -mmips:3000 }
mips_arch_create r3900 32 mips1 { gpr_ilocks } \
@@ -466,6 +466,7 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test_arches "branch-misc-2pic-64" [mips_arch_list_matching mips3]
run_dump_test "branch-misc-3"
run_dump_test "branch-swap"
+ run_dump_test_arches "branch-self" [mips_arch_list_all]
run_dump_test "div"
if { !$addr32 } {
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d 2010-12-08 00:26:16.000000000 +0000
@@ -0,0 +1,24 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+ \.\.\.
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/doc/internals.texi
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/doc/internals.texi 2010-12-08 00:26:16.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/doc/internals.texi 2010-12-08 10:03:00.000000000 +0000
@@ -1562,6 +1562,12 @@ If defined, GAS will check this macro be
the DWARF call frame debug information that is emitted. Targets which
implement link time relaxation may need to define this macro and set it to zero
if it is possible to change the size of a function's prologue.
+
+@item TC_PROPAGATE_SYMBOL_ATTRIBUTES
+@cindex TC_PROPAGATE_SYMBOL_ATTRIBUTES
+You should define this macro to copy object format specific information from
+one symbol to another. GAS will call it when one symbol is set to a sum of
+another and a constant offset.
@end table
@node Object format backend
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.0/4 v3] MIPS/GAS: Propagate symbol attributes
2010-12-08 19:24 ` [PATCH 4.0/4 v3] " Maciej W. Rozycki
@ 2010-12-09 17:25 ` Richard Sandiford
2010-12-09 18:02 ` Maciej W. Rozycki
0 siblings, 1 reply; 7+ messages in thread
From: Richard Sandiford @ 2010-12-09 17:25 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Catherine Moore, binutils
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> Sorry for not really getting to this patch when you posted the original
>> series. Do you actually have a "real world" use case for this though?
>> Why wouldn't you just put "fnord:" in the appropriate place?
>
> Please note this issue only affects disassembly -- I have not observed
> any other unexpected symptoms. In particular all the relocated entities
> referring to the symbols concerned get the ISA mode set regardless of this
> change.
>
> So it's not about whether we should treat these symbols as MIPS16 code
> references or not, but to match the reality and not mislead the user into
> thinking standard MIPS code is being concerned.
You haven't answered my question, or at least not in a way that
makes me understand it. The testcase in your original message
was a very artificial one, artificial enough that the correct
disassembly is open to debate. Do you have a real-world example
of people writing code like this?
Richard
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4.0/4 v3] MIPS/GAS: Propagate symbol attributes
2010-12-09 17:25 ` Richard Sandiford
@ 2010-12-09 18:02 ` Maciej W. Rozycki
2010-12-11 0:09 ` [PATCH 4.0/4 v4] MIPS/GAS/testsuite: Branch to self/label tests Maciej W. Rozycki
0 siblings, 1 reply; 7+ messages in thread
From: Maciej W. Rozycki @ 2010-12-09 18:02 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
On Thu, 9 Dec 2010, Richard Sandiford wrote:
> > So it's not about whether we should treat these symbols as MIPS16 code
> > references or not, but to match the reality and not mislead the user into
> > thinking standard MIPS code is being concerned.
>
> You haven't answered my question, or at least not in a way that
> makes me understand it. The testcase in your original message
> was a very artificial one, artificial enough that the correct
> disassembly is open to debate. Do you have a real-world example
> of people writing code like this?
OK, fair enough -- I did all kinds of weird assembly programming stuff in
my life (mainly back in my MS-DOS days in early 1990s), so this kind of
coding is nothing unusual to me, but no, I don't have a current real-world
example that would absolutely require this kind of an arrangement to work.
And I can understand your reluctance to make changes to generic parts of
GAS for the lone purpose of getting things 100% right where that would
hardly ever matter for anyone.
Given code actually produced is already correct, I insist on including
the test case itself though. I'll see if I can make the disassembly right
by interspersing the instructions with some otherwise unused labels.
Would it be a solution that would satisfy you?
Otherwise chances are the microMIPS change by its nature will fix the
problem automatically -- I'll check that too before fiddling with the test
case itself. The thing is for the purpose of correct microMIPS
disassembly Chao-ying was kind enough to provide a piece of code to scan
the symbol table and see if a location is within the span of any function
symbol with the microMIPS annotation present and switch to the microMIPS
mode if so. In the course of the recent rewrite I extended that approach
to MIPS16 symbols as well. I hope you agree that is reasonable too.
Maciej
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4.0/4 v4] MIPS/GAS/testsuite: Branch to self/label tests
2010-12-09 18:02 ` Maciej W. Rozycki
@ 2010-12-11 0:09 ` Maciej W. Rozycki
0 siblings, 0 replies; 7+ messages in thread
From: Maciej W. Rozycki @ 2010-12-11 0:09 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
On Thu, 9 Dec 2010, Maciej W. Rozycki wrote:
> > You haven't answered my question, or at least not in a way that
> > makes me understand it. The testcase in your original message
> > was a very artificial one, artificial enough that the correct
> > disassembly is open to debate. Do you have a real-world example
> > of people writing code like this?
>
> OK, fair enough -- I did all kinds of weird assembly programming stuff in
> my life (mainly back in my MS-DOS days in early 1990s), so this kind of
> coding is nothing unusual to me, but no, I don't have a current real-world
> example that would absolutely require this kind of an arrangement to work.
> And I can understand your reluctance to make changes to generic parts of
> GAS for the lone purpose of getting things 100% right where that would
> hardly ever matter for anyone.
>
> Given code actually produced is already correct, I insist on including
> the test case itself though. I'll see if I can make the disassembly right
> by interspersing the instructions with some otherwise unused labels.
> Would it be a solution that would satisfy you?
>
> Otherwise chances are the microMIPS change by its nature will fix the
> problem automatically -- I'll check that too before fiddling with the test
> case itself. The thing is for the purpose of correct microMIPS
> disassembly Chao-ying was kind enough to provide a piece of code to scan
> the symbol table and see if a location is within the span of any function
> symbol with the microMIPS annotation present and switch to the microMIPS
> mode if so. In the course of the recent rewrite I extended that approach
> to MIPS16 symbols as well. I hope you agree that is reasonable too.
The new code in _print_insn_mips() only checks symbols starting at the
closest address lower than or equal to one an instruction being
disassembled is at and does not check the span of any located at lower
addresses. Therefore I have come with the following update to the test
case, adding the "blah" label with the ISA bit set appropriately at the
same place as "frob" so that the ISA bit determines the correct
disassembly mode.
As symbols are sorted alphabetically before _print_insn_mips() is called
this test does not rely on the microMIPS change for correct operation in
the MIPS16 mode and can be applied as it is. With the update included
with the microMIPS change the test won't rely on label names and objdump's
internal sorting rules for correct operation.
Regression tested with mips-sde-elf and mips-gnu-linux, as usually.
2010-12-10 Maciej W. Rozycki <macro@codesourcery.com>
gas/testsuite/
* gas/mips/branch-self.d: New test for various definitions of
labels.
* gas/mips/branch-self.s: Source for the new test.
* gas/mips/mips.exp: Run the new test. Remove -mmips:16 from
"mips16" architecture.
Maciej
binutils-gas-test-branch-self.diff
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.d 2010-12-10 21:56:43.000000000 +0000
@@ -0,0 +1,30 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self.
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+([0-9a-f]+) <[^>]*> ac620000 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 1000ffff b \1 <.*>
+([0-9a-f]+) <[^>]*> 00000000 nop
+ \.\.\.
+ \.\.\.
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/branch-self.s 2010-12-10 21:56:43.000000000 +0000
@@ -0,0 +1,38 @@
+# Source file used to test branches to self.
+ .text
+foo:
+ sw $2, ($3)
+ b .
+
+ sw $2, ($3)
+0:
+ b 0b
+
+ sw $2, ($3)
+bar:
+ b bar
+
+ sw $2, ($3)
+# Put a label here to keep the ISA bit for correct disassembly.
+blah:
+ .set frob, .
+ b frob
+
+ .eqv fnord, .
+ sw $2, ($3)
+ b fnord
+
+ .eqv foobar, fnord + 4
+ .eqv foobaz, foobar - 16
+ sw $2, ($3)
+ b foobaz + 12
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 2
+ .space 8
+
+# Move the location counter away from the end of code to avoid the final
+# values of symbols equated to expressions involving the counter interfering
+# with disassembly.
+ .align 4
+ .space 16
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp 2010-12-10 21:56:42.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp 2010-12-10 21:56:43.000000000 +0000
@@ -390,7 +390,7 @@ mips_arch_create mips64r2 64 mips64 { mi
{ -mmips:isa64r2 } \
{ mipsisa64r2-*-* mipsisa64r2el-*-* }
mips_arch_create mips16 32 {} {} \
- { -march=mips1 -mips16 } { -mmips:16 }
+ { -march=mips1 -mips16 } {}
mips_arch_create r3000 32 mips1 {} \
{ -march=r3000 -mtune=r3000 } { -mmips:3000 }
mips_arch_create r3900 32 mips1 { gpr_ilocks } \
@@ -466,6 +466,7 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test_arches "branch-misc-2pic-64" [mips_arch_list_matching mips3]
run_dump_test "branch-misc-3"
run_dump_test "branch-swap"
+ run_dump_test_arches "branch-self" [mips_arch_list_all]
run_dump_test "div"
if { !$addr32 } {
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips16@branch-self.d 2010-12-10 21:56:43.000000000 +0000
@@ -0,0 +1,24 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS branches to self
+#as: -32
+#source: branch-self.s
+
+# Test various ways to request a branch to self (MIPS16).
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+([0-9a-f]+) <[^>]*> db40 sw v0,0\(v1\)
+([0-9a-f]+) <[^>]*> 17ff b \1 <.*>
+ \.\.\.
+ \.\.\.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-12-11 0:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-26 10:48 [PATCH 4/4] MIPS/GAS: Propagate symbol attributes Maciej W. Rozycki
2010-10-29 14:40 ` [PATCH 4.0/4 v2] " Maciej W. Rozycki
2010-10-30 10:37 ` Richard Sandiford
2010-12-08 19:24 ` [PATCH 4.0/4 v3] " Maciej W. Rozycki
2010-12-09 17:25 ` Richard Sandiford
2010-12-09 18:02 ` Maciej W. Rozycki
2010-12-11 0:09 ` [PATCH 4.0/4 v4] MIPS/GAS/testsuite: Branch to self/label tests Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).