Fix wrong code with small structure return on PowerPC

Message ID 14904386.2vhstX5eeo@polaris
State New
Headers show
Series
  • Fix wrong code with small structure return on PowerPC
Related show

Commit Message

Eric Botcazou Sept. 11, 2017, 10:26 a.m.
Hi,

this is a bug originally reported in Ada on 32-bit PowerPC with the SVR4 ABI 
(hence not Linux) and reproducible in C with the attached testcase at -O1.

The problem lies in function 'foo':

  struct S ret;
  char r, s, c1, c2;
  char *p = &r;

  s = bar (&p);
  if (s)
    c2 = *p;
  c1 = 0;

  ret.c1 = c1;
  ret.c2 = c2;
  return ret;

Since the call to bar returns 0, c2 is uninitialized at run time (but this 
doesn't matter for correctness since its value is never read).  Now both c1 
and c2 are represented at the RTL level by unsigned promoted subregs, i.e. 
SUBREGs verifying SUBREG_PROMOTED_VAR_P and SUBREG_PROMOTED_UNSIGNED_P

As a consequence, when store_fixed_bit_field_1 stores the 8-bit values into 
the 'ret' object, it considers that the underlying 32-bit objects have only 8 
bits set and the rest cleared so it doesn't mask the other 24 bits.  That's 
OK for c1 but not for c2, since c2 is uninitialized so the bits are random.

This appears to be an inherent weakness of the promoted subregs mechanism, but 
I don't think that we want to go for an upheaval at this point.  So the patch 
addresses the problem in an ad-hoc manner directly in store_fixed_bit_field_1 
and yields the expected 8-bit insertion:

@@ -26,7 +26,7 @@
        lwz 9,12(1)
        lbz 31,0(9)
 .L3:
-       slwi 3,31,16
+       rlwinm 3,31,16,8,15
        lwz 0,36(1)
        mtlr 0
        lwz 31,28(1)

Tested on x86-64/Linux and PowerPC64/Linux, OK for the mainline?


2017-09-11  Eric Botcazou  <ebotcazou@adacore.com>

	* expmed.c (store_fixed_bit_field_1): Force the masking if the value
 	is an unsigned promoted SUBREG of a sufficiently large object.


2017-09-11  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.c-torture/execute/20170911-1.c: New test.

Comments

Eric Botcazou Sept. 11, 2017, 10:37 a.m. | #1
> this is a bug originally reported in Ada on 32-bit PowerPC with the SVR4 ABI
> (hence not Linux) and reproducible in C with the attached testcase at -O1.

With the right C testcase this time...
Richard Biener Sept. 11, 2017, 12:05 p.m. | #2
On September 11, 2017 12:26:09 PM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>Hi,
>
>this is a bug originally reported in Ada on 32-bit PowerPC with the
>SVR4 ABI 
>(hence not Linux) and reproducible in C with the attached testcase at
>-O1.
>
>The problem lies in function 'foo':
>
>  struct S ret;
>  char r, s, c1, c2;
>  char *p = &r;
>
>  s = bar (&p);
>  if (s)
>    c2 = *p;
>  c1 = 0;
>
>  ret.c1 = c1;
>  ret.c2 = c2;
>  return ret;
>
>Since the call to bar returns 0, c2 is uninitialized at run time (but
>this 
>doesn't matter for correctness since its value is never read).  Now
>both c1 
>and c2 are represented at the RTL level by unsigned promoted subregs,
>i.e. 
>SUBREGs verifying SUBREG_PROMOTED_VAR_P and SUBREG_PROMOTED_UNSIGNED_P
>
>As a consequence, when store_fixed_bit_field_1 stores the 8-bit values
>into 
>the 'ret' object, it considers that the underlying 32-bit objects have
>only 8 
>bits set and the rest cleared so it doesn't mask the other 24 bits. 
>That's 
>OK for c1 but not for c2, since c2 is uninitialized so the bits are
>random.
>
>This appears to be an inherent weakness of the promoted subregs
>mechanism, but 
>I don't think that we want to go for an upheaval at this point.  So the
>patch 
>addresses the problem in an ad-hoc manner directly in
>store_fixed_bit_field_1 
>and yields the expected 8-bit insertion:
>
>@@ -26,7 +26,7 @@
>        lwz 9,12(1)
>        lbz 31,0(9)
> .L3:
>-       slwi 3,31,16
>+       rlwinm 3,31,16,8,15
>        lwz 0,36(1)
>        mtlr 0
>        lwz 31,28(1)
>
>Tested on x86-64/Linux and PowerPC64/Linux, OK for the mainline?

I think the issue is that we set SUBREG_PROMOTED_* on something that is possibly not so (aka uninitialized in this case). We may only set it if either the ABI or a previous operation forced it to. Maybe this is also the reason why we need this zero init pass in some cases (though that isn't a full solution either). 

Richard. 

>
>2017-09-11  Eric Botcazou  <ebotcazou@adacore.com>
>
>	* expmed.c (store_fixed_bit_field_1): Force the masking if the value
> 	is an unsigned promoted SUBREG of a sufficiently large object.
>
>
>2017-09-11  Eric Botcazou  <ebotcazou@adacore.com>
>
>	* gcc.c-torture/execute/20170911-1.c: New test.
Eric Botcazou Sept. 11, 2017, 7:59 p.m. | #3
> I think the issue is that we set SUBREG_PROMOTED_* on something that is
> possibly not so (aka uninitialized in this case).

Yes, that's what I called inherent weakness of the promoted subregs mechanism.

> We may only set it if either the ABI or a previous operation forced it to.
> Maybe this is also the reason why we need this zero init pass in some cases
> (though that isn't a full solution either).

Do you think that we should zero-init all the unsigned promoted subregs (and 
sign-extend-init all the signed promoted subregs)?  That sounds like a big 
hammer to me, but I can give it a try.
Richard Biener Sept. 13, 2017, 9:17 a.m. | #4
On Mon, Sep 11, 2017 at 9:59 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I think the issue is that we set SUBREG_PROMOTED_* on something that is
>> possibly not so (aka uninitialized in this case).
>
> Yes, that's what I called inherent weakness of the promoted subregs mechanism.
>
>> We may only set it if either the ABI or a previous operation forced it to.
>> Maybe this is also the reason why we need this zero init pass in some cases
>> (though that isn't a full solution either).
>
> Do you think that we should zero-init all the unsigned promoted subregs (and
> sign-extend-init all the signed promoted subregs)?  That sounds like a big
> hammer to me, but I can give it a try.

My suggestion would be to not set SUBREG_PROMOTED_* on "everything"
(which we seem to do).  zero-initing looks like the easier way to deal with
the situation though.

ISTR SUBREG_PROMOTED_* gets set by RTL expansion, the problematic
one should be the one in expr.c setting it on all SSA_NAMEs, maybe it is
enough to avoid setting it for SSA_NAME_IS_DEFAULT_DEF ones?

Richard.

> --
> Eric Botcazou

Patch

Index: expmed.c
===================================================================
--- expmed.c	(revision 251906)
+++ expmed.c	(working copy)
@@ -1213,13 +1213,25 @@  store_fixed_bit_field_1 (rtx op0, scalar
     }
   else
     {
-      int must_and = (GET_MODE_BITSIZE (value_mode) != bitsize
-		      && bitnum + bitsize != GET_MODE_BITSIZE (mode));
+      /* Compute whether we must mask the value in order to make sure that the
+	 bits outside the bitfield are all cleared.  Note that, even though the
+	 value has the right size, if it has a mode different from MODE, then
+	 converting it to MODE may unmask uninitialized bits in the case where
+	 it is an unsigned promoted SUBREG of a sufficiently large object.  */
+      const bool must_mask
+	= (GET_MODE_BITSIZE (value_mode) != bitsize
+	   || (value_mode != mode
+	       && GET_CODE (value) == SUBREG
+	       && SUBREG_PROMOTED_VAR_P (value)
+	       && SUBREG_PROMOTED_UNSIGNED_P (value)
+	       && GET_MODE_SIZE (GET_MODE (SUBREG_REG (value)))
+		  >= GET_MODE_SIZE (mode)))
+	  && bitnum + bitsize != GET_MODE_BITSIZE (mode);
 
       if (value_mode != mode)
 	value = convert_to_mode (mode, value, 1);
 
-      if (must_and)
+      if (must_mask)
 	value = expand_binop (mode, and_optab, value,
 			      mask_rtx (mode, 0, bitsize, 0),
 			      NULL_RTX, 1, OPTAB_LIB_WIDEN);