SRA problem with uninitialzed fields

SRA problem with uninitialzed fields

[Richard Kenner]

Robert and I were just talking on the phone about implementation issues of
packed arrays in Ada and I realized there might be a problem with SRA.
Indeed there is and I can show it with a trivial C program. 

Consider:

struct foo {unsigned int f: 2;};

int
sub1 ()
{
  struct foo x;

  x.f |= 3;
  return x.f == 3;
}

SRA makes an x$f and here's the resulting .vars file:

sub1 ()
{
  <unnamed type> x$f;

  return (int) (<unnamed type>) (unsigned char) ((signed char) x$f | 3) == 3;
}

In the original C, the value of x.f is clearly uninitialized, but the
assignment statement forces it to be 3.  So the comparison is always true.

But in the resulting code, x$f is a full byte and so the upper six bits
remain uninitialized.  Thus the comparison above may or not may be true,
depending on how things happened to be initialized.

So clearly this is an unsafe optimization, but it's not clear what the
exact condition for safety is.  I suspect the point is that the SRA'ed
variable can't ever be uninitialized for this to work.

Re: SRA problem with uninitialzed fields

[Andrew Pinski]

On Sep 24, 2004, at 10:06 AM, Richard Kenner wrote:

> Robert and I were just talking on the phone about implementation 
> issues of
> packed arrays in Ada and I realized there might be a problem with SRA.
> Indeed there is and I can show it with a trivial C program.
>

> sub1 ()
> {
>   <unnamed type> x$f;
>
>   return (int) (<unnamed type>) (unsigned char) ((signed char) x$f | 
> 3) == 3;
> }

This looks like a bug in the expanders than the SRA as the <unnamed 
type>
is the right type in the sense it say it is only a certain number of 
bits
in this case 2.

Thanks,
Andrew Pinski

Re: SRA problem with uninitialzed fields

[Richard Kenner]

    This looks like a bug in the expanders than the SRA as the <unnamed
    type> is the right type in the sense it say it is only a certain
    number of bits in this case 2.

Indeed I stopped when I looked at the .vars since I "knew" we didn't do
that sort of masking.  But now I see that we actually do that masking
operation whenever TYPE_PRECISION is less than the size of the mode, if it's
required by the language (the default is false, but set true for C/C++/Objc).

I'd guess that logic was added for precisely this case and the generated
code omits the comparison since it's always known to be true.

I clearly have to turn that flag on for Ada and then revisit some of the
discussions I had this morning in this light.

Thanks!

Re: SRA problem with uninitialzed fields

[Joseph S. Myers]

On Fri, 24 Sep 2004, Richard Kenner wrote:

> Indeed I stopped when I looked at the .vars since I "knew" we didn't do
> that sort of masking.  But now I see that we actually do that masking
> operation whenever TYPE_PRECISION is less than the size of the mode, if it's
> required by the language (the default is false, but set true for C/C++/Objc).
> 
> I'd guess that logic was added for precisely this case and the generated
> code omits the comparison since it's always known to be true.
> 
> I clearly have to turn that flag on for Ada and then revisit some of the
> discussions I had this morning in this light.

Actually, this flag is false for C++, and only enabled for C and ObjC.  
It would be nice not to have this conditional and for the flag to be true 
unconditionally (or at least default to true).  I added the langhook in 
order to solve a particular C problem - giving bit-fields the restricted 
width types they are meant to have - and get the solution into GCC in a 
reasonable amount of time without needing to work out at once solutions to 
every way this might impact trees generated for other languages, only the 
front-end and middle-end issues shown up for C; hence the hook ensuring 
that it had no effect on languages other than C and ObjC.  Some middle-end 
issues then showed up on other targets, which I duly fixed; I would not be 
surprised if there are more issues relating to such types still present 
somewhere in the compiler.

C++ enums were one case of restricted-precision types for which this 
masking was not wanted, but for now 
<http://gcc.gnu.org/ml/gcc-patches/2004-08/msg01985.html> and until a C++ 
DR about this is resolved (see the long discussions on the gcc list last 
month) they don't have the restricted precision.  So it may be that the 
hook is not now needed.  There is also a check that masking is only 
applied for INTEGER_TYPE and not ENUMERAL_TYPE, and that may also not be 
needed (if C++ enums get the old behavior back, a separate bit to 
distinguish whether masking is needed would be a more useful distinction 
than that between INTEGER_TYPE and ENUMERAL_TYPE).

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: SRA problem with uninitialzed fields

[Richard Kenner]

    C++ enums were one case of restricted-precision types for which this 
    masking was not wanted, but for now 
    <http://gcc.gnu.org/ml/gcc-patches/2004-08/msg01985.html> and until a C++ 
    DR about this is resolved (see the long discussions on the gcc list last 
    month) they don't have the restricted precision.  

But what about the C test case I sent?  Isn't that valid C++ also and
wouldn't it have the problem I mentioned if that hook is false?

Re: SRA problem with uninitialzed fields

[Joseph S. Myers]

On Fri, 24 Sep 2004, Richard Kenner wrote:

> But what about the C test case I sent?  Isn't that valid C++ also and
> wouldn't it have the problem I mentioned if that hook is false?

C++ bit-fields have a restricted width representation but their type is 
the underlying type rather than a special restricted width type.  (Thus 
e.g. with 64-bit long, 32-bit int, unsigned long:40 + unsigned long:40 has 
type unsigned long:40 in C but type unsigned long in C++.)  A proper 
implementation that keeps the types those that accord with the language 
semantics would be for the GIMPLE generated to include explicit mask 
operations for assignments to bit-fields.  If it doesn't represent that 
assignment to a bit-field from the same type may change the value, then 
there are likely to be problems.  At one point there were, but I don't see 
such problems at present in some quick tests.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: SRA problem with uninitialzed fields

[Richard Kenner]

    A proper implementation that keeps the types those that accord with
    the language semantics would be for the GIMPLE generated to include
    explicit mask operations for assignments to bit-fields.  If it doesn't
    represent that assignment to a bit-field from the same type may change
    the value, then there are likely to be problems.  At one point there
    were, but I don't see such problems at present in some quick tests.

Well, look at the test case I sent. A mask is clearly required on the
comparison (at least, I think so!), but the only mechanism to generate it
is the hook, which is false for C++.

Re: SRA problem with uninitialzed fields

[Joseph S. Myers]

On Fri, 24 Sep 2004, Richard Kenner wrote:

> Well, look at the test case I sent. A mask is clearly required on the
> comparison (at least, I think so!), but the only mechanism to generate it
> is the hook, which is false for C++.

C++ generates the following for this testcase, which includes the mask.  
In accordance with the differences between how the C and C++ standards 
define bit-fields, it doesn't use the reduced-width types.

;; Function int sub1() (_Z4sub1v)

int sub1() ()
{
  struct foo x;

<bb 0>:
  x.f = x.f | 3;
  return (BIT_FIELD_REF <x, 8, 0> & 3) == 3;

}

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: SRA problem with uninitialzed fields

[Robert Dewar]

Joseph S. Myers wrote:
> On Fri, 24 Sep 2004, Richard Kenner wrote:

> C++ generates the following for this testcase, which includes the mask.  
> In accordance with the differences between how the C and C++ standards 
> define bit-fields, it doesn't use the reduced-width types.

I am completely puzzled as to what reduced-width types have to do
with this example at the C++ semantic level, or are you just making
a comment about the internal implementation and why it happens to
work for C++ and not for C (as far as I can see these programs have
identical semantics in C and C++)

Re: SRA problem with uninitialzed fields

[Joseph S. Myers]

On Fri, 24 Sep 2004, Robert Dewar wrote:

> I am completely puzzled as to what reduced-width types have to do
> with this example at the C++ semantic level, or are you just making
> a comment about the internal implementation and why it happens to
> work for C++ and not for C (as far as I can see these programs have
> identical semantics in C and C++)

The internal representation in the front ends most cleanly follows the 
rules of the respective standards.  This then leads to different 
representations in GIMPLE, although the semantics in this case are the 
same for C and C++.  It would no doubt be desirable for the tree 
optimizers to be able to do the same optimizations in both cases, perhaps 
translating one representation into the other (both being valid GIMPLE).

The cases which have different semantics are such as

struct s { unsigned long a:40, b:40; } x;

unsigned long f() { return x.a + x.b; }

where in C, but not C++, the result of the addition is masked to 40 bits 
(presuming 32-bit int, 64-bit long for this example).

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
  http://www.srcf.ucam.org/~jsm28/gcc/#c90status - status of C90 for GCC 4.0
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)