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

Thu Apr 10 18:33:25 UTC 2014

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