This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] FIx mips PR 13509: not all labels are moved
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Andrew Pinski <andrew dot pinski at caviumnetworks dot com>
- Cc: binutils <binutils at sourceware dot org>
- Date: Sun, 08 Jan 2012 12:12:17 +0000
- Subject: Re: [PATCH] FIx mips PR 13509: not all labels are moved
- References: <CA+=Sn1kjCoGRwBLMfXn7y-RopBxpcaPue0Q0WUQED4S9ZuFhvQ@mail.gmail.com>
Andrew Pinski <andrew.pinski@caviumnetworks.com> writes:
> Hi the comment before mips_align in gas/config/tc-mips.c says any
> preceding label before an align will be moved. But currently it only
> moves the first preceding label. This can cause confusing behavior
> when you add an extra label or two and those are not moved. This also
> can cause different produced object code when gcc generates -g vs -g0
> code because of the placement of the labels. It had caused a gcc
> bootstrap miscompare with a modified gcc.
Well, the comment says "any preceding label" singular, rather than
"all preceding labels" plural, so the current implementation does
seem to match the comment. I suppose the question then is whether
it's worth changing the behaviour despite the potential lack of
backward compatibility. Since other bits of label handling
(like MIPS16ness) have long been applied to all preceding labels,
and since the current behaviour is causing a GCC miscompare,
I agree it's a good change to make.
The new loop is basically mips_move_labels without the text adjustment,
so I wanted to factor it out into a common routine. (I'm not sure that
the current code gets the text vs. data distinction right in all cases,
but that's another story.)
Since this is a deliberate change in behaviour, I also wanted a testcase
to show why.
Tested on mips64-linux-gnu and mipsisa32el-linux-gnu. Applied.
Richard
gas/
2011-01-08 Andrew Pinski <andrew.pinski@caviumnetworks.com>
Richard Sandiford <rdsandiford@googlemail.com>
* config/tc-mips.c (mips_move_labels): Take the list of labels and
textness as parameters.
(mips_move_text_labels): New function.
(append_insn): Use it instead of mips_move_labels.
(mips_emit_delays, start_noreorder): Likewise.
(mips_align): Take the labels rather than just one label.
Move all labels to after the .align.
(s_align): Change the last argument to mips_align.
(s_cons): Likewise.
(s_float_cons): Likewise.
(s_gpword): Likewise.
(s_gpdword): Likewise.
gas/testsuite/
* gas/mips/align3.s, gas/mips/align3.d: New testcase.
* gas/mips/mips.exp: Run it.
Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c 2012-01-08 11:21:52.000000000 +0000
+++ gas/config/tc-mips.c 2012-01-08 11:27:57.000000000 +0000
@@ -2761,27 +2761,36 @@ reg_needs_delay (unsigned int reg)
return 0;
}
-/* Move all labels in insn_labels to the current insertion point. */
+/* Move all labels in LABELS to the current insertion point. TEXT_P
+ says whether the labels refer to text or data. */
static void
-mips_move_labels (void)
+mips_move_labels (struct insn_label_list *labels, bfd_boolean text_p)
{
- segment_info_type *si = seg_info (now_seg);
struct insn_label_list *l;
valueT val;
- for (l = si->label_list; l != NULL; l = l->next)
+ for (l = labels; l != NULL; l = l->next)
{
gas_assert (S_GET_SEGMENT (l->label) == now_seg);
symbol_set_frag (l->label, frag_now);
val = (valueT) frag_now_fix ();
/* MIPS16/microMIPS text labels are stored as odd. */
- if (HAVE_CODE_COMPRESSION)
+ if (text_p && HAVE_CODE_COMPRESSION)
++val;
S_SET_VALUE (l->label, val);
}
}
+/* Move all labels in insn_labels to the current insertion point
+ and treat them as text labels. */
+
+static void
+mips_move_text_labels (void)
+{
+ mips_move_labels (seg_info (now_seg)->label_list, TRUE);
+}
+
static bfd_boolean
s_is_linkonce (symbolS *sym, segT from_seg)
{
@@ -4149,7 +4158,7 @@ append_insn (struct mips_cl_insn *ip, ex
frag_grow (40);
}
- mips_move_labels ();
+ mips_move_text_labels ();
#ifndef NO_ECOFF_DEBUGGING
if (ECOFF_DEBUGGING)
@@ -4541,7 +4550,7 @@ mips_emit_delays (void)
{
while (nops-- > 0)
add_fixed_insn (NOP_INSN);
- mips_move_labels ();
+ mips_move_text_labels ();
}
}
mips_no_prev_insn ();
@@ -4585,7 +4594,7 @@ start_noreorder (void)
decrease the size of prev_nop_frag. */
frag_wane (frag_now);
frag_new (0);
- mips_move_labels ();
+ mips_move_text_labels ();
}
mips_mark_labels ();
mips_clear_insn_labels ();
@@ -15649,11 +15658,18 @@ get_symbol (void)
fill byte should be used, FILL points to an integer that contains
that byte, otherwise FILL is null.
- The MIPS assembler also automatically adjusts any preceding
- label. */
+ This function used to have the comment:
+
+ The MIPS assembler also automatically adjusts any preceding label.
+
+ The implementation therefore applied the adjustment to a maximum of
+ one label. However, other label adjustments are applied to batches
+ of labels, and adjusting just one caused problems when new labels
+ were added for the sake of debugging or unwind information.
+ We therefore adjust all preceding labels (given as LABELS) instead. */
static void
-mips_align (int to, int *fill, symbolS *label)
+mips_align (int to, int *fill, struct insn_label_list *labels)
{
mips_emit_delays ();
mips_record_compressed_mode ();
@@ -15662,12 +15678,7 @@ mips_align (int to, int *fill, symbolS *
else
frag_align (to, fill ? *fill : 0, 0);
record_alignment (now_seg, to);
- if (label != NULL)
- {
- gas_assert (S_GET_SEGMENT (label) == now_seg);
- symbol_set_frag (label, frag_now);
- S_SET_VALUE (label, (valueT) frag_now_fix ());
- }
+ mips_move_labels (labels, FALSE);
}
/* Align to a given power of two. .align 0 turns off the automatic
@@ -15709,7 +15720,7 @@ s_align (int x ATTRIBUTE_UNUSED)
struct insn_label_list *l = si->label_list;
/* Auto alignment should be switched on by next section change. */
auto_align = 1;
- mips_align (temp, fill_ptr, l != NULL ? l->label : NULL);
+ mips_align (temp, fill_ptr, l);
}
else
{
@@ -15879,12 +15890,10 @@ s_cons (int log_size)
{
segment_info_type *si = seg_info (now_seg);
struct insn_label_list *l = si->label_list;
- symbolS *label;
- label = l != NULL ? l->label : NULL;
mips_emit_delays ();
if (log_size > 0 && auto_align)
- mips_align (log_size, 0, label);
+ mips_align (log_size, 0, l);
cons (1 << log_size);
mips_clear_insn_labels ();
}
@@ -15894,18 +15903,15 @@ s_float_cons (int type)
{
segment_info_type *si = seg_info (now_seg);
struct insn_label_list *l = si->label_list;
- symbolS *label;
-
- label = l != NULL ? l->label : NULL;
mips_emit_delays ();
if (auto_align)
{
if (type == 'd')
- mips_align (3, 0, label);
+ mips_align (3, 0, l);
else
- mips_align (2, 0, label);
+ mips_align (2, 0, l);
}
float_cons (type);
@@ -16686,7 +16692,6 @@ s_gpword (int ignore ATTRIBUTE_UNUSED)
{
segment_info_type *si;
struct insn_label_list *l;
- symbolS *label;
expressionS ex;
char *p;
@@ -16699,10 +16704,9 @@ s_gpword (int ignore ATTRIBUTE_UNUSED)
si = seg_info (now_seg);
l = si->label_list;
- label = l != NULL ? l->label : NULL;
mips_emit_delays ();
if (auto_align)
- mips_align (2, 0, label);
+ mips_align (2, 0, l);
expression (&ex);
mips_clear_insn_labels ();
@@ -16726,7 +16730,6 @@ s_gpdword (int ignore ATTRIBUTE_UNUSED)
{
segment_info_type *si;
struct insn_label_list *l;
- symbolS *label;
expressionS ex;
char *p;
@@ -16739,10 +16742,9 @@ s_gpdword (int ignore ATTRIBUTE_UNUSED)
si = seg_info (now_seg);
l = si->label_list;
- label = l != NULL ? l->label : NULL;
mips_emit_delays ();
if (auto_align)
- mips_align (3, 0, label);
+ mips_align (3, 0, l);
expression (&ex);
mips_clear_insn_labels ();
Index: gas/testsuite/gas/mips/align3.d
===================================================================
--- /dev/null 2012-01-08 08:41:25.262776686 +0000
+++ gas/testsuite/gas/mips/align3.d 2012-01-08 11:32:42.000000000 +0000
@@ -0,0 +1,6 @@
+# objdump: -sj.data
+
+.*
+
+Contents of section \.data:
+ 0000 00000000 (00000004|04000000) (00000004|04000000) 00000000 ................
Index: gas/testsuite/gas/mips/align3.s
===================================================================
--- /dev/null 2012-01-08 08:41:25.262776686 +0000
+++ gas/testsuite/gas/mips/align3.s 2012-01-08 11:31:29.000000000 +0000
@@ -0,0 +1,9 @@
+ .data
+start:
+ .byte 0
+foo:
+bar:
+ .align 2
+ .word foo - start
+ .word bar - start
+ .word 0
Index: gas/testsuite/gas/mips/mips.exp
===================================================================
--- gas/testsuite/gas/mips/mips.exp 2012-01-08 11:32:27.000000000 +0000
+++ gas/testsuite/gas/mips/mips.exp 2012-01-08 11:32:32.000000000 +0000
@@ -1049,6 +1049,7 @@ if { [istarget mips*-*-vxworks*] } {
run_dump_test "align"
run_dump_test "align2"
run_dump_test "align2-el"
+ run_dump_test "align3"
run_dump_test "odd-float"
run_list_test_arches "mips-macro-ill-sfp" "-32 -msingle-float" \