Fix -- Wide int sign extension in combine.c

Donn Terry donn@interix.com
Fri Feb 19 10:05:00 GMT 1999


Jeffrey A Law wrote:
> 
>   In message < 36C31851.386C696C@interix.com >you write:
>   > Change to gcc.  When HOST_WIDE_INT is 64 bits.
>   > Failure from testcase gcc.c-torture/930506-2.c.
> What target?  I haven't seen this test failing on any of the 64bit MIPS
> targets I work on regularly.

Intel, with HWI set to 64.  You already have the testcase
for this one (930506-2.c).  Specifically, the
INITIALIZE_TRAMPOLINE macro uses negative offsets.

Here's a snippet of the testcase's assembler output in this case:

.globl _f1
_f1:
        pushl %ebp
        movl %esp,%ebp
        subl $32,%esp
        leal 4294967268(%ebp),%eax  <-----  0xffffffe4 (unopt: this is an 'and' involving -29)
        movl $____.7-10,%edx
        movb $185,(%eax)
        movl %ebp,1(%eax)
        movb $233,5(%eax)
        subl %eax,%edx
        movl %edx,6(%eax)
        leal 4294967284(%ebp),%eax  <----- 0xfffffff4
        movl $____.3-10,%edx
        movb $185,(%eax)
        movl %ebp,1(%eax)
        movb $233,5(%eax)

What I was seeing originally was decimal for 0x1ffffffe4, I suspect
I'll have to do a full bootstrap build with the fix turned off
to reproduce that.  However, the above isn't right (but would
work "by accident".)  (Note... other compilers could be generating
this, and since this is a compile-only test, you'd never get an
error report; the only reason I noticed was that my assembler
fell over (weirdly) with the 33 bit value.  In fact, a more
robust assembler might simply ignore the problem even with 33
significant bits of number.)

Setting a breakpoint at the recursive force_to_mode call near line 6427
(while compiling 930506-2.i):

6427                  return force_to_mode (plus_constant (XEXP (x, 0),
(gdb) p debug_rtx(x)
(plus:SI (reg:SI 6 %ebp)
    (const_int -26))
$4 = void

## The value for x above is negative.

(gdb) enable 5  # at force_to_mode
(gdb) c
Continuing.

Breakpoint 5, force_to_mode (x=0x9db220, mode=SImode, mask=4294967292,
    reg=0x0, just_select=0) at combine.c:6217
6217      enum rtx_code code = GET_CODE (x);
(gdb) finish

# just so we can see the result from line 6427

Run till exit from #0  force_to_mode (x=0x9db220, mode=SImode,
    mask=4294967292, reg=0x0, just_select=0) at combine.c:6217
0x4ae2be in force_to_mode (x=0x9c30e0, mode=SImode, mask=4294967292, reg=0x0,
    just_select=0) at combine.c:6427
6427                  return force_to_mode (plus_constant (XEXP (x, 0),
Value returned is $5 = (struct {...} *) 0x9db220

# Now look at the result.
(gdb) p debug_rtx(0x9db220)

(plus:SI (reg:SI 6 %ebp)
    (const_int 4294967268))
$6 = void

# Oops, positive.  It got masked down to 32 bits, and since it
# will be printed in the assembler as a 64 bit decimal, we've
# lost the sign.

As I noted above, there's somewhere else in the compiler that
a bootstrapped build with this fix applied "fixes" such that
subsequent operations don't carry into the 33d bit.

>   > The code just prior to here computes smask, which is the sign-extended
>   > (to HWI bits) version of the mask of 'interesting' bits, if a signed
>   > number is involved.
> Right.
> 
>   > However, in computing the value argument to plus_constant, mask,
>   > not smask, is used.  If XEXP(x,1) is negative, this masks off the
>   > sign extension (from 32 to 64 bits, e.g.).
> True, but what I don't see is why this is important.   If WIDTH is 32bits,
> we don't actually care what's in the high bits.

Ahh... but we do care about sign extension, at least for later printing
in the assembler source.
> 
> Maybe an example would help.  ie, what are the values of the variables in
> question.  Or better yet, a testcase.

The testcase is tree.c (but I gather you'd prefer something
smaller, so...)

unsigned char foo;
int
integer_pow2p ()
{
  int prec;
  long long  high, low;
  prec = foo;
  if (prec == 2 * 64 )
    ;
  else if (prec > 64 )
    high &= ~((long long ) (-1) << (prec - 64 ));
  else
    {
      high = 0;
      if (prec < 64 )
        low &= ~((long long ) (-1) << prec);
    }
  return ((high == 0 && (low & (low - 1)) == 0)
          || (low == 0 && (high & (high - 1)) == 0));
}

I won't claim that this is COMPLETLY minimized, but it does exhibit
the problem.  Note that foo is critically part of the bug...
it seems to revolve around the compiler knowing that prec's
value is limited to the range of an unsigned char.
(Code is extracted from tree.c, so copyright is FSF.)

With combine.c reading as follows (except for the ^ showing the change):

              return force_to_mode (plus_constant (XEXP (x, 0),
                                                   INTVAL (XEXP (x, 1)) & smask),
                                                                          ^
                                    mode, mask, reg, next_select);

This causes an infinite recursion.

So does:

              return force_to_mode (plus_constant (XEXP (x, 0),
                                                   INTVAL (XEXP (x, 1)) & smask),
                                                                          ^
                                    mode, smask, reg, next_select);
                                          ^

> 
> I'm not saying you're wrong, I'm looking for more detailed information.
> 
>   > The prior changes outside the ifdef/endif:
>   > It turns out that just the fix above causes an infinite recursion
>   > in compiling gcc/tree.c (in this case, dealing with QI type).  Using
>   > smask in these locations seems intuitively reasonable, fixes the
>   > recursion, and improves the regression results.  (Note; the situation
>   > is rare; tree.c is the first file compiled during a build that appears
>   > to hit the problem.)
> I'd like to see a testcase for this too.  There are some cases where
> nonzero_bits and friends can recurse infinitly, but they're problems  with
> the state of the various reg_last_set* tables.

It's not nonzero_bits, it's force_to_mode that recursing all by itself.  
Once it gets to this point with the debugger values shown below, it
basically takes the most direct possible path back to this point from
the beginning of the function.  There's a trace also below.

(gdb) p debug_rtx(x)

(plus:SI (reg/v:SI 22)
    (const_int -64))
$6 = void
(gdb) p/x mask
$7 = 0xff
(gdb) p/x smask
$8 = 0xffffffff

A trace:
Starting program: //D/home/donn.intel/egcs.bin/gcc/./cc1 xtree.i -quiet -O2

Breakpoint 4, force_to_mode (x=0x9c5678, mode=QImode, mask=255, reg=0x0,
    just_select=0) at combine.c:6427
6427                  return force_to_mode (plus_constant (XEXP (x, 0),

# A little dance to let us get into the recursive call of force_to_mode
(gdb) s
plus_constant_wide (x=0x9c5070, c=-64) at explow.c:52
52        int all_constant = 0;
(gdb) finish
Run till exit from #0  plus_constant_wide (x=0x9c5070, c=-64) at explow.c:52

# Now we're at the point of call for the recursion (having finished the
# other function in the return statement
0x4b1373 in force_to_mode (x=0x9c5678, mode=QImode, mask=255, reg=0x0,
    just_select=0) at combine.c:6427
6427                  return force_to_mode (plus_constant (XEXP (x, 0),
Value returned is $10 = (struct {...} *) 0x9e0208

# Let's check that result
(gdb) p debug_rtx(0x9e0208)
(plus:SI (reg/v:SI 22)
    (const_int -64))
$13 = void
# Note... sign properly retained.

# step into the actual recursion.
(gdb) s
force_to_mode (x=0x9e0208, mode=QImode, mask=4294967295, reg=0x0,
    just_select=0) at combine.c:6217
6217      enum rtx_code code = GET_CODE (x);
(gdb) n
6218      int next_select = just_select || code == XOR || code == NOT || code ==
 NEG;
(gdb)
6229      if (code == CALL || code == ASM_OPERANDS || code == CLOBBER)
(gdb)
6235      op_mode = ((GET_MODE_CLASS (mode) == GET_MODE_CLASS (GET_MODE (x))
(gdb)
6243      if ((code == LSHIFTRT || code == ASHIFTRT)
(gdb)
6248      if (op_mode)
(gdb)
6249        mask &= GET_MODE_MASK (op_mode);
(gdb)
6254      if (op_mode)
(gdb)
6255        fuller_mask = (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT
(gdb)
6262      nonzero = nonzero_bits (x, mode);
(gdb)
6265      if (! just_select && (nonzero & mask) == 0)
(gdb)
6270      if (GET_CODE (x) == CONST_INT)
(gdb)
6286      if (GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (mode)
(gdb)
6292      if (GET_MODE (x) == mode && code != SUBREG && (~ mask & nonzero) == 0)

(gdb)
6295      switch (code)
(gdb)
6394            int width = GET_MODE_BITSIZE (mode);
(gdb)
6395            unsigned HOST_WIDE_INT smask = mask;
(gdb) 
6400            if (width < HOST_BITS_PER_WIDE_INT
(gdb)
6402              smask |= (HOST_WIDE_INT) -1 << width;
(gdb)
6404            if (GET_CODE (XEXP (x, 1)) == CONST_INT
(gdb)
6425                if ((nonzero_bits (XEXP (x, 0), mode) & ~ mask) == 0
(gdb)

# oops....  And the args look just the same as before, so....
Breakpoint 4, force_to_mode (x=0x9e0208, mode=QImode, mask=255, reg=0x0,
    just_select=0) at combine.c:6427
6427                  return force_to_mode (plus_constant (XEXP (x, 0),

> 
> It sounds like you're trying to address two separate problems; we should
> work on them separately -- ie, submit them as different patches with testcases.

Possibly true, but they should NOT be separate patches, since the effect of fixing
them in the wrong order is to cause the build of the compiler to crash.

> That way if we agree that one of them is correct we can install it immediately
> without having to wait on the other patch if it needs further review.

Risky...  don't want to break the compiler build.


===================================================
Donn Terry                  mailto:donn@interix.com
Softway Systems, Inc.        http://www.interix.com
2850 McClelland Dr, Ste. 1800   Ft.Collins CO 80525
Tel: +1-970-204-9900           Fax: +1-970-204-9951
===================================================


More information about the Gcc-patches mailing list