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 11:15:29 UTC 2014
On 11/04/2014 8:43 PM, David Holmes wrote:
> 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.
Ignore that. I just realized it is not the external linkage to these
methods that is the main issue, but some internal hotspot code calling
the global operator new/delete.
David
> 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