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 17:24:43 UTC 2014
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?
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