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

[David Holmes]

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.

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
>>>>
>>