RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
Volker Simonis
volker.simonis at gmail.com
Tue Apr 29 13:43:16 UTC 2014
Hi everybody,
I'm still looking for a sponsor for this change.
Thanks,
Volker
On Fri, Apr 11, 2014 at 1:15 PM, David Holmes <david.holmes at oracle.com> wrote:
> 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