diff mbox

Fix Thumb-1 jump table alignment length calculation (PR target/43961)

Message ID Pine.LNX.4.64.1301132235520.1412@digraph.polyomino.org.uk
State New
Headers show

Commit Message

Joseph Myers Jan. 13, 2013, 10:37 p.m. UTC 
Bug 43961 (a regression in 4.5 and later) is the ARM back end
generating an out-of-range Thumb-1 branch because the calculation (in
architecture-independent code) of the size of a jump table fails to
allow for alignment.

Jump table alignment should be allowed for through ADDR_VEC_ALIGN, but
the ASM back end has a comment /* Jump table alignment is explicit in
ASM_OUTPUT_CASE_LABEL.  */ and defines ADDR_VEC_ALIGN to 0, so meaning
that calculations relating to branch ranges fail to allow for the
alignment.

Bug 43961 has an old patch attached to define ADDR_VEC_ALIGN properly.
Although that fixes the Thumb-1 issue, it introduces other problems
for Thumb-2 jump tables because of an interaction between
ADDR_VEC_ALIGN and CASE_VECTOR_SHORTEN_MODE.  When
CASE_VECTOR_SHORTEN_MODE is defined, the size of jump table entries,
and so the alignment required for the jump table, can be changed in
shorten_branches, but the alignment of the label before the jump table
has already been computed based on the original mode, and is not then
updated for the changed mode.  This results in alignment being used
before a Thumb-2 tbb jump table, which needs to follow the tbb
instruction immediately.

This patch arranges for shorten_branches to recalculate the alignments
of labels before jump tables if CASE_VECTOR_SHORTEN_MODE.  The
recalculation logic may not match the logic for the original
calculation in all cases - doing so would probably require additional
information to be stored at the time of the original calculation so it
is still available for the recalculation.  But I believe the
recalculated value based on ADDR_VEC_ALIGN should always work - and in
the cases where, as for the Thumb-2 tbb case, overalignment will not
work, only the recalculated value is going to be correct.

(There is no testcase in this patch because this sort of thing is
always extremely sensitive to slight changes in code generation
between compiler versions and tends to require large,
version-dependent testcases.)

Tested with no regressions with cross to arm-none-linux-gnueabi (both
default configuration, testing both -marm and -mthumb, and v7-A, again
testing both -marm and -mthumb).  OK to commit?

2013-01-13  Joseph Myers  <joseph@codesourcery.com>
	    Mikael Pettersson  <mikpe@it.uu.se>

	PR target/43961
	* config/arm/arm.h (ADDR_VEC_ALIGN): Align SImode jump tables for
	Thumb.
	(ASM_OUTPUT_CASE_LABEL): Remove.
	(ASM_OUTPUT_BEFORE_CASE_LABEL): Define to empty.
	* final.c (shorten_branches): Update alignment of labels before
	jump tables if CASE_VECTOR_SHORTEN_MODE.

Comments

Richard Earnshaw Jan. 15, 2013, 10:56 a.m. UTC | #1 
On 13/01/13 22:37, Joseph S. Myers wrote:
> Bug 43961 (a regression in 4.5 and later) is the ARM back end
> generating an out-of-range Thumb-1 branch because the calculation (in
> architecture-independent code) of the size of a jump table fails to
> allow for alignment.
>
> Jump table alignment should be allowed for through ADDR_VEC_ALIGN, but
> the ASM back end has a comment /* Jump table alignment is explicit in
> ASM_OUTPUT_CASE_LABEL.  */ and defines ADDR_VEC_ALIGN to 0, so meaning
> that calculations relating to branch ranges fail to allow for the
> alignment.
>
> Bug 43961 has an old patch attached to define ADDR_VEC_ALIGN properly.
> Although that fixes the Thumb-1 issue, it introduces other problems
> for Thumb-2 jump tables because of an interaction between
> ADDR_VEC_ALIGN and CASE_VECTOR_SHORTEN_MODE.  When
> CASE_VECTOR_SHORTEN_MODE is defined, the size of jump table entries,
> and so the alignment required for the jump table, can be changed in
> shorten_branches, but the alignment of the label before the jump table
> has already been computed based on the original mode, and is not then
> updated for the changed mode.  This results in alignment being used
> before a Thumb-2 tbb jump table, which needs to follow the tbb
> instruction immediately.
>
> This patch arranges for shorten_branches to recalculate the alignments
> of labels before jump tables if CASE_VECTOR_SHORTEN_MODE.  The
> recalculation logic may not match the logic for the original
> calculation in all cases - doing so would probably require additional
> information to be stored at the time of the original calculation so it
> is still available for the recalculation.  But I believe the
> recalculated value based on ADDR_VEC_ALIGN should always work - and in
> the cases where, as for the Thumb-2 tbb case, overalignment will not
> work, only the recalculated value is going to be correct.
>
> (There is no testcase in this patch because this sort of thing is
> always extremely sensitive to slight changes in code generation
> between compiler versions and tends to require large,
> version-dependent testcases.)
>
> Tested with no regressions with cross to arm-none-linux-gnueabi (both
> default configuration, testing both -marm and -mthumb, and v7-A, again
> testing both -marm and -mthumb).  OK to commit?
>
> 2013-01-13  Joseph Myers  <joseph@codesourcery.com>
> 	    Mikael Pettersson  <mikpe@it.uu.se>
>
> 	PR target/43961
> 	* config/arm/arm.h (ADDR_VEC_ALIGN): Align SImode jump tables for
> 	Thumb.
> 	(ASM_OUTPUT_CASE_LABEL): Remove.
> 	(ASM_OUTPUT_BEFORE_CASE_LABEL): Define to empty.
> 	* final.c (shorten_branches): Update alignment of labels before
> 	jump tables if CASE_VECTOR_SHORTEN_MODE.
>

OK.

R.
diff mbox

Patch

Index: gcc/final.c
===================================================================
--- gcc/final.c	(revision 195098)
+++ gcc/final.c	(working copy)
@@ -1182,6 +1182,29 @@  shorten_branches (rtx first)
 	  if (LABEL_P (insn))
 	    {
 	      int log = LABEL_TO_ALIGNMENT (insn);
+
+#ifdef CASE_VECTOR_SHORTEN_MODE
+	      /* If the mode of a following jump table was changed, we
+		 may need to update the alignment of this label.  */
+	      rtx next;
+	      bool next_is_jumptable;
+
+	      next = next_nonnote_insn (insn);
+	      next_is_jumptable = next && JUMP_TABLE_DATA_P (next);
+	      if ((JUMP_TABLES_IN_TEXT_SECTION
+		   || readonly_data_section == text_section)
+		  && next_is_jumptable)
+		{
+		  int newlog = ADDR_VEC_ALIGN (next);
+		  if (newlog != log)
+		    {
+		      log = newlog;
+		      LABEL_TO_ALIGNMENT (insn) = log;
+		      something_changed = 1;
+		    }
+		}
+#endif
+
 	      if (log > insn_current_align)
 		{
 		  int align = 1 << log;
Index: gcc/config/arm/arm.h
===================================================================
--- gcc/config/arm/arm.h	(revision 195098)
+++ gcc/config/arm/arm.h	(working copy)
@@ -2129,20 +2129,13 @@  extern int making_const_table;
 	asm_fprintf (STREAM, "\tpop {%r}\n", REGNO);	\
     } while (0)
 
-/* Jump table alignment is explicit in ASM_OUTPUT_CASE_LABEL.  */
-#define ADDR_VEC_ALIGN(JUMPTABLE) 0
+#define ADDR_VEC_ALIGN(JUMPTABLE)	\
+  ((TARGET_THUMB && GET_MODE (PATTERN (JUMPTABLE)) == SImode) ? 2 : 0)
 
-/* This is how to output a label which precedes a jumptable.  Since
-   Thumb instructions are 2 bytes, we may need explicit alignment here.  */
-#undef  ASM_OUTPUT_CASE_LABEL
-#define ASM_OUTPUT_CASE_LABEL(FILE, PREFIX, NUM, JUMPTABLE)		\
-  do									\
-    {									\
-      if (TARGET_THUMB && GET_MODE (PATTERN (JUMPTABLE)) == SImode)	\
-        ASM_OUTPUT_ALIGN (FILE, 2);					\
-      (*targetm.asm_out.internal_label) (FILE, PREFIX, NUM);		\
-    }									\
-  while (0)
+/* Alignment for case labels comes from ADDR_VEC_ALIGN; avoid the
+   default alignment from elfos.h.  */
+#undef ASM_OUTPUT_BEFORE_CASE_LABEL
+#define ASM_OUTPUT_BEFORE_CASE_LABEL(FILE, PREFIX, NUM, TABLE) /* Empty.  */
 
 /* Make sure subsequent insns are aligned after a TBB.  */
 #define ASM_OUTPUT_CASE_END(FILE, NUM, JUMPTABLE)	\