RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp

Fri Apr 11 10:34:32 UTC 2014

On Fri, Apr 11, 2014 at 3:58 AM, David Holmes <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/
>>
>
> 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.

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>
>> 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
>>>>
>>>> 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/
>>>>> 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).
>>>>>
>>>>> In the new C++11/14 standard
>>>>> (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).
>>>>>
>>>>> 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://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
>>>>>
>>>>>
>>>