PATCH: Do not make -falign-labels align jump tables
Mark Mitchell
mitchell@codesourcery.com
Mon Mar 2 17:54:00 GMT 2009
On ARM, the casesi pattern for switch tables does a PC-relative load
to get at the jump table. But, there's a label before the jump table,
and -falign-labels will insert alignment padding -- which means that
the PC-relative load gets garbage data.
This patch fixes the problem by making shorten_branches not align such
labels. (There's also some tidying by using JUMP_TABLE_DATA_P instead
of expanding it inline.) Even if this worked, it doesn't seem
obviously desirable to me; -falign-labels is documented as aligning
*branch targets*, and a jump table is certainly not a branch target.
I've tested this on a 4.3-based toolchain on arm-none-eabi. I'll
repeat on mainline, but wanted to see if anyone comments on the basic
idea.
OK for 4.5, assuming testing succeeds?
(I don't have evidence that this is a regression, so cannot justify
for 4.4.)
--
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713
2009-03-01 Mark Mitchell <mark@codesourcery.com>
* final.c (shorten_branches): Do not align labels for jump tables.
(final_scan_insn): Use JUMP_TABLE_DATA_P.
Index: gcc/final.c
===================================================================
--- gcc/final.c (revision 238065)
+++ gcc/final.c (working copy)
@@ -893,6 +893,7 @@ shorten_branches (rtx first ATTRIBUTE_UN
if (LABEL_P (insn))
{
rtx next;
+ bool next_is_jumptable;
/* Merge in alignments computed by compute_alignments. */
log = LABEL_TO_ALIGNMENT (insn);
@@ -902,31 +903,30 @@ shorten_branches (rtx first ATTRIBUTE_UN
max_skip = LABEL_TO_MAX_SKIP (insn);
}
- log = LABEL_ALIGN (insn);
- if (max_log < log)
+ next = next_nonnote_insn (insn);
+ next_is_jumptable = next && JUMP_TABLE_DATA_P (next);
+ if (!next_is_jumptable)
{
- max_log = log;
- max_skip = LABEL_ALIGN_MAX_SKIP;
+ log = LABEL_ALIGN (insn);
+ if (max_log < log)
+ {
+ max_log = log;
+ max_skip = LABEL_ALIGN_MAX_SKIP;
+ }
}
- next = next_nonnote_insn (insn);
/* ADDR_VECs only take room if read-only data goes into the text
section. */
- if (JUMP_TABLES_IN_TEXT_SECTION
- || readonly_data_section == text_section)
- if (next && JUMP_P (next))
- {
- rtx nextbody = PATTERN (next);
- if (GET_CODE (nextbody) == ADDR_VEC
- || GET_CODE (nextbody) == ADDR_DIFF_VEC)
- {
- log = ADDR_VEC_ALIGN (next);
- if (max_log < log)
- {
- max_log = log;
- max_skip = LABEL_ALIGN_MAX_SKIP;
- }
- }
- }
+ if ((JUMP_TABLES_IN_TEXT_SECTION
+ || readonly_data_section == text_section)
+ && next_is_jumptable)
+ {
+ log = ADDR_VEC_ALIGN (next);
+ if (max_log < log)
+ {
+ max_log = log;
+ max_skip = LABEL_ALIGN_MAX_SKIP;
+ }
+ }
LABEL_TO_ALIGNMENT (insn) = max_log;
LABEL_TO_MAX_SKIP (insn) = max_skip;
max_log = 0;
@@ -2006,48 +2006,41 @@ final_scan_insn (rtx insn, FILE *file, i
}
next = next_nonnote_insn (insn);
- if (next != 0 && JUMP_P (next))
+ /* If this label is followed by a jump-table, make sure we put
+ the label in the read-only section. Also possibly write the
+ label and jump table together. */
+ if (next != 0 && JUMP_TABLE_DATA_P (next))
{
- rtx nextbody = PATTERN (next);
-
- /* If this label is followed by a jump-table,
- make sure we put the label in the read-only section. Also
- possibly write the label and jump table together. */
-
- if (GET_CODE (nextbody) == ADDR_VEC
- || GET_CODE (nextbody) == ADDR_DIFF_VEC)
- {
#if defined(ASM_OUTPUT_ADDR_VEC) || defined(ASM_OUTPUT_ADDR_DIFF_VEC)
- /* In this case, the case vector is being moved by the
- target, so don't output the label at all. Leave that
- to the back end macros. */
+ /* In this case, the case vector is being moved by the
+ target, so don't output the label at all. Leave that
+ to the back end macros. */
#else
- if (! JUMP_TABLES_IN_TEXT_SECTION)
- {
- int log_align;
+ if (! JUMP_TABLES_IN_TEXT_SECTION)
+ {
+ int log_align;
- switch_to_section (targetm.asm_out.function_rodata_section
- (current_function_decl));
+ switch_to_section (targetm.asm_out.function_rodata_section
+ (current_function_decl));
#ifdef ADDR_VEC_ALIGN
- log_align = ADDR_VEC_ALIGN (next);
+ log_align = ADDR_VEC_ALIGN (next);
#else
- log_align = exact_log2 (BIGGEST_ALIGNMENT / BITS_PER_UNIT);
+ log_align = exact_log2 (BIGGEST_ALIGNMENT / BITS_PER_UNIT);
#endif
- ASM_OUTPUT_ALIGN (file, log_align);
- }
- else
- switch_to_section (current_function_section ());
+ ASM_OUTPUT_ALIGN (file, log_align);
+ }
+ else
+ switch_to_section (current_function_section ());
#ifdef ASM_OUTPUT_CASE_LABEL
- ASM_OUTPUT_CASE_LABEL (file, "L", CODE_LABEL_NUMBER (insn),
- next);
+ ASM_OUTPUT_CASE_LABEL (file, "L", CODE_LABEL_NUMBER (insn),
+ next);
#else
- targetm.asm_out.internal_label (file, "L", CODE_LABEL_NUMBER (insn));
+ targetm.asm_out.internal_label (file, "L", CODE_LABEL_NUMBER (insn));
#endif
#endif
- break;
- }
+ break;
}
if (LABEL_ALT_ENTRY_P (insn))
output_alternate_entry_point (file, insn);
Index: gcc/testsuite/gcc.dg/falign-labels-1.c
===================================================================
--- gcc/testsuite/gcc.dg/falign-labels-1.c (revision 0)
+++ gcc/testsuite/gcc.dg/falign-labels-1.c (revision 0)
@@ -0,0 +1,38 @@
+/* { dg-do run } */
+/* { dg-options "-falign-labels=8" } */
+
+/* On ARMv7-A CPUs, this test resulted in incorrect code generation.
+ The code generated for the switch statement expected the jump table
+ to immediately follow the jump instruction, but -falign-labels
+ caused the label preceding the table to be aligned. */
+
+volatile int x;
+
+int main(void)
+{
+ int y;
+
+ x = 0;
+
+ switch(x)
+ {
+ case 0:
+ y = 2 * x;
+ break;
+ case 1:
+ y = -3 * x;
+ break;
+ case 2:
+ y = x + 5;
+ break;
+ case 3:
+ y = x - 7;
+ break;
+ default:
+ break;
+ }
+
+ x = y;
+
+ return 0;
+}
More information about the Gcc-patches
mailing list