RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
Lois Foltan
lois.foltan at oracle.com
Tue Apr 29 13:59:55 UTC 2014
On 4/29/2014 9:43 AM, Volker Simonis wrote:
> Hi everybody,
>
> I'm still looking for a sponsor for this change.
Hi Volker,
Can I ask a favor, could you send out your latest webrev. Maybe my
misunderstanding, but I thought this was left off at David's last
comment that ALLOW_OPERATOR_NEW should not be removed after all which
you had removed in your last webrev.
I can sponsor you for JDK 9 but am not a committer for JDK 8 so would
not be able to backport the change.
Thanks,
Lois
>
> 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