Bug 36741 - [4.3/4.4 regression] Bogus "large integer implicitly truncated" passing size_t constant to new
Summary: [4.3/4.4 regression] Bogus "large integer implicitly truncated" passing size_...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.4.0
: P2 normal
Target Milestone: 4.3.3
Assignee: Dodji Seketeli
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2008-07-06 07:08 UTC by Ian Lance Taylor
Modified: 2008-09-16 22:59 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.2.4
Known to fail: 4.3.1
Last reconfirmed: 2008-08-12 13:25:52


Attachments
primary candidate fix (600 bytes, patch)
2008-08-12 14:42 UTC, Dodji Seketeli
Details | Diff
second fix candidate (641 bytes, patch)
2008-08-12 16:10 UTC, Dodji Seketeli
Details | Diff
third fix candidate (883 bytes, patch)
2008-08-14 16:06 UTC, Dodji Seketeli
Details | Diff
Fourth fix candidate (915 bytes, patch)
2008-08-18 15:52 UTC, Dodji Seketeli
Details | Diff
5th patch (946 bytes, patch)
2008-08-19 11:00 UTC, Dodji Seketeli
Details | Diff
6th patch (892 bytes, patch)
2008-08-20 12:33 UTC, Dodji Seketeli
Details | Diff
7th patch (1010 bytes, patch)
2008-08-20 19:18 UTC, Dodji Seketeli
Details | Diff
8th version (848 bytes, patch)
2008-08-22 17:34 UTC, Dodji Seketeli
Details | Diff
gcc44-pr36741.patch (1.20 KB, patch)
2008-09-16 22:59 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Lance Taylor 2008-07-06 07:08:39 UTC
Compiling this simple C++ program with 4.3 or 4.4:

#include <stddef.h>
const char* foo() { return new char[~static_cast<size_t>(0)]; }

gives this warning:

foo.cc: In function ‘const char* foo()’:
foo.cc:2: warning: large integer implicitly truncated to unsigned type

The warning is bogus: there is no truncation here.  It would be reasonable to give a warning that the new would fail, but the warning it actually gives is simply wrong.  I haven't looked into this much, but it seems to be related to sizetype.

4.1 gives no warning.  I don't have 4.2 handy.
Comment 1 Andrew Pinski 2008-07-07 02:53:51 UTC
This is another sizetype issue.
Comment 2 Richard Biener 2008-07-10 14:24:55 UTC
Confirmed.
Comment 3 Dodji Seketeli 2008-08-12 14:42:15 UTC
Created attachment 16058 [details]
primary candidate fix

This minimal patch fixes the problem for me and regtests on x86_64.

I have some questions though and would like to have you guys comments, if possible.

My understanding is that the parameter of the new operator being size_t, its type should represented in GENERIC using an integer that is not signed.

The issue here is that the type of the parameter of the new operator has the same time as 'sizetype', which has its flag TYPE_IS_SIZETYPE set.

When TYPE_IS_SIZETYPE is set on an integer, some type checking machinery consider that integer to be signed. So the the representation of that integer is sign extended.

So a quick fix was to make sure the type of the parameter of the new operator has the same time as 'size_type_node', instead of 'sizetype'.

Maybe there is a better way of solving this.

Also, maybe I should modify the other uses of 'sizetype' in the build_new_1() function and replace them with "size_type_node' instead. But that's maybe out of the strict scope of this bug. I am not sure.
Comment 4 Dodji Seketeli 2008-08-12 16:10:31 UTC
Created attachment 16060 [details]
second fix candidate

This patch should be better than the previous one because it one must use size_binop() with a sizetype integer, not with a size_type_node one.

regtested on x86_64.
Comment 5 Dodji Seketeli 2008-08-14 16:06:24 UTC
Created attachment 16074 [details]
third fix candidate

This patch tries another approach.
Basically it changes the low level function (shared with the C front-end) that tests if an integer value fits within a given type.
It provides a special casing for integers of type sizetype.
Comment 6 Manuel López-Ibáñez 2008-08-15 20:16:22 UTC
(In reply to comment #5)
> Created an attachment (id=16074) [edit]
> third fix candidate
> 
> This patch tries another approach.

Patches go to gcc-patches@. 
Comment 7 Dodji Seketeli 2008-08-18 15:47:27 UTC
Manuel, yes I know that patches go to gcc-patches@ :-)

I am stacking these here to not loose them, but at the same time, I am not sure if they are solid enough for submission to gcc-patches. I am still working on them.

I will submit a patch to gcc-patches when I fill a bit more confident :-)
Comment 8 Dodji Seketeli 2008-08-18 15:52:00 UTC
Created attachment 16083 [details]
Fourth fix candidate

The previous patch was broken for say cross-compilers addressing a 64 target on a 32 bits host.

I hope this one is better.

Bootstrapped and regtested on x86_64.
Comment 9 Manuel López-Ibáñez 2008-08-18 16:03:28 UTC
(In reply to comment #7)
> 
> I am stacking these here to not loose them, but at the same time, I am not sure
> if they are solid enough for submission to gcc-patches. I am still working on
> them.

s/loose/lose/

OK but if you are expecting comments you should send the patch to gcc-patches. If they bootstrap+regtest they are solid enough to get comments in gcc-patches. ;-)

> I will submit a patch to gcc-patches when I fill a bit more confident :-)

s/fill/feel/

Do as you please but gcc-patches is where you will get the highest visibility. Just say that you are asking for comments [RFC] or help [RFH] and be clear that you are unsure about your approach. This will give reviewers a hint that they must be extra-careful.

Plus, you can ask for extra testing in different machines.
Comment 10 Dodji Seketeli 2008-08-19 11:00:48 UTC
Created attachment 16097 [details]
5th patch

This patch makes sure the integer type used in the conversion to a zero extended unsigned integral has a precision that is high enough.

regtested on x86_64.
Comment 11 Dodji Seketeli 2008-08-20 12:33:08 UTC
Created attachment 16108 [details]
6th patch

Another refinement.
Comment 12 Dodji Seketeli 2008-08-20 19:18:34 UTC
Created attachment 16117 [details]
7th patch

Another iteration :-)
Comment 13 Dodji Seketeli 2008-08-22 17:34:54 UTC
Created attachment 16131 [details]
8th version
Comment 14 Joseph S. Myers 2008-08-27 22:05:03 UTC
4.3.2 is released, changing milestones to 4.3.3.
Comment 15 Dodji Seketeli 2008-08-28 14:50:49 UTC
Subject: Bug 36741

Author: dodji
Date: Thu Aug 28 14:49:25 2008
New Revision: 139711

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=139711
Log:
2008-08-28  Dodji Seketeli  <dodji@redhat.com>

	PR c++/36741
	* tree.c (int_fits_type_p): Don't forget unsigned integers
	  of type sizetype which higher end word equals -1.


Added:
    branches/gcc-4_3-branch/gcc/testsuite/g++.dg/other/new-size-type.C
Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_3-branch/gcc/tree.c

Comment 16 Dodji Seketeli 2008-08-28 14:51:10 UTC
Subject: Bug 36741

Author: dodji
Date: Thu Aug 28 14:49:48 2008
New Revision: 139712

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=139712
Log:
2008-08-28  Dodji Seketeli  <dodji@redhat.com>

	PR c++/36741
	* tree.c (int_fits_type_p): Don't forget unsigned integers
	  of type sizetype which higher end word equals -1.


Added:
    trunk/gcc/testsuite/g++.dg/other/new-size-type.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree.c

Comment 17 Manuel López-Ibáñez 2008-08-28 14:56:43 UTC
Subject: Re:  [4.3/4.4 regression] Bogus "large integer implicitly truncated" passing size_t constant to new

2008/8/28 dodji at gcc dot gnu dot org <gcc-bugzilla@gcc.gnu.org>:
> Log:
> 2008-08-28  Dodji Seketeli  <dodji@redhat.com>
>
>        PR c++/36741
>        * tree.c (int_fits_type_p): Don't forget unsigned integers
>          of type sizetype which higher end word equals -1.
>
>
> Added:
>    trunk/gcc/testsuite/g++.dg/other/new-size-type.C

You forgot to mention this file in the commit log. Please rectify the log.

Thanks

Manuel.
Comment 18 Dodji Seketeli 2008-08-28 15:29:23 UTC
Fixed in trunk and gcc-4_3-branch.
Comment 19 David Edelsohn 2008-09-04 20:25:20 UTC
The patch for this bug significantly degrades PowerPC performance.
Comment 20 Dodji Seketeli 2008-09-04 20:42:41 UTC
Hello,

It would be nice to provide a testcase that could help reproduce the performance degradation easily.

Moreoever, I don't have access to a ppc box yet. How could I have that ?
Comment 21 Jakub Jelinek 2008-09-16 11:50:52 UTC
At least for the branch I'd very much prefer the second candidate patch, which is much less intrusive.
Comment 22 Jakub Jelinek 2008-09-16 22:59:36 UTC
Created attachment 16341 [details]
gcc44-pr36741.patch

Only lightly tested patch that reverts the tree.c change and instead does size computation in build_new_1 in size_t instead of sizetype.