[PATCH] D31885: Remove TBAA information from LValues representing union members

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 19 18:11:56 PDT 2017


rjmccall added a comment.

In https://reviews.llvm.org/D31885#731306, @hfinkel wrote:

> In https://reviews.llvm.org/D31885#730853, @hfinkel wrote:
>
> >
>
>
> ...
>
> > 
> > 
> >> (And the nonsensicality of the standard very much continues. The code that we've all agreed is obviously being miscompiled here is still a strict violation of the "improved" union rules in C++, and if we decide to treat it as ill-formed we're going to break a massive amount of code (and vector code in particular heavily uses unions), because trust me, nobody is reliably placement-newing union fields when they're non-trivial. C++'s new rules are unworkable in part because they rely on C++-specific features that cannot be easily adopted in headers which need to be language-neutral.)
> > 
> > I think we all agree that the vector types need to have their scalar (element) types as parents in the current TBAA representation. That's a separate change that we should just make. That, unfortunately, only fixes a subset of the union-related miscompiles.
>
> Looking at this in more detail, we actually generate builtin-vector types which use char* as their access type (always, even for floating-point vectors). It is only the struct-path info which causes the issue for the test case here. It might be reasonable to make this **less** conservative, but that's another story...


Unfortunately, probably not.  The root problem there is that the design of vector types and vector interfaces is generally quite bad; you cannot rely on things like vectors being stored with an appropriate element type for whatever value the user is actually trying to work with.

For example, Clang's xmmintrin.h specifies that __m128 is a vector of 4 ints.  How confident are you that that type is never used to store a vector of 4 floats?  Keep in mind that the compiler allows __m128 to freely implicitly convert to __v4sf.

John.


Repository:
  rL LLVM

https://reviews.llvm.org/D31885





More information about the cfe-commits mailing list