RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
Volker Simonis
volker.simonis at gmail.com
Wed Apr 30 16:44:11 UTC 2014
On Wed, Apr 30, 2014 at 3:33 PM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> 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
Hi Severin,
could you please post the stack trace of the assertion to see where
the operators are called from.
> 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?
>
I must confess that I don't know exactly how the Shark build works,
but if you load llvm as a shared library the llvm code shouldn't even
see the global HotSpot new/delete operators because we don't export
them and we compile with -fvisibility=hidden. Looking at the created
libjvm.so, this is confirmed:
00000000003d4442 t operator new[](unsigned long)
00000000003d44b4 t operator new[](unsigned long, std::nothrow_t&)
00000000003d440b t operator new(unsigned long)
000000000038a986 t operator new(unsigned long, void*)
00000000003d4479 t operator new(unsigned long, std::nothrow_t const&)
My only explanation for the problems you see would be if you compile
the llvm code right into the libjvm.so. If that's the case you should
either consider to build the llvm into it's own library or you should
flag the global operators in allocation.cpp with #ifndef SHARK".
By the way, I also couldn't find a place n the current makefiles where
ALLOW_OPERATOR_NEW_USAGE is defined for a SHARK build. Where do you
currently set that define?
Regards,
Volker
> 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