| Message ID | 4FC4F2AE.2090106@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 29, 2012 at 11:00 AM, Florian Weimer <fweimer@redhat.com> wrote: > This patch flags operator new on variably modified types as an error. > If this is acceptable, this will simplify the implementation of the > C++11 requirement to throw std::bad_array_new_length instead of > allocating a memory region which is too short. > > Okay for trunk? Or should I guard this with -fpermissive? I must say that ideally this should go in. However, this having been accepted in previous releases, I think people would like one release of deprecation. So my suggestion is: -- make it an error unless -fpermissive. -- if -fpermissive, make it unconditionally deprecated. -- schedule for entire removal in 4.9. > > 2012-05-29 Florian Weimer <fweimer@redhat.com> > > * init.c (build_new): Reject variably modified types. > > > 2012-05-29 Florian Weimer <fweimer@redhat.com> > > * g++.dg/init/new33.C: New. > > -- > Florian Weimer / Red Hat Product Security Team
On 05/29/2012 06:41 PM, Gabriel Dos Reis wrote: > On Tue, May 29, 2012 at 11:00 AM, Florian Weimer<fweimer@redhat.com> wrote: >> This patch flags operator new on variably modified types as an error. >> If this is acceptable, this will simplify the implementation of the >> C++11 requirement to throw std::bad_array_new_length instead of >> allocating a memory region which is too short. >> >> Okay for trunk? Or should I guard this with -fpermissive? > > I must say that ideally this should go in. However, this having > been accepted in previous releases, I think people would like > one release of deprecation. So my suggestion is: > -- make it an error unless -fpermissive. > -- if -fpermissive, make it unconditionally deprecated. > -- schedule for entire removal in 4.9. On the other hand, it is such an obscure feature that it is rather unlikely that it has any users. The usual C++ conformance fixes and libstdc++ header reorganizations cause much more pain, and no depreciation is required for them. Perhaps we can get away here without depreciation, too? I wrote a few tests for operator new[] (attached), and it does seem to work correctly as required. I secretly hoped it was broken, but no luck there.
On Wed, May 30, 2012 at 3:47 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 05/29/2012 06:41 PM, Gabriel Dos Reis wrote: >> >> On Tue, May 29, 2012 at 11:00 AM, Florian Weimer<fweimer@redhat.com> >> wrote: >>> >>> This patch flags operator new on variably modified types as an error. >>> If this is acceptable, this will simplify the implementation of the >>> C++11 requirement to throw std::bad_array_new_length instead of >>> allocating a memory region which is too short. >>> >>> Okay for trunk? Or should I guard this with -fpermissive? >> >> >> I must say that ideally this should go in. However, this having >> been accepted in previous releases, I think people would like >> one release of deprecation. So my suggestion is: >> -- make it an error unless -fpermissive. >> -- if -fpermissive, make it unconditionally deprecated. >> -- schedule for entire removal in 4.9. > > > On the other hand, it is such an obscure feature that it is rather unlikely > that it has any users. The usual C++ conformance fixes and libstdc++ header > reorganizations cause much more pain, and no depreciation is required for > them. > > Perhaps we can get away here without depreciation, too? That is a good point. Jason, what do you think? -- Gaby
On May 30, 2012, at 9:15 AM, Gabriel Dos Reis wrote: >> On the other hand, it is such an obscure feature that it is rather unlikely >> that it has any users. The usual C++ conformance fixes and libstdc++ header >> reorganizations cause much more pain, and no depreciation is required for >> them. >> >> Perhaps we can get away here without depreciation, too? > > That is a good point. Jason, what do you think? My take, -fpermissive is when 100s of projects make extensive use of the feature and you don't want to kill them all. :-) For really obscure corners, better to just fix bugs and refine semantics and not worry too much about it, life is too short. If there are tons of user reports of, you guys broke this... it can always be added in a .1 release later, if not caught before the .0 release.
On 05/29/2012 12:00 PM, Florian Weimer wrote: > This patch flags operator new on variably modified types as an error. > If this is acceptable, this will simplify the implementation of the > C++11 requirement to throw std::bad_array_new_length instead of > allocating a memory region which is too short. Hmm. I'm somewhat reluctant to outlaw a pattern that has an obvious meaning. On the other hand, it is an extension that is mostly there for C compatibility, which would not be affected by this restriction. So I guess the change is OK, but please add a comment about the motivation. Jason
On 05/29/2012 06:00 PM, Florian Weimer wrote: > This patch flags operator new on variably modified types as an error. > If this is acceptable, this will simplify the implementation of the > C++11 requirement to throw std::bad_array_new_length instead of > allocating a memory region which is too short. It turns out that the patch is not good enough. Apparently, people write new (T[n]) instead of new T[n] from time to time. This is ill-formed (for variable n), and is currently accepted silently. We even have test suite coverage for this. I'll try to warn about this case and make the transformation to the proper operator new[] call.
As someone who wrote code that does such allocations, I'm surprised that this
is a GNU extension, and is rejected even by the C++11 standard. How is one
supposed to perform allocations of two-dimensional arrays when inner dimension
is given as function argument?
I'm relatively inexperienced, but let me disagree about the assessment of this
feature as "obscure". I would expect that some existing programs that
perform, say, CFD calculations on two- or three-dimensional regular grids,
or, better yet, do image processing, would allocate grid data in exactly that
manner, and would be broken by this change:
void foo(int gridSizeH, int gridSizeV)
{
typedef double gridRow[gridSizeH];
gridRow *grid = new gridRow[gridSizeV];
...
}
Therefore I suggest a deprecation period with -fpermissive.
Thanks.
Alexander
Index: gcc/cp/init.c =================================================================== --- gcc/cp/init.c (revision 187951) +++ gcc/cp/init.c (working copy) @@ -2844,6 +2844,13 @@ if (!complete_type_or_maybe_complain (type, NULL_TREE, complain)) return error_mark_node; + if (variably_modified_type_p (type, NULL_TREE)) + { + if (complain & tf_error) + error ("new cannot be applied to a variably modified type"); + return error_mark_node; + } + rval = build_new_1 (placement, type, nelts, init, use_global_new, complain); if (rval == error_mark_node) return error_mark_node; Index: gcc/testsuite/g++.dg/init/new33.C =================================================================== --- gcc/testsuite/g++.dg/init/new33.C (revision 0) +++ gcc/testsuite/g++.dg/init/new33.C (revision 0) @@ -0,0 +1,10 @@ +// { dg-do compile } + +int +main (int argc, char **argv) +{ + typedef char A[argc]; + new A; // { dg-error "variably modified" } + new A[0]; // { dg-error "variably modified" } + new A[5]; // { dg-error "variably modified" } +}