RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
Lois Foltan
lois.foltan at oracle.com
Thu May 1 14:26:13 UTC 2014
On 4/30/2014 5:58 AM, David Holmes wrote:
> 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.
Thank you David for the clarification, just wanted to make sure all
review comments were covered!
Lois
>
> 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