User account creation filtered due to spam.

Bug 33887 - [4.2 Regression] Reference to bitfield gets wrong value when optimizing
Summary: [4.2 Regression] Reference to bitfield gets wrong value when optimizing
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.2.0
: P1 normal
Target Milestone: 4.3.0
Assignee: Not yet assigned to anyone
URL:
Keywords: patch, wrong-code
: 33205 36122 (view as bug list)
Depends on:
Blocks: 30332
  Show dependency treegraph
 
Reported: 2007-10-24 23:44 UTC by Ian Lance Taylor
Modified: 2009-03-31 00:35 UTC (History)
13 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.0.4 4.3.0
Known to fail: 4.1.0 4.2.5
Last reconfirmed: 2008-01-21 09:20:24


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Lance Taylor 2007-10-24 23:44:24 UTC
This C++ test case passes a bitfield value to a function which expects a const reference.  When optimizing, the function gets the wrong value: it gets the value 0x1000000, which is an acceptable value for a 24-bit bitfield, but is the wrong value for extending the bitfield out to a 32-bit integer.  This failure happens on i686-pc-linux-gnu with -O2.  The test passes without optimization.

extern "C" void abort() __attribute__ ((noreturn));

template<typename t1, typename t2>
void fn(const t1&, const t2&) __attribute__ ((noinline));

template<typename t1, typename t2>
void fn(const t1& v1, const t2& v2)
{
  if (v1 != v2)
    abort();
}

struct s
{
  unsigned long long f1 : 40;
  unsigned int f2 : 24;
};

s sv;

int main()
{
  sv.f1 = 0;
  sv.f2 = (1 << 24) - 1;
  fn(sv.f1, 0);
  fn(sv.f2, (1 << 24) - 1);
  ++sv.f2;
  fn(sv.f2, 0);
  return 0;
}
Comment 1 Andrew Pinski 2007-10-24 23:51:37 UTC
As far as I know this code is invlaid as you should not able take the address of the bitfield.
Comment 2 Ian Lance Taylor 2007-10-25 05:07:08 UTC
Nothing is taking the address of a bitfield.  It's a const reference, which should get initialized with the value.  If the reference is not const, then the compiler gives an error.

In any case, this code fails the same way, and clearly does not take the address of a bitfield.

extern "C" void abort() __attribute__ ((noreturn));

template<typename t1, typename t2>
void fn(const t1, const t2) __attribute__ ((noinline));

template<typename t1, typename t2>
void fn(const t1 v1, const t2 v2)
{
  if (v1 != v2)
    abort();
}

struct s
{
  unsigned long long f1 : 40;
  unsigned int f2 : 24;
};

s sv;

int main()
{
  sv.f1 = 0;
  sv.f2 = (1 << 24) - 1;
  int f2 = sv.f2;
  fn(f2, (1 << 24) - 1);
  ++sv.f2;
  f2 = sv.f2;
  fn(f2, 0);
  return 0;
}
Comment 3 Andrew Pinski 2007-10-25 05:21:47 UTC
  int f2 = sv.f2;

That should sign extend as far as I know.
Comment 4 Ian Lance Taylor 2007-10-25 13:25:08 UTC
Yes, of course it should sign extend.

This is a wrong-code bug.
Comment 5 Andrew Pinski 2007-12-08 20:07:32 UTC
  <unnamed-unsigned:24> D.2020;

<bb 2>:
  sv.f1 = 0;
  sv.f2 = 16777215;
  fn (16777215, 16777215);
  D.2020 = sv.f2 + 1;
  sv.f2 = D.2020;
  fn ((int) D.2020, 0);


So this is a middle-end issue of not doing an AND and then a sign extend.
Comment 6 Richard Biener 2007-12-11 12:38:15 UTC
In reply to comment #3:

no, 

int f2 = sv.f2;

does _not_ sign extend.  4.7/3 says "If the destination type is signed, the
value is unchanged if it can be represented in the destination type...".
Note your bitfield is unsigned.

Instead the error is with

  ++sv.f2;
  f2 = sv.f2;
  fn(f2, 0);

which should result in zero (as tested).  But we expand the above to

;; D.2041 = sv.f2 + 1
(insn 20 17 21 t.ii:27 (set (reg/f:DI 71)
        (const:DI (plus:DI (symbol_ref:DI ("sv") [flags 0x2] <var_decl 0x2b1ba3468140 sv>)
                (const_int 4 [0x4])))) -1 (nil))

(insn 21 20 22 t.ii:27 (set (reg:SI 73)
        (mem/s/c:SI (reg/f:DI 71) [0+4 S4 A32])) -1 (nil))

(insn 22 21 23 t.ii:27 (parallel [
            (set (reg:SI 72)
                (lshiftrt:SI (reg:SI 73)
                    (const_int 8 [0x8])))
            (clobber (reg:CC 17 flags))
        ]) -1 (expr_list:REG_EQUAL (lshiftrt:SI (mem/s/c:SI (reg/f:DI 71) [0+4 S4 A32])
            (const_int 8 [0x8]))
        (nil)))

(insn 23 22 0 t.ii:27 (parallel [
            (set (reg:SI 58 [ D.2041 ])
                (plus:SI (reg:SI 72)
                    (const_int 1 [0x1])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

;; sv.f2 = D.2041
(insn 25 23 26 t.ii:27 (set (reg/f:DI 75)
        (const:DI (plus:DI (symbol_ref:DI ("sv") [flags 0x2] <var_decl 0x2b1ba3468140 sv>)
                (const_int 4 [0x4])))) -1 (nil))

(insn 26 25 27 t.ii:27 (parallel [
            (set (reg:SI 76)
                (ashift:SI (reg:SI 58 [ D.2041 ])
                    (const_int 8 [0x8])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

(insn 27 26 28 t.ii:27 (set (reg:SI 77)
        (mem/s/j/c:SI (reg/f:DI 75) [0+4 S4 A32])) -1 (nil))

(insn 28 27 29 t.ii:27 (parallel [
            (set (reg:SI 78)
                (and:SI (reg:SI 77)
                    (const_int 255 [0xff])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

(insn 29 28 30 t.ii:27 (parallel [
            (set (reg:SI 79)
                (ior:SI (reg:SI 78)
                    (reg:SI 76)))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

(insn 30 29 0 t.ii:27 (set (mem/s/j/c:SI (reg/f:DI 75) [0+4 S4 A32])
        (reg:SI 79)) -1 (nil))

;; fn ((int) D.2041, 0)
(insn 31 30 32 t.ii:29 (set (reg:SI 4 si)
        (const_int 0 [0x0])) -1 (nil))

(insn 32 31 33 t.ii:29 (set (reg:SI 5 di)
        (reg:SI 58 [ D.2041 ])) -1 (nil))

(call_insn 33 32 0 t.ii:29 (call (mem:QI (symbol_ref/i:DI ("_Z2fnIiiEvT_T0_") [flags 0x1] <function_decl 0x2b1ba3461820 fn>) [0 S1 A8])
...

because reg:SI 58 is not a properly truncated intermediate result.
Comment 7 Richard Biener 2007-12-11 12:50:33 UTC
Reduced testcase:

extern "C" void abort() __attribute__ ((noreturn));

struct s
{
  unsigned long long f1 : 40;
  unsigned int f2 : 24;
} sv;

int main()
{
  int f2;
  sv.f2 = (1 << 24) - 1;
  __asm__ volatile ("" : : : "memory");
  ++sv.f2;
  f2 = sv.f2;
  if (f2 != 0)
    abort();
  return 0;
}

it doesn't fail with the C frontend though, even though the IL before
expansion is the same:

  <unnamed-unsigned:24> D.1548;

<bb 2>:
  sv.f2 = 16777215;
  __asm__ __volatile__("":::"memory");
  D.1548 = sv.f2 + 1;
  sv.f2 = D.1548;
  if (D.1548 != 0)

the difference is:

 (insn 13 10 14 3 t5.i:14 (set (reg/f:DI 67)
-        (const:DI (plus:DI (symbol_ref:DI ("sv") <var_decl 0x2b6b88bba320 sv>)
+        (const:DI (plus:DI (symbol_ref:DI ("sv") [flags 0x2] <var_decl 0x2b2e527ee0a0 sv>)
                 (const_int 4 [0x4])))) -1 (nil))
 
 (insn 14 13 15 3 t5.i:14 (set (reg:SI 69)
@@ -197,59 +185,52 @@
             (const_int 8 [0x8]))
         (nil)))
 
-(insn 16 15 17 3 t5.i:14 (parallel [
-            (set (reg:SI 58 [ D.1548 ])
+(insn 16 15 18 3 t5.i:14 (parallel [
+            (set (reg:SI 58 [ D.2025 ])
                 (plus:SI (reg:SI 68)
                     (const_int 1 [0x1])))
             (clobber (reg:CC 17 flags))
         ]) -1 (nil))
 
-(insn 17 16 19 3 t5.i:14 (parallel [
-            (set (reg:SI 58 [ D.1548 ])
-                (and:SI (reg:SI 58 [ D.1548 ])
-                    (const_int 16777215 [0xffffff])))
-            (clobber (reg:CC 17 flags))
-        ]) -1 (nil))
-
-(insn 19 17 20 3 t5.i:14 (set (reg/f:DI 71)
-        (const:DI (plus:DI (symbol_ref:DI ("sv") <var_decl 0x2b6b88bba320 sv>)
+(insn 18 16 19 3 t5.i:14 (set (reg/f:DI 71)
+        (const:DI (plus:DI (symbol_ref:DI ("sv") [flags 0x2] <var_decl 0x2b2e527ee0a0 sv>)
                 (const_int 4 [0x4])))) -1 (nil))
 
-(insn 20 19 21 3 t5.i:14 (parallel [
+(insn 19 18 20 3 t5.i:14 (parallel [
             (set (reg:SI 72)
-                (ashift:SI (reg:SI 58 [ D.1548 ])
+                (ashift:SI (reg:SI 58 [ D.2025 ])
                     (const_int 8 [0x8])))
             (clobber (reg:CC 17 flags))
         ]) -1 (nil))
 
...

Comment 8 Richard Biener 2007-12-11 13:08:27 UTC
Which is because

  if (lang_hooks.reduce_bit_field_operations
      && TREE_CODE (type) == INTEGER_TYPE
      && GET_MODE_PRECISION (mode) > TYPE_PRECISION (type))
    {
      /* An operation in what may be a bit-field type needs the 
         result to be reduced to the precision of the bit-field type,
         which is narrower than that of the type's mode.  */
      reduce_bit_field = true;
      if (modifier == EXPAND_STACK_PARM)
        target = 0;
    }

so this becomes a frontend bug again, because none but c-objc-common.h sets
that langhook to true.
Comment 9 Richard Biener 2007-12-11 13:31:29 UTC
Fails since 4.1.0, works up to 4.0.4.
Comment 10 Richard Biener 2007-12-11 13:34:58 UTC
I wonder what broke this.  Janis, can you hunt this with the testcase in
comment #7?  (g++ -O is enough)
Comment 11 Richard Biener 2007-12-11 15:55:07 UTC
The following fixes this failure, but breaks libjava a lot:

Index: cp/cp-lang.c
===================================================================
--- cp/cp-lang.c        (revision 130773)
+++ cp/cp-lang.c        (working copy)
@@ -54,6 +54,8 @@ static const char * cxx_dwarf_name (tree
 #define LANG_HOOKS_FOLD_OBJ_TYPE_REF cp_fold_obj_type_ref
 #undef LANG_HOOKS_INIT_TS
 #define LANG_HOOKS_INIT_TS cp_init_ts
+#undef LANG_HOOKS_REDUCE_BIT_FIELD_OPERATIONS
+#define LANG_HOOKS_REDUCE_BIT_FIELD_OPERATIONS true
 
 /* Each front end provides its own lang hook initializer.  */
 const struct lang_hooks lang_hooks = LANG_HOOKS_INITIALIZER;


but if that is not the correct fix, then the IL emitted by the frontend
for the increment

  <<cleanup_point <<< Unknown tree: expr_stmt
  (void)  ++sv.f2 >>>
>>;

or its gimplification (for !reduce_bit_field_operations languages)

  <unnamed-unsigned:24> D.2024;
  <unnamed-unsigned:24> D.2025;

...
    D.2024 = sv.f2;
    D.2025 = D.2024 + 1;
    sv.f2 = D.2025;

is wrong.
Comment 12 Janis Johnson 2007-12-11 23:56:47 UTC
Before noticing comment #9, I tested a few saved installs of trunk and ran a regression hunt for trunk on powerpc-linux using the testcase from comment #7, which found this patch that caused the test to start failing:

    http://gcc.gnu.org/viewcvs?view=rev&rev=126149

    r126149 | dberlin | 2007-06-30 14:15:26 +0000 (Sat, 30 Jun 2007)

I've started off additional reghunts for a break between 4.0 and 4.1 and a fix on mainline between 20061027 and 20070321.
Comment 13 Janis Johnson 2007-12-12 22:04:38 UTC
Additional regression hunts on trunk discovered that the test starts failing with:

    http://gcc.gnu.org/viewcvs?view=rev&rev=103956

    r103956 | steven | 2005-09-06 18:51:26 +0000 (Tue, 06 Sep 2005)

and starts passing with:

    http://gcc.gnu.org/viewcvs?view=rev&rev=119760

    r119760 | dnovillo | 2006-12-12 01:48:51 +0000 (Tue, 12 Dec 2006)

Comment #12 reports when it starts failing again.
Comment 14 Richard Biener 2007-12-12 22:47:38 UTC
Great.  That doesn't make too much sense ;)  Because the testcase in comment #7
cannot be "optimized" on the tree level, but I see it is wrongly expanded to
RTL instead.

Now, the revisions are

 103956: make tree-PRE smarter, looking through loads
 119760: mem-ssa merge (my favorite)
 126149: SCCVN merge

nothing expansion related or FE specific.  Bah.
Comment 15 Paolo Bonzini 2007-12-14 14:31:52 UTC
I have a patch that makes the reduce_bitfield_operations langhook a per-type field, but it doesn't affect code generation.

Isn't there a testcase in the C++ library that fails if the langhook is false?...
Comment 16 rguenther@suse.de 2007-12-14 14:36:44 UTC
Subject: Re:  [4.1/4.2/4.3 Regression] Reference to bitfield
 gets wrong value when optimizing

On Fri, 14 Dec 2007, bonzini at gnu dot org wrote:

> I have a patch that makes the reduce_bitfield_operations langhook a per-type
> field, but it doesn't affect code generation.
> 
> Isn't there a testcase in the C++ library that fails if the langhook is
> false?...

Unfortunately not :(  But there are lots of libjava testsuite failures.

Richard.
Comment 17 Richard Biener 2008-01-02 15:22:57 UTC
Looking at assembly differences in libjava doesn't reveal something obvious.  It
seems to be neither boolean bitfields nor enums (which would have been the
obvious differences where C and C++ semantics differ).  Mark, what else could
break if we reduce bitfield operations for C++?  I still don't see how _not_
reducing bitfield operations for any language could be the right thing to do
(apart from the fact that it may generate less RTL if the FE already makes sure
the reduction happens).
Comment 18 Alexandre Oliva 2008-01-11 06:46:36 UTC
I don't see anything in expand_expr_real_1 that, given something like (wider)narrower_typed_value, would reduce the value in a way that takes the narrower_type into account.  NOP_EXPR and CONVERT_EXPR will just expand the operand and be done with it if the modes of both types are the same and, in the case of bitfields with the same mode but lower precision, they are.

Also, all uses of REDUCE_BIT_FIELD take only the target type into account, which means that, if the intermediate expression wasn't reduced at the time it was computed, then it won't ever be.  And when the reduce_bit_field_operation langhook is false, then the intermediate expression won't have been reduced at the time it was computed.  So we lose.

What I don't know is whether languages that set reduce_bit_field_operation could expect expanders to take care of such expansions for them (they'd better, since the need for explicit reduction code is determined by whether the value is extracted from a bit-field in memory or held in a gimple reg that might be the result of optimization), or they're supposed to take care of the widening extension themselves (say, genericize (wider)unsigned_narrower_type_value as (wider)unsigned_narrower_type_value & mask, and (wider)signed_narrower_type_value as (((wider)signed_narrower_type_value & mask) ^ signbit) - signbit).

Which is it?  I.e., when !reduce_bit_field_operation, should expand_expr_real_1() perform the reduction to the inner type in widening conversions, or should convert_to_integer() take care of it when outprec > inprec?
Comment 19 Richard Biener 2008-01-14 15:45:40 UTC
The bug here is that the FE does not perform integer promotion for ++sv.f2,
but emits

  <<cleanup_point <<< Unknown tree: expr_stmt
  (void)  ++sv.f2 >>> 
>>;

which is gimplified to operations on a bitfield type:

  <unnamed-unsigned:24> D.2043;
  <unnamed-unsigned:24> D.2044;

    D.2043 = sv.f2;
    D.2044 = D.2043 + 1;
    sv.f2 = D.2044;

Once the optimizer realizes it doesn't need to go through memory for sv.f2,
things go downhill, as expand now reaches D.2043 + 1 without the implicit
truncation done at the store to memory.

See my bitfield rant on gcc@ - it's all broke.

The proper C++ FE fix is to perform arithmetic in a promoted type here.
Comment 20 Richard Biener 2008-01-14 16:55:15 UTC
So even if you fix that, and you'll end up with

  <unnamed-unsigned:24> D.2029;

<bb 2>:
  D.2029 = (<unnamed-unsigned:24>) (unsigned int) ((int) x.i + -1);
  x.i = D.2029;
  if (D.2029 != 16777215)
    goto <bb 3>;
  else
    goto <bb 4>;

before expand, the truncation at the store to register D.2029 still is
not reflected in code generated by expand, if the language does not
have REDUCE_BIT_FIELD_OPERATIONS set.

Which means that the GIMPLE IL

  bool retval.0;
  <unnamed-unsigned:24> D.2025;
  int D.2026;
  int D.2027;
  unsigned int D.2028;
  <unnamed-unsigned:24> D.2029;
  unsigned int D.2030;
  unsigned int D.2031;

  D.2025 = x.i;
  D.2026 = (int) D.2025;
  D.2027 = D.2026 + -1;
  D.2028 = (unsigned int) D.2027;
  D.2029 = (<unnamed-unsigned:24>) D.2028;
  x.i = D.2029;
  D.2025 = x.i;
  retval.0 = D.2025 != 16777215;
  if (retval.0)

still does not have frontend independent semantics.  IMHO this is a
middle-end bug.

Let's hope that the IL fix remove the issues with libjava you get into
if enabling the langhook for C++.
Comment 21 Richard Biener 2008-01-14 17:08:54 UTC
Another testcase we miscompile due to the fact in comment #20  (w/o the temporary
tmp fold converts the comparison to a comparision with a BIT_FIELD_REF, which
is handled correctly (not optimized)):

extern "C" void abort (void);

struct s 
{
  unsigned long long f1 : 40;
  unsigned int f2 : 24;
};

s sv;

void __attribute__((noinline)) foo(unsigned int i)
{
  unsigned int tmp;
  sv.f2 = i;
  tmp = sv.f2;
  if (tmp != 0)
    abort ();
}

int main()
{
  foo (0xff000000u);
  return 0;
}
Comment 22 Alexandre Oliva 2008-01-14 19:56:21 UTC
Both are cases in which the absence of bit-field reduction *on widening conversions* causes the problem.

I realize the absence of such reductions can be harmful in other cases as well, e.g. division and right-shifts.  But for most cases, modulo semantics guarantees the correct results as long as we stick to the narrower type.  However, when modulo semantics is required, reduction must take place, e.g. for compare operations that don't disregard the bits that are not in the type's implied mask.

I believe the module semantics is the reasoning behind the no-reduction policy.  But I do see that we fail to implement it properly, and I don't know whether it is the front-end's responsibility to provide the missing bits explicitly, or the  middle end to cover for what the front end might be assuming the middle end will provide.
Comment 23 Richard Biener 2008-01-15 09:45:54 UTC
I don't see where there is a problem with widening conversions.  The problem is
we re-use the  (unsigned : 24) i; value for the comparison, which looks reasonable, but this _narrowing_ conversion is not reflected by actual code
in expand.

I believe the only way without touching expand too much is to make the frontend
more explicit (and of course generate code for widening/narrowing conversions).
Comment 24 Richard Biener 2008-01-15 13:23:34 UTC
So the issue is that for

void foo(unsigned int) (i)
{
  <unnamed-unsigned:24> i.0;

<bb 2>:
  i.0 = (<unnamed-unsigned:24>) i;
  sv.f2 = i.0;
  if ((unsigned int) i.0 != 0)

we neither emit code for the narrowing nor for the widening, but only
for the bitfield store:

;; i.0 = (<unnamed-unsigned:24>) i
(insn 6 5 0 t.ii:16 (set (reg:SI 58 [ i.0 ])
        (reg/v:SI 59 [ i ])) -1 (nil))

;; sv.f2 = i.0
... proper code

;; if ((unsigned int) i.0 != 0)
(insn 14 13 15 t.ii:18 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg:SI 58 [ i.0 ])
            (const_int 0 [0x0]))) -1 (nil))

(jump_insn 15 14 0 t.ii:18 (set (pc)
        (if_then_else (eq (reg:CCZ 17 flags)
                (const_int 0 [0x0]))
            (label_ref 0)
            (pc))) -1 (expr_list:REG_BR_PROB (const_int 9900 [0x26ac])
        (nil)))

see how we simply re-use the incoming register for the comparison.  Neither
did we truncate that, nor do we zero-extend before the comparison.

The C frontend produces the same IL but due to the activated langhook we
instead expand to

;; i.0 = (<unnamed-unsigned:24>) i
(insn 6 5 0 t.i:12 (parallel [
            (set (reg:SI 58 [ i.0 ])
                (and:SI (reg/v:SI 59 [ i ])
                    (const_int 16777215 [0xffffff])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

;; sv.f2 = i.0
... same code as for C++

;; if ((unsigned int) i.0 != 0)
(insn 14 13 15 t.i:14 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg:SI 58 [ i.0 ]) 
            (const_int 0 [0x0]))) -1 (nil))

(jump_insn 15 14 0 t.i:14 (set (pc)
        (if_then_else (eq (reg:CCZ 17 flags)
                (const_int 0 [0x0]))
            (label_ref 0)
            (pc))) -1 (expr_list:REG_BR_PROB (const_int 9900 [0x26ac])
        (nil)))

note we also do not emit code for the widening here.

The same is true for the signed case.

We can enable this particular behavior for all frontends with

Index: expr.c
===================================================================
--- expr.c      (revision 131542)
+++ expr.c      (working copy)
@@ -7157,7 +7157,11 @@ expand_expr_real_1 (tree exp, rtx target
       mode = TYPE_MODE (type);
       unsignedp = TYPE_UNSIGNED (type);
     }
-  if (lang_hooks.reduce_bit_field_operations
+  if ((lang_hooks.reduce_bit_field_operations
+       /* Always reduce conversion results to the target precision.  */
+       || code == NON_LVALUE_EXPR
+       || code == NOP_EXPR
+       || code == CONVERT_EXPR)
       && TREE_CODE (type) == INTEGER_TYPE
       && GET_MODE_PRECISION (mode) > TYPE_PRECISION (type))
     {

but that breaks libjava again.

It also does _not_ fix the original testcase, as that would require
reducing also the increment expression.  This can be fixed by properly
lowering increment expressions as with

Index: cp/typeck.c
===================================================================
*** cp/typeck.c (revision 131542)
--- cp/typeck.c (working copy)
*************** build_unary_op (enum tree_code code, tre
*** 4340,4345 ****
--- 4347,4375 ----
              }
            val = boolean_increment (code, arg);
          }
+       /* If the type is a bitfield, lower the expression to an
+          assignment with a properly promoted bitfield rvalue increment.
+
+            [5.3.2/1]  If x is not of type bool, the expression ++x is
+            equivalent to x+=1.
+
+          thus integer promotions are supposed to happen, and in the
+          case of bitfields it is important for semantics.  */
+       else if (INTEGRAL_TYPE_P (argtype)
+                && (TYPE_PRECISION (argtype)
+                    != GET_MODE_BITSIZE (TYPE_MODE (argtype))))
+         {
+           tree rarg = arg;
+           if (code == POSTINCREMENT_EXPR || code == POSTDECREMENT_EXPR)
+             rarg = save_expr (arg);
+           if (code == PREINCREMENT_EXPR || code == POSTINCREMENT_EXPR)
+             val = build_binary_op (PLUS_EXPR, rarg, inc, 1);
+           else
+             val = build_binary_op (MINUS_EXPR, rarg, inc, 1);
+           val = build_modify_expr (arg, NOP_EXPR, val);
+           if (code == POSTINCREMENT_EXPR || code == POSTDECREMENT_EXPR)
+             val = build_compound_expr (val, rarg);
+         }
        else
          val = build2 (code, TREE_TYPE (arg), arg, inc);


but this adjustment alone doesn't fix the original testcase either,
because we still do the comparison in the bitfield type.
Comment 25 Richard Biener 2008-01-15 14:05:29 UTC
Unassigning.  I like to have help from somebody knowing how to debug the
libjava failures.  The only bitfields are defined in jvmti.h.
Comment 26 Alexandre Oliva 2008-01-18 18:08:53 UTC
I installed the patch in comment #11 and rebuilt all libraries, then I started investigating the first testsuite failure: libjava.cni/PR9577.java.

So far, I've found out that it is the gnu.classpath.SystemProperties class initializer that throws an exception out of this checkcast:

#0  _Jv_IsAssignableFrom (source=0x2aaaad5ee1e0, target=0x2aaaad610a80) at ../../../libjava/java/lang/natClass.cc:1847
#1  0x00002aaaac2c7071 in _Jv_CheckCast (c=0x2aaaad610a80, obj=0x2aaab266ab40) at ../../../libjava/java/lang/natClass.cc:1887
#2  0x00002aaaac74e606 in java.util.Hashtable.putAllInternal(java.util.Map)void (this=0x2aaab2676f80, m=<value optimized out>) at java/util/Hashtable.java:864
#3  0x00002aaaac74e013 in java.util.Hashtable.clone()java.lang.Object (this=0x2aaab2676fc0) at java/util/Hashtable.java:558
#4  0x00002aaaac2df849 in gnu.classpath.SystemProperties.<clinit>()void () at gnu/classpath/SystemProperties.java:116

the cast is from java.util.Hashtable$Entry to java.util.Map$Entry, which *should* pass.  But target->ioffsets is NULL, so the assignable test returns false.  I don't think it should be NULL, though.  Something is already broken at this point.
Comment 27 Alexandre Oliva 2008-01-18 18:47:52 UTC
Found it (or at least the first one):
in link.cc:665, has_interfaces is a jboolean (unsigned 1-bit type), but it's operated on like this:
      has_interfaces += klass0->interface_count;

if interface_count is even (which, in this case, it is), when has_interfaces is reduced to 1 bit with a bit mask, we lose.
Comment 28 Alexandre Oliva 2008-01-18 19:11:58 UTC
Subject: Bug 33887

Author: aoliva
Date: Fri Jan 18 19:11:15 2008
New Revision: 131632

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131632
Log:
PR c++/33887
* link.cc (_Jv_Linker::prepare_constant_time_tables):
has_interfaces is boolean, treat it as such.

Modified:
    trunk/libjava/ChangeLog
    trunk/libjava/link.cc

Comment 29 Richard Biener 2008-01-18 21:04:32 UTC
I'm trying again with enabling the langhook for C++.
Comment 30 Alexandre Oliva 2008-01-19 00:51:31 UTC
I tried that myself (patch in comment #11) and got no regressions.  It's a reasonable possibility, but isn't it a bit too early to close the bug? :-)
Comment 31 Richard Biener 2008-01-21 09:20:13 UTC
Err, did I really close this bug?!  I wanted to assign it to myself ...
Comment 33 Mark Mitchell 2008-01-25 05:35:07 UTC
This patch:

http://gcc.gnu.org/ml/gcc-patches/2008-01/msg00862.html

is OK.  This patch:

http://gcc.gnu.org/ml/gcc-patches/2008-01/msg00951.html

is OK if no objections from a Java maintainer within 24 hours.

Comment 34 Richard Biener 2008-01-25 12:07:20 UTC
Subject: Bug 33887

Author: rguenth
Date: Fri Jan 25 12:06:31 2008
New Revision: 131823

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131823
Log:
2008-01-25  Richard Guenther  <rguenther@suse.de>

	PR c++/33887
	* cp-lang.c (LANG_HOOKS_REDUCE_BIT_FIELD_OPERATIONS): Define
	to true.

	* g++.dg/torture/pr33887-1.C: New testcase.
	* g++.dg/torture/pr33887-2.C: Likewise.
	* g++.dg/torture/pr33887-3.C: Likewise.
	* gcc.c-torture/execute/20071211-1.c: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/torture/pr33887-1.C
    trunk/gcc/testsuite/g++.dg/torture/pr33887-2.C
    trunk/gcc/testsuite/g++.dg/torture/pr33887-3.C
    trunk/gcc/testsuite/gcc.c-torture/execute/20071211-1.c
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-lang.c
    trunk/gcc/testsuite/ChangeLog

Comment 35 Richard Biener 2008-01-25 12:10:35 UTC
Fixed on the trunk.
Comment 36 Tom Tromey 2008-01-25 20:48:38 UTC
The second patch is fine by me, you might as well commit it now.
Thanks.
Comment 37 Richard Biener 2008-01-25 21:20:46 UTC
Subject: Bug 33887

Author: rguenth
Date: Fri Jan 25 21:20:00 2008
New Revision: 131840

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131840
Log:
2008-01-25  Richard Guenther  <rguenther@suse.de>

	PR c++/33887
	* decl.c (record_builtin_java_type): Make __java_boolean
	a variant of bool.
	* typeck.c (structural_comptypes): Move TYPE_FOR_JAVA check
	after TYPE_MAIN_VARIANT check.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/cp/typeck.c

Comment 38 Joseph S. Myers 2008-02-01 16:54:58 UTC
4.2.3 is being released now, changing milestones of open bugs to 4.2.4.
Comment 39 Richard Biener 2008-02-07 18:03:27 UTC
*** Bug 33205 has been marked as a duplicate of this bug. ***
Comment 40 Richard Biener 2008-03-12 14:26:36 UTC
Subject: Bug 33887

Author: rguenth
Date: Wed Mar 12 14:25:48 2008
New Revision: 133142

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=133142
Log:
2008-03-12  Richard Guenther  <rguenther@suse.de>

	PR c++/35469
	Revert:
	2008-02-04  Richard Guenther  <rguenther@suse.de>

        PR java/35035
        * decl.c (record_builtin_java_type): Make jboolean a
        integer type again where its mode doesn't match that of bool.

	2008-01-25  Richard Guenther  <rguenther@suse.de>

        PR c++/33887
        * decl.c (record_builtin_java_type): Make __java_boolean
        a variant of bool.
        * typeck.c (structural_comptypes): Move TYPE_FOR_JAVA check
        after TYPE_MAIN_VARIANT check.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/cp/typeck.c

Comment 41 Richard Biener 2008-03-12 14:30:23 UTC
Subject: Bug 33887

Author: rguenth
Date: Wed Mar 12 14:29:35 2008
New Revision: 133143

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=133143
Log:
2008-03-12  Richard Guenther  <rguenther@suse.de>

	PR c++/35469
	Revert:
	2008-02-04  Richard Guenther  <rguenther@suse.de>

        PR java/35035
        * decl.c (record_builtin_java_type): Make jboolean a
        integer type again where its mode doesn't match that of bool.

	2008-01-25  Richard Guenther  <rguenther@suse.de>

        PR c++/33887
        * decl.c (record_builtin_java_type): Make __java_boolean
        a variant of bool.
        * typeck.c (structural_comptypes): Move TYPE_FOR_JAVA check
        after TYPE_MAIN_VARIANT check.

Modified:
    branches/gcc-4_3-branch/gcc/cp/ChangeLog
    branches/gcc-4_3-branch/gcc/cp/decl.c
    branches/gcc-4_3-branch/gcc/cp/typeck.c

Comment 42 Bernhard Reutner-Fischer 2008-04-18 14:03:14 UTC
Isn't this fixed now?
Comment 43 Richard Biener 2008-04-18 15:06:27 UTC
Not in 4.2 or 4.1.
Comment 44 Richard Biener 2008-05-07 19:43:39 UTC
*** Bug 36122 has been marked as a duplicate of this bug. ***
Comment 45 Joseph S. Myers 2008-05-19 20:23:37 UTC
4.2.4 is being released, changing milestones to 4.2.5.
Comment 46 Joseph S. Myers 2008-07-04 22:52:27 UTC
Closing 4.1 branch.
Comment 47 Joseph S. Myers 2009-03-31 00:35:44 UTC
Closing 4.2 branch, fixed in 4.3.