Re: [patch] Fix wrong code with small structure return on PowerPC

[Richard Biener <richard dot guenther at gmail dot com>]

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.