RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
David Holmes
david.holmes at oracle.com
Wed Apr 30 09:58:50 UTC 2014
On 30/04/2014 3:24 AM, Volker Simonis wrote:
> Hi Lois,
>
> sure, the latest webrev was:
>
> http://cr.openjdk.java.net/~simonis/webrevs/8039805.v3/
>
> As far as I understood, David recalled his concerns in his last mail
> ("..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.." - see
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-April/013523.html)
>
> @David: are you OK with the change now?
Yes. To clarify for Lois my "Ignore that" retracted my comment that
everything in the "#ifndef ALLOW_OPERATOR_NEW_USAGE" block could be
deleted. I'm okay with getting rid of ALLOW_OPERATOR_NEW_USAGE itself.
Thanks,
David
> Thanks,
> Volker
>
> PS: jdk9 is just fine for now - thanks for helping out!
>
>
> On Tue, Apr 29, 2014 at 3:59 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>>
>> 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