[cfe-commits] [cfe-dev] Miscompilation of sizeof

[Abramo Bagnara]

Il 12/12/2011 17:29, Reid Kleckner ha scritto:
> Most people use clang with assertions disabled, so this doesn't really
> solve the underlying problem.

Yes, it helps only to realize the issue (consider that there is a
pending issue in bugzilla submitted more than one year ago).

I guess this has not been yet solved because it impacts on several things.

This is the reason why I've proposed the patch: hiding the problem while
code is miscompiled does not help anyone, perhaps if the problem is more
visible it will be easy to find someone that will fix it.

Also to decide (once seen the issue) that type size should be evaluated
in bytes and not in bits would help. However I don't know if this is
acceptable... I've asked, but I got no answer about the reason why it
was considered that to compute the size in bits was the right thing to do.


> On Sun, Dec 11, 2011 at 3:55 AM, Abramo Bagnara
> <abramo.bagnara at gmail.com <mailto:abramo.bagnara at gmail.com>> wrote:
> 
>     Il 10/12/2011 22:41, David Blaikie ha scritto:
>     > Did you mean to attach a patch showing the assert you intend to add?
> 
>     I believed it was not needed, but I've attached it now for review.
> 
>     With that patch applied the following testcase (specific for 64 bit
>     architecture) now triggers the assertion instead to be horribly
>     miscompiled.
> 
>     #include <stdio.h>
> 
>     typedef int x[1UL<<59];
> 
>     int main() {
>      printf("%lu\n", sizeof(x));
>     }
> 
> 
>     > On Sat, Dec 10, 2011 at 12:52 PM, Abramo Bagnara
>     > <abramo.bagnara at gmail.com <mailto:abramo.bagnara at gmail.com>> wrote:
>     >>
>     >> Ping.
>     >>
>     >>>
>     >>> In ASTContext:::getTypeInfo(const Type *T) const we have:
>     >>>
>     >>>   uint64_t Width=0;
>     >>>
>     >>>   case Type::ConstantArray: {
>     >>>     const ConstantArrayType *CAT = cast<ConstantArrayType>(T);
>     >>>
>     >>>     std::pair<uint64_t, unsigned> EltInfo =
>     >>> getTypeInfo(CAT->getElementType());
>     >>>     Width = EltInfo.first*CAT->getSize().getZExtValue();
>     >>>
>     >>> But this multiplication can overflow (because for reasons that I
>     don't
>     >>> known getTypeInfo return width specified in bits).
>     >>>
>     >>> If there are no objections I'd add an assert: probably we'll
>     induce some
>     >>> crashes, but I believe that this would be *far* better than to
>     >>> miscompile the code (and assertions will be triggered only when code
>     >>> would be miscompiled).
>     >>>