Bug 21848 - load_mems / replace_loop_mems bug causes miscompilation of jcf-io.c / SEGV while processing java/lang/AbstractMethodError
Summary: load_mems / replace_loop_mems bug causes miscompilation of jcf-io.c / SEGV wh...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.1.0
: P1 normal
Target Milestone: 4.1.0
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: build, patch, wrong-code
Depends on:
Blocks: 22366
  Show dependency treegraph
 
Reported: 2005-05-31 20:09 UTC by Jorn Wolfgang Rennecke
Modified: 2005-07-22 17:22 UTC (History)
1 user (show)

See Also:
Host: i686-pc-linux-gnu
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-06-25 13:04:49


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jorn Wolfgang Rennecke 2005-05-31 20:09:03 UTC
gcc fails to bootstrap on i686-pc-linux-gnu:
./../.././gcc/gcjh -classpath '' -bootclasspath . java/lang/AbstractMethodError
make[2]: *** [java/lang/AbstractMethodError.h] Segmentation fault
make[2]: *** Deleting file `'
make[2]: Leaving directory
`/mnt/scratch/nightly/2005-05-31/i686/i686-pc-linux-gnu/libjava'
make[1]: *** [all-target-libjava] Error 2

gcc/java/jcf-io.c:format_uint is miscompiled.
This is the function:

extern void format_uint (char *, unsigned long long, int);

void
format_uint (char *buffer, unsigned long long value, int base)
{

  char buf[(4 + sizeof(unsigned long long) * 8)];
  char *buf_ptr = buf+(4 + sizeof(unsigned long long) * 8);
  int chars_written;
  int i;



  do {
    int digit = value % base;
    static const char digit_chars[] = "0123456789abcdefghijklmnopqrstuvwxyz";
    *--buf_ptr = digit_chars[digit];
    value /= base;
  } while (value != 0);

  chars_written = buf+(4 + sizeof(unsigned long long) * 8) - buf_ptr;
  for (i = 0; i < chars_written; i++)
    buffer[i] = *buf_ptr++;
  buffer[i] = 0;
}

compiled with:

stage2/cc1 -fpreprocessed jcf-io.i -quiet -dumpbase jcf-io.c -march=i686
-auxbase-strip trash -g -O2 -W -Wall -Wwrite-strings -Wstrict-prototypes
-Wmissing-prototypes -pedantic -Wno-long-long -Wno-variadic-macros
-Wold-style-definition -Werror -version -fomit-frame-pointer -fno-common -o jcf-io.s

compilation with the stage1 compiler shows identical miscompilation.
The parameters for umoddi are not written to the stack.

In jcf-io.c.05.gcse, we have:

(insn 22 21 23 1 jcf-io-0.i:17 (set (mem:DI (plus:SI (reg/f:SI 7 sp)
                (const_int 8 [0x8])) [0 S8 A32])
        (reg:DI 62 [ pretmp.7 ])) 58 {*movdi_2} (nil)
    (insn_list:REG_LIBCALL 25 (nil)))

(insn 23 22 24 1 jcf-io-0.i:17 (set (mem:DI (reg/f:SI 7 sp) [0 S8 A32])
        (reg/v:DI 68 [ value ])) 58 {*movdi_2} (nil)
    (nil))

(call_insn/u 24 23 25 1 jcf-io-0.i:17 (set (reg:DI 0 ax)
        (call (mem:QI (symbol_ref:SI ("__umoddi3") [flags 0x41]) [0 S1 A8])
            (const_int 16 [0x10]))) 529 {*call_value_0} (nil)
    (expr_list:REG_EH_REGION (const_int -1 [0xffffffff])
        (nil))
    (nil))

(insn 25 24 27 1 jcf-io-0.i:17 (set (reg:DI 76)
        (reg:DI 0 ax)) 58 {*movdi_2} (nil)
    (insn_list:REG_RETVAL 22 (expr_list:REG_EQUAL (umod:DI (reg/v:DI 68 [ value 
])
                (reg:DI 62 [ pretmp.7 ]))
            (nil))))

But in jcf-io.c.06.loop:
(insn 22 21 23 1 jcf-io-0.i:17 (set (reg/v:DI 87 [ pretmp.7 ])
        (reg:DI 62 [ pretmp.7 ])) -1 (nil)
    (insn_list:REG_LIBCALL 25 (nil)))

(insn 23 22 24 1 jcf-io-0.i:17 (set (reg/v:DI 88 [ value ])
        (reg/v:DI 68 [ value ])) -1 (nil)
    (nil))

(call_insn/u 24 23 25 1 jcf-io-0.i:17 (set (reg:DI 0 ax)
        (call (mem:QI (symbol_ref:SI ("__umoddi3") [flags 0x41]) [0 S1 A8])
            (const_int 16 [0x10]))) -1 (nil)
    (expr_list:REG_EH_REGION (const_int -1 [0xffffffff])
        (nil))
    (nil))

(insn 25 24 27 1 jcf-io-0.i:17 (set (reg:DI 76)
        (reg:DI 0 ax)) -1 (nil)
    (insn_list:REG_RETVAL 22 (expr_list:REG_EQUAL (umod:DI (reg/v:DI 68 [ value 
])
                (reg:DI 62 [ pretmp.7 ]))
            (nil))))

The SET_SRC of insn 22 is changed here:

Hardware watchpoint 6: *$4

Old value = 0xb5927d14
New value = 0xb591da80
validate_change (object=0xb58b7c08, loc=0xb5927838, new=0xb591da80, in_group=1)
    at ../../srcw/gcc/recog.c:203
203       if (num_changes >= changes_allocated)
(gdb) call debug_rtx_find(get_insns(),22)
(insn 22 21 23 jcf-io-0.i:17 (set (reg/v:DI 87)
        (reg:DI 62 [ pretmp.7 ])) -1 (nil)
    (insn_list:REG_LIBCALL 25 (nil)))

$5 = (struct rtx_def *) 0xb58b7c08
(gdb) frame 1
#1  0x08584119 in replace_loop_mem (mem=0xb5927838, data=0xbfffb480)
    at ../../srcw/gcc/loop.c:11374
11374     validate_change (args->insn, mem, args->replacement, 1);
(gdb) bt
#0  validate_change (object=0xb58b7c08, loc=0xb5927838, new=0xb591da80, 
    in_group=1) at ../../srcw/gcc/recog.c:203
#1  0x08584119 in replace_loop_mem (mem=0xb5927838, data=0xbfffb480)
    at ../../srcw/gcc/loop.c:11374
#2  0x0845797e in for_each_rtx_1 (exp=0xb5927834, n=0, 
    f=0x8584097 <replace_loop_mem>, data=0xbfffb480)
    at ../../srcw/gcc/rtlanal.c:2645
#3  0x084579dc in for_each_rtx_1 (exp=0xb58b7c08, n=5, 
    f=0x8584097 <replace_loop_mem>, data=0xbfffb480)
    at ../../srcw/gcc/rtlanal.c:2660
#4  0x08457b4b in for_each_rtx (x=0xbfffb4a0, f=0x8584097 <replace_loop_mem>, 
    data=0xbfffb480) at ../../srcw/gcc/rtlanal.c:2741
#5  0x08584155 in replace_loop_mems (insn=0xb58b7c08, mem=0xb5927d14, 
    reg=0xb591da80, written=1) at ../../srcw/gcc/loop.c:11388
#6  0x08583470 in load_mems (loop=0x9812738) at ../../srcw/gcc/loop.c:10968
#7  0x08572f66 in scan_loop (loop=0x9812738, flags=0)
    at ../../srcw/gcc/loop.c:1543
#8  0x08571321 in loop_optimize (f=0xb58afae0, dumpfile=0x0, flags=0)
    at ../../srcw/gcc/loop.c:907
#9  0x084b9495 in rest_of_handle_loop_optimize ()
    at ../../srcw/gcc/passes.c:1111
#10 0x084ba079 in rest_of_compilation () at ../../srcw/gcc/passes.c:1573
Comment 1 Jorn Wolfgang Rennecke 2005-05-31 20:56:08 UTC
The problem would not arise if the call was neither const nor pure.

cselib_process_insn has this code:

  if (CALL_P (insn))
    {
      for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
        if (call_used_regs[i]
            || (REG_VALUES (i) && REG_VALUES (i)->elt
                && HARD_REGNO_CALL_PART_CLOBBERED (i,
                      GET_MODE (REG_VALUES (i)->elt->u.val_rtx))))
          cselib_invalidate_regno (i, reg_raw_mode[i]);

      if (! CONST_OR_PURE_CALL_P (insn))
        cselib_invalidate_mem (callmem);
    }

So, this explains why ordinary calls work.

There is code in load_mems to take the argument of calls into account, but
it requires CALL_INSN_FUNCTION_USAGE to be set.  So a possible fix would make
sure that CALL_INSN_FUNCTION_USAGE is set correctly, at least for
CONST_OR_PURE_CALL_P functions.
Comment 2 Andrew Pinski 2005-06-19 14:06:39 UTC
This was fixed by reverting the patch which caused this.
Comment 3 Jorn Wolfgang Rennecke 2005-06-20 08:48:34 UTC
(In reply to comment #2)
> This was fixed by reverting the patch which caused this.

Which patch was this?
Comment 4 Andrew Pinski 2005-06-20 12:09:43 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > This was fixed by reverting the patch which caused this.
> 
> Which patch was this?
2005-05-31  Pat Haugen  <pthaugen@us.ibm.com>

	* loop.c (loop_invariant_p, valid_initial_value_p): Revert last
	change.


The last change was:
2005-05-30  Pat Haugen  <pthaugen@us.ibm.com>

	* loop.c (loop_invariant_p, valid_initial_value_p): Use
	regs_invalidated_by_call instead of call_used_regs.
Comment 5 Jorn Wolfgang Rennecke 2005-06-20 13:05:54 UTC
(In reply to comment #4)
> 2005-05-31  Pat Haugen  <pthaugen@us.ibm.com>
> 
> 	* loop.c (loop_invariant_p, valid_initial_value_p): Revert last
> 	change.
> 
> 
> The last change was:
> 2005-05-30  Pat Haugen  <pthaugen@us.ibm.com>
> 
> 	* loop.c (loop_invariant_p, valid_initial_value_p): Use
> 	regs_invalidated_by_call instead of call_used_regs.
> 

The failure that I analyzed involved killing stores to memory, not fixed
registers.  Therefore, the underlying problem has not been fixed, it has
only become latent (again?).  I don't think we should daclare a latent
wrong-code bug as fixed.  We might change the severity amd/or priority,
though.
Comment 6 Jorn Wolfgang Rennecke 2005-06-20 13:21:28 UTC
P.S.: I realize now that the code in gcc/java/jcf-io.c is no longer miscompiled
for i686-pc-linux-gnu because the memory addresses contain stack pointer
references, and the stack pointer is a fixed register, i.e. call used, but not
invalidated by a call.  Hence with Pat Haugen's patch reversed, we falsely
conclude that addresses of the stack locations are not loop invariant.
Two wrongs don't make a right here, though.  The wider context of the
loop_invariant_p code is:

   case REG:
      if ((x == frame_pointer_rtx || x == hard_frame_pointer_rtx
           || x == arg_pointer_rtx || x == pic_offset_table_rtx)
          && ! current_function_has_nonlocal_goto)
        return 1;

      if (LOOP_INFO (loop)->has_call
          && REGNO (x) < FIRST_PSEUDO_REGISTER && call_used_regs[REGNO (x)])
        return 0;


I.e. if the libcall parameter values are addressed using an argument or
frame pointer instead of directly via the stack pointer, invalid hoisting of
the stores can still occur. 
Comment 7 Jorn Wolfgang Rennecke 2005-06-21 05:37:22 UTC
The actual reason for the wrong-code generation has not been fixed.
Moreover, AFAICS, on ACCUMULATE_OUTGOING_ARGS targets, the stack
pointer is loop invariant unless current_function_calls_alloca, and
pretending otherwise is an unnecessary pessimization.
Comment 8 GCC Commits 2005-07-18 14:57:24 UTC
Subject: Bug 21848

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	sh-elf-4_1-branch
Changes by:	amylaar@gcc.gnu.org	2005-07-18 14:57:06

Modified files:
	gcc            : ChangeLog loop.c cse.c calls.c ifcvt.c 

Log message:
	2005-07-15  J"orn Rennecke <joern.rennecke@st.com>
	
	cvs update -j1.531 -j1.530 loop.c
	Re-apply this patch:
	2005-05-30  Pat Haugen  <pthaugen@us.ibm.com>
	* loop.c (loop_invariant_p, valid_initial_value_p): Use
	regs_invalidated_by_call instead of call_used_regs.
	
	2005-07-15  J"orn Rennecke <joern.rennecke@superh.com>
	
	PR rtl-optimization/18992
	http://gcc.gnu.org/ml/gcc-patches/2005-07/msg01140.html
	Back out this patch:
	2003-10-08  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
	PR optimization/12142
	* cse.c (count_reg_usage): In a SET with a REG SET_DEST, count the
	uses of the register in the SET_SRC.  Remove unnecessary argument.
	
	Replace it with this:
	* cse.c (count_reg_usage): In INSN, JUMP_INSN and CALL_INSN cases,
	if flag_non_call_exceptions is set and the insn may trap, pass
	pc_rtx as dest for recursion.
	In SET_SRC part of SET case, if dest is already set, pass it down
	unchanged.
	
	2005-07-15  J"orn Rennecke <joern.rennecke@superh.com>
	
	PR rtl-optimization/21848
	http://gcc.gnu.org/ml/gcc-patches/2005-07/msg01142.html
	* calls.c (emit_library_call_value_1): For const functions, add
	USEs of the stack slots to CALL_INSN_FUNCTION_USAGE.
	
	2005-07-15  J"orn Rennecke <joern.rennecke@superh.com>
	
	PR rtl-optimization/20370
	http://gcc.gnu.org/ml/gcc-patches/2005-07/msg01148.html
	* ifcvt.c (dead_or_predicable): Before calling propagate_block,
	call allocate_reg_info if necessary.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=sh-elf-4_1-branch&r1=2.8142.2.23&r2=2.8142.2.24
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/loop.c.diff?cvsroot=gcc&only_with_tag=sh-elf-4_1-branch&r1=1.525.2.2&r2=1.525.2.3
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cse.c.diff?cvsroot=gcc&only_with_tag=sh-elf-4_1-branch&r1=1.351.2.1&r2=1.351.2.2
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/calls.c.diff?cvsroot=gcc&only_with_tag=sh-elf-4_1-branch&r1=1.385.2.1&r2=1.385.2.2
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ifcvt.c.diff?cvsroot=gcc&only_with_tag=sh-elf-4_1-branch&r1=1.184.2.2&r2=1.184.2.3

Comment 9 GCC Commits 2005-07-22 12:15:54 UTC
Subject: Bug 21848

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	amylaar@gcc.gnu.org	2005-07-22 12:15:40

Modified files:
	gcc            : ChangeLog calls.c 

Log message:
	PR rtl-optimization/21848
	* calls.c (emit_library_call_value_1): For const functions, add
	USEs of the stack slots to CALL_INSN_FUNCTION_USAGE.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9517&r2=2.9518
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/calls.c.diff?cvsroot=gcc&r1=1.394&r2=1.395

Comment 10 Andrew Pinski 2005-07-22 17:22:02 UTC
Fixed.