RFR JDK-8021954 VM SIGSEGV during classloading on MacOS; hs_err_pid file produced

[David Holmes]

On 29/08/2013 1:53 AM, Coleen Phillimore wrote:
> On 8/27/2013 8:46 PM, David Holmes wrote:
>> Thanks for the detailed explanation. It is frustrating that we keep
>> having to revisit these allocation routines.
>>
>> However it still seems to me that we don't need NO_EXCEPT on most of
>> our operator new definitions because they will never return NULL. This
>> is trivially so for things like:
>>
>>  void* StackObj::operator new(size_t size)     NOEXCEPT {
>> ShouldNotCallThis(); return 0; }
>> void* _ValueObj::operator new(size_t size)    NOEXCEPT {
>> ShouldNotCallThis(); return 0; }
>>
>> but also for anything that invokes vm_exit_out_of_memory directly or
>> indirectly.
>
> I don't think this is a good idea at all.   We want the constructor
> prologue to always check for null before calling the constructor. We
> will never throw C++ exceptions for these so all operator new's are
> NOEXCEPT.   These trivial examples would return zero rather than
> throwing if ShouldNotCallThis() is ifdef'ed out in product.   So, this
> example is either trivially wrong without NOEXCEPT or correct without
> NOEXCEPT.   It is safer code to make them all NOEXCEPT.

If the allocator can never return a NULL value then checking for NULL is 
completely redundant. We don't check our internal allocation routines 
for NULL returns where we know they abort on failure, so how is this any 
different ? AFAICS ShouldNotCallThis() is not ifdef'd out but if it were 
then yes this would fail - as would ifdef'ing out a 
vm_exit_out_of_memory in the other allocation routines.

> So really the only question is whether NOEXCEPT is better than
> throw().    throw() looks nicer because it's not a macro.  NOEXCEPT may
> be required for the future if that's required to conditionally add the
> noexcept keyword.
>
> Maybe we should use throw() for now and worry about the noexcept keyword
> if that becomes an issue.

It would be less ugly to use throw() but more intrusive in that it 
affects all the compilers. Plus I don't know what the benefit would be 
of moving from throw() to noexcept if they both mean the same thing?

> David, where are you removing spurious throw() expressions and why?

./share/vm/code/nmethod.cpp:void* nmethod::operator new(size_t size, int 
nmethod_size) throw () {

See:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2013-August/010526.html

and subsequent replies. The presence of throw() was considered an error 
at the time but now it seems not. I don't know if it has been removed in 
hotspot-compiler, or perhaps just in the staging repos for the ppc64 
changes.

Cheers,
David
------



> Thanks,
> Coleen
>
>>
>> David
>> -----
>>
>> On 27/08/2013 10:44 PM, Lois Foltan wrote:
>>>
>>> On 8/27/2013 7:31 AM, David Holmes wrote:
>>>> On 27/08/2013 6:01 PM, Mikael Gerdin wrote:
>>>>> On 2013-08-27 04:41, David Holmes wrote:
>>>>>> On 27/08/2013 4:36 AM, Lois Foltan wrote:
>>>>>>> Please review the following fix:
>>>>>>>
>>>>>>> Internal webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~coleenp/bug_jdk8021954/
>>>>>>>
>>>>>>> Bug: VM SIGSEGV during classloading on MacOS; hs_err_pid file
>>>>>>> produced &
>>>>>>>           runtime/6878713/Test6878713.sh fails on mac
>>>>>>>
>>>>>>>      bug links at: https://bugs.openjdk.java.net/browse/JDK-8021954
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8022140
>>>>>>>
>>>>>>> Summary of fix:
>>>>>>>      On MacOS, currently Hotspot is built specifying the -fcheck-new
>>>>>>> command line option to the llvm-g++ compiler.
>>>>>>>      The -fcheck-new option directs the compiler to "check that the
>>>>>>> pointer returned by |operator new| is non-null
>>>>>>>       before attempting to modify the storage allocated." The
>>>>>>> clang++
>>>>>>> compiler does not support the
>>>>>>>       -fcheck-new option.  To obtain similiar functionality when
>>>>>>> building Hotspot with clang++, empty exception
>>>>>>>       throw() specifications must be added to all user-defined
>>>>>>> operator
>>>>>>> new()'s.
>>>>>>
>>>>>> We just spotted something related in the PPC64 port and were going
>>>>>> the
>>>>>> other way or removing these "spurious" throw() declarations.
>>>>>>
>>>>>> But this seems really ugly - is there really a need to specialize
>>>>>> this
>>>>>> for clang and use NOEXCEPT to hide it? Shouldn't all C++ compilers
>>>>>> honour the nothrow() semantics?
>>>>>>
>>>>>> That said I thought we already handled this using the "const
>>>>>> std::nothrow_t&  nothrow_constant" where needed? Our other
>>>>>> operator new
>>>>>> implementations shouldn't return NULL but will abort.
>>>>>>
>>>>>> So why do we need -fcheck-new or this empty throw() ?
>>>>>
>>>>> I guess that if the C++ compiler knows that operator new() will
>>>>> throw an
>>>>> exception if an allocation fails then it doesn't need to emit code to
>>>>> explicitly avoid calling the constructor.
>>>>
>>>> Yes that is what happens, but why do _we_ need it when ...
>>>>
>>>>> If we declare operator new() with an empty throw() we will also inform
>>>>> the compiler that new can't throw an exception and therefore the
>>>>> compiler needs to explicitly handle failed allocations.
>>>>
>>>> ... we have two kinds of operator new implementations (or at least we
>>>> _should_ only have two kinds):
>>>>
>>>> 1. Will abort on OOM (either ResourceArea exhaustion or Stack
>>>> exhaustion) and so never returns NULL
>>>> 2. Can return NULL and uses std::nothrow to indicate that
>>>>
>>>> So we should not need this if the nothrow is acting as I think it
>>>> should. Have we missed places where nothrow is needed? Do we have
>>>> errant operator new definitions unrelated to the allocation base
>>>> classes?
>>> Hi David,
>>>
>>> I think there may be some confusion about what std::nothrow_t is
>>> actually doing.  It provides a mechanism to overload operator new &
>>> delete in order to direct the compiler to prefer a function definition
>>> that also should contain an empty "throw()" specification.  Take for
>>> example the standard definition of the global ::operator new(). It
>>> actually looks like:
>>>
>>>   * void * operator new <a00960.html#a1414bcdb34c39ce82fc84bc8d5287966>
>>>     (std::size_t) throw (std::bad_alloc)
>>>   * void * operator new <a00960.html#a940b606c7824b4d5dd121199c277d629>
>>>     (std::size_t, const std::nothrow_t &) throw ()
>>>
>>> When the std::nothrow_t parameter is passed to the global ::operator
>>> new() the invoked function is the one that also contains an empty
>>> throw() specification.  It is due to this empty throw() specification
>>> that directs or triggers the C++ compiler to generate the detection of
>>> NULL constructor prologue.   Specifying only the std::nothrow_t
>>> parameter to a class member user-defined ::operator new() does not
>>> indicate to a C++ compiler that the function does not throw an
>>> exception.  According to the c++98 standard, a function that will throw
>>> no exceptions should be delcared with an empty "throw()" list
>>> specification.  When first experimenting with a fix, I did just try to
>>> overload Hotspot's user-defined operator new() with the std:nothrow_t
>>> parameter.  As expected, the mere presence of this parameter did not
>>> trigger the clang++ compiler to generate NULL detection constructor
>>> prologue code.  Unfortunately, probably due to legacy code, some C++
>>> compilers behave differently.  For example, I did confirm with Solaris
>>> C++ development, the Solaris C++ by default always generates constructor
>>> prologue code that detects NULL for class member user-defined operator
>>> new() even in a throw situation. G++ provides the -fcheck-new command
>>> line option.  It is due to these differences of behavior or Hotspot's
>>> build specification of -fcheck-new that has led us to believe that
>>> std::nothrow_t was providing something that it is not.
>>>
>>> That said, I would also prefer what you propose, deciding on two kinds
>>> of operator new()'s that can be defined.  I did examine all of the
>>> user-defined operator new()'s within the code and I determined that only
>>> a very small number possibly could not return NULL.  Given this,
>>> factored in with the historical reliance on -fcheck-new for the g++
>>> compilers, I decided the safe route for JDK 8 was to introduce the
>>> NOEXCEPT macro.
>>>
>>> Lois
>>>>
>>>> David
>>>> -----
>>>>
>>>>> The g++ man page states:
>>>>>
>>>>>         -fcheck-new
>>>>>             Check that the pointer returned by "operator new" is
>>>>> non-null before attempting to modify the storage allocated. This check
>>>>> is normally unnecessary because the C++ standard specifies that
>>>>>             "operator new" will only return 0 if it is declared
>>>>> throw(),
>>>>> in which case the compiler will always check the return value even
>>>>> without this option.  In all other cases, when "operator new"
>>>>>             has a non-empty exception specification, memory exhaustion
>>>>> is signalled by throwing "std::bad_alloc".  See also new (nothrow).
>>>>>
>>>>> /Mikael
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Tests:
>>>>>>>
>>>>>>>       Solaris: built fastdebug & product images
>>>>>>>       Linux: built fastdebug & product images
>>>>>>>       MacOS:  built fastdebug & product images using llvm-g++ - ran
>>>>>>> JTREG
>>>>>>>                      built fastdebug & product images using
>>>>>>> clang++ -
>>>>>>> ran JTREG, JCK vm & lang, vm.quick.testlist (in progress)
>>>>>>>       Windows:  built fastdebug & product images with VS2010
>>>>>>>
>>>
>