RFR(S): 8039805: Fix the signature of the global new/delete operators in allocation.cpp

[Volker Simonis]

Hi Lois,

thanks for the review. Please find my comments inline:

On Thu, Apr 10, 2014 at 3:34 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
> Hi Volker,
>
> Overall I understand the motivation for this change but have some concerns.
>
> make/aix/makefiles/vm.make
> No comments.
>
> make/bsd/makefiles/vm.make
> 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.
>
> src/os/aiz/vm/os_aix.cpp
> No comments.
>
> src/os/aix/vm/porting_aix.cpp
> No comments.
>
> src/share/vm/memory/allocation.cpp
> 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".

> 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
> confusion.

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.

> 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?

Regards,
Volker

> Thanks,
> Lois
>
>
>
> On 4/9/2014 12:34 PM, Volker Simonis wrote:
>>
>> Hi,
>>
>> could you please review and sponsor the following small change:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/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).
>>
>> In the new C++11/14 standard
>> (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).
>>
>> 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://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
>
>