Bug 4516 - [SH] sh-unknown-linux-gnu: big jump table
Summary: [SH] sh-unknown-linux-gnu: big jump table
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 3.0.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: patch, wrong-code
Depends on:
Blocks:
 
Reported: 2001-10-10 03:56 UTC by gniibe
Modified: 2007-11-27 00:22 UTC (History)
5 users (show)

See Also:
Host: sh-unknown-elf
Target: sh-unknown-elf
Build:
Known to work:
Known to fail:
Last reconfirmed: 2006-04-26 21:16:54


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description gniibe 2001-10-10 03:56:30 UTC
"make bootstrap" fails.
The compiler miscomiples expr.c and fold-const.c with -O0.
With -mbigtable it  does not miscompile.
With -ON (N>=1), it does not miscompile.

It does miscompile because there's no validation for value of jump table.
When it's bigger than 32767, at runtime, it goes completly
different place.

Release:
3.0.1 or mainline

Environment:
sh-unknown-linux-gnu
Debain GNU/Linux (unstable)

How-To-Repeat:
"make bootstrap"
Comment 1 gniibe 2001-10-10 03:56:30 UTC
Fix:
I don't have, for now.
Default to -mbigtable?
Comment 2 Craig Rodrigues 2002-02-24 12:08:31 UTC
State-Changed-From-To: open->feedback
State-Changed-Why: Is this solved in the current compiler?
Comment 3 gniibe 2002-02-25 15:39:06 UTC
From: NIIBE Yutaka <gniibe@m17n.org>
To: rodrigc@gcc.gnu.org, gcc-bugs@gcc.gnu.org, gcc-prs@gcc.gnu.org,
   nobody@gcc.gnu.org, gcc-gnats@gcc.gnu.org
Cc:  
Subject: Re: target/4516: [SH] sh-unknown-linux-gnu: big jump table
Date: Mon, 25 Feb 2002 15:39:06 +0900 (JST)

 rodrigc@gcc.gnu.org wrote:
  > Old Synopsis: sh-unknown-linux-gnu: big jump table
  > New Synopsis: [SH] sh-unknown-linux-gnu: big jump table
  > 
  > State-Changed-From-To: open->feedback
  > State-Changed-By: rodrigc
  > State-Changed-When: Sun Feb 24 12:08:31 2002
  > State-Changed-Why:
  >     Is this solved in the current compiler?
 
 I don't think it's fixed.
 
 The issue here is that alignement calculation could be wrong.
 
 I've been using following patch (re-diffed and re-wrote ChangeLog entries),
 with no problem to bootstrap.
 
 2002-02-25  NIIBE Yutaka  <gniibe@m17n.org>
 
 	* final.c (shorten_branches): Update insn_current_address
 	in the for loop.
 	Set insn_current_address to be aligned.
 
 	* doc/invoke.texi (Option Summary: SH Options): Remove -mbigtable.
 
 	* config/sh/sh-protos.h (shl_casesi_worker_length): New function.
 
 	* config/sh/sh.c (shl_casesi_worker_length): New function.
 
 	* config/sh/sh.h (BIGTABLE_BIT, TARGET_BIGTABLE): Removed.
 	(optimized_size): Declared.
 	(TARGET_SWITCHES): Remove -mbigtable option.
 	(CASE_VECTOR_MODE): Always SImode.  Starting from QImode (or
 	HImode) may result assembler error of "pcrel too far" (e.g.
 	compiling f/symbol.c).  It's because barrier_align depends
 	on the mode.
 	(CASE_VECTOR_SHORTEN_MODE): When it's 32768..65536 and -Os,
 	Use HImode with offset_unsigned =1.
 	(ASM_OUTPUT_ADDR_VEC_ELT): Always emit as .long.
 
 	* config/sh/sh.md (*casesi_worker): Handle the case of
 	offset_unsigned is set when HImode.
 	Set length attribute with shl_casesi_worker_length.
 
 --- gcc-HEAD/gcc/final.c	Mon Feb 25 11:26:13 2002
 +++ gcc/gcc/final.c	Mon Feb 25 11:35:43 2002
 @@ -1115,7 +1115,7 @@
    /* Compute initial lengths, addresses, and varying flags for each insn.  */
    for (insn_current_address = FIRST_INSN_ADDRESS, insn = first;
         insn != 0;
 -       insn_current_address += insn_lengths[uid], insn = NEXT_INSN (insn))
 +       insn = NEXT_INSN (insn))
      {
        uid = INSN_UID (insn);
  
 @@ -1129,6 +1129,7 @@
  	      int align = 1 << log;
  	      int new_address = (insn_current_address + align - 1) & -align;
  	      insn_lengths[uid] = new_address - insn_current_address;
 +	      insn_current_address = new_address;
  	    }
  	}
  
 @@ -1208,6 +1209,8 @@
        if (insn_lengths[uid] < 0)
  	fatal_insn ("negative insn length", insn);
  #endif
 +
 +      insn_current_address += insn_lengths[uid];
      }
  
    /* Now loop over all the insns finding varying length insns.  For each,
 --- gcc-HEAD/gcc/doc/invoke.texi	Mon Feb 25 11:27:29 2002
 +++ gcc/gcc/doc/invoke.texi	Mon Feb 25 11:38:00 2002
 @@ -544,7 +544,7 @@
  -m5-32media -m5-32media-nofpu @gol
  -m5-compact -m5-compact-nofpu @gol
  -mb  -ml  -mdalign  -mrelax @gol
 --mbigtable  -mfmovd  -mhitachi  -mnomacsave @gol
 +-mfmovd  -mhitachi  -mnomacsave @gol
  -mieee  -misize  -mpadstruct  -mspace @gol
  -mprefergot  -musermode}
  
 @@ -8545,11 +8545,6 @@
  Shorten some address references at link time, when possible; uses the
  linker option @option{-relax}.
  
 -@item -mbigtable
 -@opindex mbigtable
 -Use 32-bit offsets in @code{switch} tables.  The default is to use
 -16-bit offsets.
 -
  @item -mfmovd
  @opindex mfmovd
  Enable the use of the instruction @code{fmovd}.
 --- gcc-HEAD/gcc/config/sh/sh-protos.h	Mon Feb 25 11:27:19 2002
 +++ gcc/gcc/config/sh/sh-protos.h	Mon Feb 25 11:42:25 2002
 @@ -74,6 +74,7 @@
  extern int shl_sext_length PARAMS ((rtx));
  extern int gen_shl_sext PARAMS ((rtx, rtx, rtx, rtx));
  extern rtx gen_datalabel_ref PARAMS ((rtx));
 +extern int shl_casesi_worker_length PARAMS ((rtx));
  extern int regs_used PARAMS ((rtx, int));
  extern void fixup_addr_diff_vecs PARAMS ((rtx));
  extern int get_dest_uid PARAMS ((rtx, int));
 --- gcc-HEAD/gcc/config/sh/sh.c	Mon Feb 25 11:27:19 2002
 +++ gcc/gcc/config/sh/sh.c	Mon Feb 25 11:46:02 2002
 @@ -2143,6 +2143,48 @@
    return sym;
  }
  
 +
 +/* Function to be used in the length attribute of the casesi_worker
 +   instruction.  Returns number of instructions, which is half of the
 +   length of bytes. */
 +
 +int
 +shl_casesi_worker_length (insn)
 +     rtx insn;
 +{
 +  rtx set_src, label;
 +  rtx diff_vec;
 +
 +  set_src = SET_SRC (XVECEXP (PATTERN (insn), 0, 0));
 +  if (!(GET_CODE (set_src) == UNSPEC
 +	&& XINT (set_src, 1) == UNSPEC_CASESI))
 +    abort ();
 +
 +  label = XVECEXP (set_src, 0, 2);
 +  if (GET_CODE (label) != LABEL_REF)
 +    abort ();
 +
 +  diff_vec = PATTERN (next_real_insn (XEXP (label, 0)));
 +
 +  if (GET_CODE (diff_vec) != ADDR_DIFF_VEC)
 +    abort ();
 +
 +  switch (GET_MODE (diff_vec))
 +    {
 +    case SImode:
 +      return 2;
 +    case HImode:
 +      if (ADDR_DIFF_VEC_FLAGS (diff_vec).offset_unsigned)
 +	return 3;
 +      return 2;
 +    case QImode:
 +      if (ADDR_DIFF_VEC_FLAGS (diff_vec).offset_unsigned)
 +	return 2;
 +      return 1;
 +    default:
 +      abort ();
 +    }
 +}
  
  /* The SH cannot load a large constant into a register, constants have to
     come from a pc relative load.  The reference of a pc relative load
 --- gcc-HEAD/gcc/config/sh/sh.h	Mon Feb 25 11:27:19 2002
 +++ gcc/gcc/config/sh/sh.h	Mon Feb 25 11:56:10 2002
 @@ -234,9 +238,6 @@
  /* Nonzero if we should generate smaller code rather than faster code.  */
  #define TARGET_SMALLCODE   (target_flags & SPACE_BIT)
  
 -/* Nonzero to use long jump tables.  */
 -#define TARGET_BIGTABLE     (target_flags & BIGTABLE_BIT)
 -
  /* Nonzero to generate pseudo-ops needed by the assembler and linker
     to do function call relaxing.  */
  #define TARGET_RELAX (target_flags & RELAX_BIT)
 @@ -297,7 +298,6 @@
    {"5-compact-nofpu", TARGET_NONE, "" },	\
    {"5-compact-nofpu", SH5_BIT|SH3_BIT|SH2_BIT|SH1_BIT, "Generate FPU-less SHcompact code" }, \
    {"b",		-LITTLE_ENDIAN_BIT, "" },  	\
 -  {"bigtable", 	BIGTABLE_BIT, "" },		\
    {"dalign",  	DALIGN_BIT, "" },		\
    {"fmovd",  	FMOVD_BIT, "" },		\
    {"hitachi",	HITACHI_BIT, "" },		\
 @@ -2493,16 +2494,22 @@
      goto LABEL;								\
  }
  
 +extern int optimize; /* needed for gen_casesi.  */
 +extern int optimize_size;
 +
  /* Specify the machine mode that this machine uses
     for the index in the tablejump instruction.  */
 -#define CASE_VECTOR_MODE (TARGET_BIGTABLE ? SImode : HImode)
 +#define CASE_VECTOR_MODE SImode
  
  #define CASE_VECTOR_SHORTEN_MODE(MIN_OFFSET, MAX_OFFSET, BODY) \
  ((MIN_OFFSET) >= 0 && (MAX_OFFSET) <= 127 \
   ? (ADDR_DIFF_VEC_FLAGS (BODY).offset_unsigned = 0, QImode) \
   : (MIN_OFFSET) >= 0 && (MAX_OFFSET) <= 255 \
   ? (ADDR_DIFF_VEC_FLAGS (BODY).offset_unsigned = 1, QImode) \
 - : (MIN_OFFSET) >= -32768 && (MAX_OFFSET) <= 32767 ? HImode \
 + : (MIN_OFFSET) >= -32768 && (MAX_OFFSET) <= 32767 \
 + ? (ADDR_DIFF_VEC_FLAGS (BODY).offset_unsigned = 0, HImode) \
 + : optimize_size && (MIN_OFFSET) >= 0 && (MAX_OFFSET) <= 65535 \
 + ? (ADDR_DIFF_VEC_FLAGS (BODY).offset_unsigned = 1, HImode) \
   : SImode)
  
  /* Define as C expression which evaluates to nonzero if the tablejump
 @@ -3038,10 +3045,7 @@
  /* Output an absolute table element.  */
  
  #define ASM_OUTPUT_ADDR_VEC_ELT(STREAM,VALUE)  				\
 -  if (TARGET_BIGTABLE) 							\
 -    asm_fprintf ((STREAM), "\t.long\t%LL%d\n", (VALUE)); 			\
 -  else									\
 -    asm_fprintf ((STREAM), "\t.word\t%LL%d\n", (VALUE)); 			\
 +    asm_fprintf ((STREAM), "\t.long\t%LL%d\n", (VALUE))
  
  /* Output various types of constants.  */
  
 @@ -3166,8 +3170,6 @@
  
  #define sh_cpu_attr ((enum attr_cpu)sh_cpu)
  extern enum processor_type sh_cpu;
 -
 -extern int optimize; /* needed for gen_casesi.  */
  
  enum mdep_reorg_phase_e
  {
 --- gcc-HEAD/gcc/config/sh/sh.md	Mon Feb 25 11:27:19 2002
 +++ gcc/gcc/config/sh/sh.md	Mon Feb 25 11:49:04 2002
 @@ -6446,6 +6446,8 @@
      case SImode:
        return \"shll2	%1\;mov.l	@(r0,%1),%0\";
      case HImode:
 +      if (ADDR_DIFF_VEC_FLAGS (diff_vec).offset_unsigned)
 +	return \"add	%1,%1\;mov.w	@(r0,%1),%0\;extu.w	%0,%0\";
        return \"add	%1,%1\;mov.w	@(r0,%1),%0\";
      case QImode:
        if (ADDR_DIFF_VEC_FLAGS (diff_vec).offset_unsigned)
 @@ -6455,7 +6457,15 @@
        abort ();
      }
  }"
 -  [(set_attr "length" "4")])
 +  [(set (attr "length")
 +	(cond [(eq (symbol_ref "shl_casesi_worker_length (insn)") (const_int 1))
 +	       (const_string "2")
 +	       (eq (symbol_ref "shl_casesi_worker_length (insn)") (const_int 2))
 +	       (const_string "4")
 +	       ;; Put "match_dup" here so that insn_variable_length_p return 1.
 +	       (ne (match_dup 2) (match_dup 2))
 +	       (const_string "4")]
 +	      (const_string "6")))])
  
  (define_insn "casesi_shift_media"
    [(set (match_operand 0 "arith_reg_operand" "=r")
 -- 
Comment 4 Wolfgang Bangerth 2002-11-20 19:03:54 UTC
State-Changed-From-To: feedback->analyzed
State-Changed-Why: Feedback had been provided. What is the present status of this?
Comment 5 Dara Hazeghi 2003-05-31 21:35:31 UTC
Hello,

can anyone comment on the status of this bug? Was the patch included every installed? Does this 
miscompilation still occur? Thanks,

Dara
Comment 6 Andrew Pinski 2003-06-26 21:53:23 UTC
The patch loos like it was never commited but it was submitted for approval: <http://
gcc.gnu.org/ml/gcc-patches/2002-02/msg01735.html> and some comments about it 
here: <http://gcc.gnu.org/ml/gcc-patches/2002-02/msg01777.html> but nothing came of it.
Comment 7 Andrew Pinski 2003-06-30 15:28:50 UTC
Should no longer be in waiting because I looked into the mailing lists and saw the patch 
in this PR was never committed.
Comment 8 Steven Bosscher 2006-02-16 20:04:19 UTC
One and a half year without any activity -- what's up with this bug?
Comment 9 Andrew Pinski 2006-04-26 21:16:54 UTC
This is still a bug, the patch has not been approved or rejected.
Comment 10 Steven Bosscher 2007-11-26 13:53:05 UTC
Again 1.5 years and zero progress.  Kaz?
Comment 11 Kazumoto Kojima 2007-11-27 00:22:02 UTC
I thought that the problem was fixed with the patches revision
61075 and 61803, though I missed the track of this PR.  I'd like
to close this as fixed. Sorry for forgetting it for a long time.