RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp
volker.simonis at gmail.com
Fri Apr 11 10:30:03 UTC 2014
sorry, the last webrev accidentally contained some additional, unrelated
makefile changes which fix another problem. Here's the clean version:
On Fri, Apr 11, 2014 at 12:05 AM, Lois Foltan <lois.foltan at oracle.com>wrote:
> On 4/10/2014 2:04 PM, Volker Simonis wrote:
> Hi Lois,
> thanks for the review. Please find my comments inline:
> Hi Volker,
> My comments are inlined below as well.
> On Thu, Apr 10, 2014 at 3:34 PM, Lois Foltan <lois.foltan at oracle.com> <lois.foltan at oracle.com> wrote:
> Hi Volker,
> Overall I understand the motivation for this change but have some concerns.
> No comments.
> No comments, but I do share Vladimir Kozlov's concern that this change to
> remove the definition of ALLOW_OPERATOR_NEW_USAGE may need to be considered
> further by the runtime group.
> No comments.
> No comments.
> I would prefer not to remove the empty "throw()" specifications. For
> But removing the empty "throw()" specifications is actually the only
> reason for this change!
> Some compilers simply can not override the global new operators if the
> user provided operators have a different exception specification. And
> as I wrote, specifying "throw()" means that the operator won't throw
> any exception which is actually quite the opposit of the standard new
> operators which have "throw(std::bad_alloc)" and which means that they
> can throw "std::bad_alloc".
> This is unfortunate for two reasons, the JVM really doesn't want any new
> operators whether user or globally defined to throw exceptions and secondly
> most compilers (g++, clang, Solaris C++, Visual C++) can override the
> global new operators.
> consistency reasons, any user-defined operator new within the JVM should be
> defined with the empty "throw()" specification in order to ensure that the
> issues encountered within JDK-8021954 do not resurface. I think this type
> of consistency is important whether in production or debug mode to avoid
> There is really no need for consistency here - especially if we only
> insist on consistency as an end in itself.
> The two global new allocators which I've changed are there for error
> checking only. They should never be called and if they are called they
> will never return because they'll shut down the VM immediately. So
> there's absolutely no need to do any checks on their return values.
> Again, I understand and since you are correct that this is a situation
> where these definitions of global new operators should never be called, I
> am okay with you going ahead with the change. I would just like to go on
> record as stating that I think consistency is good in this case because the
> issue within JDK-8021954 was actually quite subtle. Without indicating to
> the clang compiler that a user defined operator new would not throw an
> exception, needed internally generated C++ constructor prolog code that the
> JVM relies on, was not being generated by default.
> We are still a long way off before adopting the C++11/14
> standard. The intent was that in the future, when C++11 is adopted, all
> "throw()" specifications would be changed to use the "nothrow" keyword.
> Since JDK-8021954 addressed issues with the clang++ compiler on MacOS, I am
> curious to know if you completed any testing of the JVM built with clang++?
> I've just build with clang as well and run the jdk regression test
> suite without any special issues (besides jdk/sun/misc/CopyMemory.java
> which crashes. But it crashes with the global new operators from
> allocation.cpp as well as without. And it doesn't crash if compiled
> with g++, so that seems a different problem) .
> Do you have any special test in mind which I could run to see the
> problems you are afraid off?
> Thank you for completing this testing. I think the original test cases
> in JDK-8021954 and JDK-8022140 were in the hotspot/test/runtime area as
> well, so it would be good to run those tests.
Done. The hotspot/test/runtime tests run without any regressions compared
to the original version (they revealed that -XX:+UseMallocOnly is currently
not working for HS 'optimized' builds but that's again another problem).
> The issue with jdk/sun/misc/CopyMemory.java does concern me but I agree
> this seems to be a separate issue. There was a MacOS clang C++ compiler
> optimization issue, see JDK-8022407<https://bugs.openjdk.java.net/browse/JDK-8022407>,
> that was fixed in JDK 8. What version of Xcode are you compiling with?
You're right, this is indeed
It seems I'm using an older compiler (I'm actually not very familiar with
MacOS X - just have ssh access to a server box).
$ xcodebuild -version
Build version 4F1003
$ clang++ --version
Apple clang version 4.0 (tags/Apple/clang-421.0.60) (based on LLVM 3.1svn)
Thread model: posix
Which is not covered by your patch. If I extend it to also cover clang 4.0,
the test succeeds. I'll let it up to you if you want to include this small
change (it is currently not in the webrev) but I personally think it would
be a good idea. Considering the fact that it happens with Xcode 4.4.1 and
Xcode 4.6.2, but not with Xcode 4.5.2 may also justify to unconditionally
compile unsafe.cpp with -O1 only.
diff -r a694dbf1c497 make/bsd/makefiles/gcc.make
--- a/make/bsd/makefiles/gcc.make Wed Apr 09 17:39:43 2014 +0200
+++ b/make/bsd/makefiles/gcc.make Fri Apr 11 12:23:08 2014 +0200
@@ -313,7 +313,7 @@
# Work around some compiler bugs.
ifeq ($(USE_CLANG), true)
- ifeq ($(shell expr $(CC_VER_MAJOR) = 4 \& $(CC_VER_MINOR) = 2), 1)
+ ifeq ($(shell expr $(CC_VER_MAJOR) = 4 \& $(CC_VER_MINOR) = 2 \|
$(CC_VER_MAJOR) = 4 \& $(CC_VER_MINOR) = 0), 1)
OPT_CFLAGS/loopTransform.o += $(OPT_CFLAGS/NOOPT)
OPT_CFLAGS/unsafe.o += -O1
One final comment, since you are modifying make files I think you also
> should sent the RFR to the build-dev at openjdk.java.net distro as well.
My last webrev contained some unrelated makefile changes. I think the
current changes are quite trivial - they just remove the ALLOW_OPERATOR_NEW
Lois, can you please sponsor this change and run it trough JPRT?
Thank you and best regards,
> On 4/9/2014 12:34 PM, Volker Simonis wrote:
> could you please review and sponsor the following small change:
> which fixes the signature of the global new/delete operators in
> 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')
> 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
> 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++ standardhttp://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1905.pdf).
> In the new C++11/14 standard
> 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).
> 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"
> 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,
More information about the hotspot-dev