RFR JDK-8021954 VM SIGSEGV during classloading on MacOS; hs_err_pid file produced
Lois Foltan
lois.foltan at oracle.com
Fri Aug 30 04:24:10 PDT 2013
On 8/29/2013 10:22 PM, David Holmes wrote:
> 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.
Hi David,
Yes, you are correct. Coleen & I discussed this yesterday and she
pointed out more of the subtleties behind this. However, we did go
forward with the change mainly because of two reasons. The code's
user-defined operator new()s do not throw C++ exceptions, this should be
explicitly stated to all compilers. Secondly, even in the case of
allocators that do not return NULL, we have relied on C++ compilers to
implicitly generate the constructor prologue code to protect against any
possible situation where they could return NULL.
Thank you for the great discussion on this and your code review,
Lois
>
>>> 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
>>>>>>>>>>
>>>>>>
>>>>
>>
More information about the hotspot-runtime-dev
mailing list