RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
Severin Gehwolf
sgehwolf at redhat.com
Wed Apr 30 13:33:05 UTC 2014
Hi Volker,
On Wed, 2014-04-30 at 19:58 +1000, 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.
I believe removing the #ifndef ALLOW_OPERATOR_NEW_USAGE will break the
shark JVM variant. Currently shark will need some more work to be
useful, but a slowdebug build of shark doesn't even get past "java
-version" due to those assertions. Defining it for a shark build + one
other small fix will make "java -version" work.
Take away: Shark code uses llvm and that in turn calls those global
new/delete operators in spades. How do you suggest to work around this
with this ifndef removed. Thoughts?
Thanks,
Severin
> 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