RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
David Holmes
david.holmes at oracle.com
Fri Apr 11 10:43:17 UTC 2014
On 11/04/2014 8:34 PM, Volker Simonis wrote:
> On Fri, Apr 11, 2014 at 3:58 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> On 11/04/2014 4:33 AM, Volker Simonis wrote:
>
> Hi David,
>
> thanks for looking at this issue.
>
> I agree with you and I've completely removed ALLOW_OPERATOR_NEW now:
>
> http://cr.openjdk.java.net/~__simonis/webrevs/8039805.v2/
> <http://cr.openjdk.java.net/~simonis/webrevs/8039805.v2/>
>
>
> I actually meant delete the whole block guarded by
> ALLOW_OPERATOR_NEW - we don't need these error-trapping definitions
> now we have fixed the export problem.
>
>
> OK, but arguing this way, we could remove remove all asserts from the
> code, once we fixed an error revealed by them.
Obviously you have to draw a line somewhere. But I don't think these
particular methods are worth the problem they are now causing.
Cheers,
David
> I think the error-trapping new/delete operators are still valuable in
> protecting people from accidentally calling them (and they don't really
> hurt anybody in terms of performance or space).
>
> Regards,
> Volker
>
> FYI I'm not in the runtime group.
>
> David
> -----
>
>
> I've also changed the "guarantee" into "fatal" as proposed by
> Vladimir.
>
> I've thought a little while about the problem that some other
> code may
> unintentionally use these operators but I couldn’t really find a
> scenario where this could happen. Because as you correctly pointed
> out, these operators aren't exported from libjvm.so - after all,
> that's the whole reason for compiling with visibility=hidden and
> using
> of export maps. So if another program/library will load
> libjvm.so, the
> operators won't be visible. On the other hand, if the libjvm.so
> loads
> another shared libraries which use these operators they either have
> their own, private versions of them or they are dynamically linked
> against another library (most probably the standard library) which
> provides versions of the operators.
>
> So if I'm not totally wrong, we could in principle also enable these
> operators in the product build. However, I'm not proposing that for
> this change. Let's first fix the signatures and get rid of
> ALLOW_OPERATOR_NEW with this change. If everything works fine,
> we can
> think about enabling these global operators in product builds as
> well.
>
> By the way - are you from the runtime group?
> I was asked to get a review from a runtime-group member -
> anybody out
> there willing to volunteer?
>
> Thank you and best regards,
> Volker
>
>
> On Thu, Apr 10, 2014 at 5:19 AM, David Holmes
> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>
> I think we should just delete the whole ALLOW_OPERATOR_NEW
> block. We fixed
> the problem of it being called outside the VM under 8014326.
>
> David
>
>
> On 10/04/2014 12:48 PM, David Holmes wrote:
>
>
> Hi Volker,
>
> Need more time to consider this in full but from the
> existing code:
>
> 689 // On certain platforms, such as Mac OS X
> (Darwin), in debug
> version, new is being called
> 690 // from jdk source and causing data corruption.
> Such as
> 691 //
> Java_sun_security_ec___ECKeyPairGenerator___generateECKeyPair
> 692 // define ALLOW_OPERATOR_NEW_USAGE for platform
> on which global
> operator new allowed.
>
> we actually fixed that by using the mapfiles to ensure
> the hotspot
> operator new was not visible externally. The existence of
> ALLOW_OPERATOR_NEW_USAGE wasn't even raised at the time :(
>
> https://bugs.openjdk.java.net/__browse/JDK-8014326
> <https://bugs.openjdk.java.net/browse/JDK-8014326>
>
> David
>
> On 10/04/2014 2:34 AM, Volker Simonis wrote:
>
>
> Hi,
>
> could you please review and sponsor the following
> small change:
>
> http://cr.openjdk.java.net/~__simonis/webrevs/8039805/
> <http://cr.openjdk.java.net/~simonis/webrevs/8039805/>
> https://bugs.openjdk.java.net/__browse/JDK-8039805
> <https://bugs.openjdk.java.net/browse/JDK-8039805>
>
> which fixes the signature of the global new/delete
> operators in
> allocation.cpp
>
> For non-product builds allocation.cpp defines global
> new/delete
> operators which shut down the VM if they get called
> at runtime. The
> rational behind this is that the these global
> operators should never
> be used in HotSpot.
>
> Unfortunately, the signature of some of these
> operators doesn't
> conform to the C++ standard which confuses some C++
> compilers. For a
> more detailed explanation of the C++ background of
> this issue see the
> new comments in allcoation.cpp and the end of this mail.
>
> This change also replaces the asserts in the
> operators with guarantees
> because the code may also be active in not-product
> (aka. 'optimized')
> builds.
>
> Finally, the webrev fixes two places in the AIX-port
> which used the
> global new operator. After the change we can now
> remove the definition
> of ALLOW_OPERATOR_NEW_USAGE from aix/makefiles/vm.make.
>
> I have also removed ALLOW_OPERATOR_NEW_USAGE from
> bsd/makefiles/vm.make and the corresponding comments
> in allcoation.cpp
> which state that on Mac OS X the global new/delete
> operators from the
> HotSpot cause problems together with usages of these
> operators from
> the class library such as the ones from
> Java_sun_security_ec___ECKeyPairGenerator___generateECKeyPair.
> I couldn’t
> observe any such problems but if anybody has some
> concrete concerns
> I'm ready to remove this part from the webrev.
>
> I've build and tested these changes on Linux/x86_64,
> Linux/ppc64,
> Solaris/Sparc, Windows/x86_64, MacOS X and
> AIX/ppc64. I've especially
> run the regression tests from sun/security/ec which
> exercise the
> method
> Java_sun_security_ec___ECKeyPairGenerator___generateECKeyPair
> which
> was mentioned to cause problems in conjunction with
> the globally
> defined new/delete operators from the HotSpot but
> couldn't see any
> issues, neither in the slowdebug nor in the
> optimized build.
>
> Following some C++ background regarding the global
> new/delete operators:
>
> In C++98/03 the throwing new operators are defined
> with the following
> signature:
>
> void* operator new(std::size_tsize)
> throw(std::bad_alloc);
> void* operator new[](std::size_tsize)
> throw(std::bad_alloc);
>
> while all the other (non-throwing) new and delete
> operators are
> defined with an empty throw clause (i.e. "operator
> delete(void* p)
> throw()") which means that they do not throw any
> exceptions (see
> section 18.4 of the C++ standard
> http://www.open-std.org/jtc1/__sc22/wg21/docs/papers/2005/__n1905.pdf
> <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1905.pdf>).
>
> In the new C++11/14 standard
> (http://www.open-std.org/jtc1/__sc22/wg21/docs/papers/2013/__n3797.pdf
> <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3797.pdf>),
> the signature of the throwing new operators was
> changed by completely
> omitting the throw clause (which effectively means
> they could throw
> any exception) while all the other new/delete
> operators where changed
> to have a 'nothrow' clause instead of an empty throw
> clause.
>
> Unfortunately, the support for exception
> specifications among C++
> compilers is still very fragile. While some more
> strict compilers like
> AIX xlC or HP aCC reject to override the default
> throwing new operator
> with a user operator with an empty throw() clause,
> the MS Visual C++
> compiler warns for every non-empty throw clause like
> throw(std::bad_alloc) that it will ignore the
> exception specification
> (see
> http://msdn.microsoft.com/en-__us/library/sa28fef8.aspx
> <http://msdn.microsoft.com/en-us/library/sa28fef8.aspx>).
>
> I've therefore changed the operator definitions such
> that they
> correctly work with all currently supported
> compilers and in way which
> should be upwards compatible with C++11/14.
>
> Please notice that I'm aware of the discussion
> around "8021954: VM
> SIGSEGV during classloading on MacOS; hs_err_pid
> file produced"
>
> (http://mail.openjdk.java.net/__pipermail/hotspot-runtime-dev/__2013-August/thread.html#8924
> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2013-August/thread.html#8924>,
>
> http://hg.openjdk.java.net/__hsx/hotspot-comp/hotspot/rev/__9758d9f36299
> <http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/9758d9f36299>)
> which introduced empty throw() clauses on all user
> defined new
> operators. But I think the rational used for that
> change doesn't apply
> here, because these global, user user defined new
> operators changed in
> this webrev aren't meant to be really used. There
> only task is to
> override the default, global operators (and
> therefore they need to
> have the right signature) and to shut the VM down if
> they get called.
>
> Thank you and best regards,
> Volker
>
>
>
More information about the hotspot-dev
mailing list