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

[David Holmes]

On 29/08/2013 11:15 PM, Lois Foltan wrote:
>
> On 8/29/2013 7:13 AM, David Holmes wrote:
>> 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.
> Hi David,
> We do have allocators that can return a NULL value, like
> MetaSpace::allocate(), Chunk's operator new(), etc.  Both demonstrating
> the return of NULL in the two bug reports.

Please re-read what I said. I know we do have allocators that return 
NULL. We also have allocators that DO NOT return NULL. The NULL check is 
only needed on the former, not the latter.

>> We don't check our internal allocation routines for NULL returns where
>> we know they abort on failure, so how is this any different ?
> Actually we have been checking NULL, it's just that the C++ compiler's
> have been implicitly checking NULL for us.  Solaris C++ generates by
> default NULL detection constructor prologue code for user-defined class
> member operator new() even in a throw situation. For gcc we specify
> -fcheck-new.  So whether or not we explicitly direct the compiler to
> check via an empty "throw()" specification or not, it's still being
> done.  So why should we check now?  Because we have a C++ compiler,
> clang, that adheres to the standard and doesn't support -fcheck-new.  It
> wants to be explicitly told.

See above.

Thanks,
David
------

>> 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?
> I believe the c++11 standard obsolesces "throw()" in favor of noexcept
> keyword.  However, I can't imagine that a compiler would completely
> obsolesce it, I can image there will be a mode to still support "throw()".
>
> Lois
>>
>>> 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
>>>>>>>>>
>>>>>
>>>
>