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

[Lois Foltan]

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> 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!
Understood.

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

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

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.
Thanks,
Lois

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