RFR(S): 8199698: Change 8199275 breaks template instantiation for xlC (and potentially other compliers)

[coleen.phillimore at oracle.com]

Hi, I've looked at both fixes and I really like Stefan's version. Having 
new and delete to be trivially inlined in allocation.hpp solves a lot of 
problems that I've found when removing more .inline.hpp includes from 
.hpp files (hashtable.inline.hpp from systemDictionary.hpp, I think).

Also, rather than NOINLINE, it's a lot nicer to put this in the .cpp file.

Thanks,
Coleen

On 3/16/18 4:18 AM, Stefan Karlsson wrote:
> Hi Volker,
>
> This seems fine to be.
>
> An alternative fix for the allocation.inline.hpp problem would be to 
> move the AllocateHeap code into allocation.cpp, and get rid of the 
> NOINLINE usage.
>
> I've created a prototype for that:
> http://cr.openjdk.java.net/~stefank/8199698/prototypeAllocateHeapInCpp/
>
> I've visually inspected the output from NMT and it seems to give 
> correct stack traces. For example:
>
> [0x00007f4bffa10eff] ObjectSynchronizer::omAlloc(Thread*)+0x3cf
> [0x00007f4bffa1244c] ObjectSynchronizer::inflate(Thread*, oopDesc*, 
> ObjectSynchronizer::InflateCause)+0x8c
> [0x00007f4bffa1425a] ObjectSynchronizer::FastHashCode(Thread*, 
> oopDesc*)+0x7a
> [0x00007f4bff616142] JVM_IHashCode+0x52
>                              (malloc=4144KB type=Internal #129)
>
> Thanks,
> StefanK
>
> On 2018-03-15 18:20, Volker Simonis wrote:
>> Hi,
>>
>> can I please have a review for the following small fix:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8199698/
>> https://bugs.openjdk.java.net/browse/JDK-8199698
>>
>> The fix is actually trivial: just defining the corresponding
>> "NOINLINE" macro for xlC. Unfortunately it's syntax requirements are a
>> little different for method declarations, compared to the other
>> platforms (it has to be placed AFTER the method declarator instead of
>> BEFORE it). Fortunately, there are no differences for method
>> definitions, so we can safely move the NOINLINE attributes from the
>> method declarations in allocation.hpp to the method definitions in
>> allocation.inline.hpp.
>>
>> Thank you and best regards,
>> Volker
>>
>> PS: or true C++ enthusiasts I've also included the whole story of why
>> this happens and why it happens just now, right after 8199275 :)
>>
>> Change "8199275: Fix inclusions of allocation.inline.hpp" replaced the
>> inclusion of "allocation.inline.hpp" in some .hpp files (e.g.
>> constantPool.hpp) by "allocation.hpp".
>>
>> "allocation.inline.hpp" contains not only the definition of some
>> inline methods (as the name implies) but also the definition of some
>> template methods (notably the various CHeapObj<>::operator new()
>> versions).
>>
>> Template functions are on orthogonal concept with regard to inline
>> functions, but they share on implementation communality: at their call
>> sites, the compiler absolutely needs the corresponding function
>> definition. Otherwise it can either not inline the corresponding
>> function in the case of inline functions or it won't even be able to
>> create the corresponding instantiation in the case of a template
>> function.
>>
>> Because of this reason, template functions and methods are defined in
>> their corresponding .inline.hpp files in HotSpot (even if they are not
>> subject to inlining). This is especially true for the before mentioned
>> CHeapObj<>:: new operators, which are explicitly marked as "NOINLINE"
>> in allocation.hpp but defined in allocation.inline.hpp.
>>
>> Now every call site of these CHeapOb<>::new() operators which only
>> includes "allocation.hpp" will emit a call to the corresponding
>> instantiation of the CHeapObj<>:: new operator, but wont be able to
>> actually create that instantiation (simply because it doesn't see the
>> corresponding definition in allocation.inline.hpp). On the other side,
>> call sites of a CHeapObj<>:: new operator which include
>> allocation.inline.hpp will instantiate the required version in the
>> current compilation unit (or even inline that method instance if it is
>> not flagged as "NOINLINE").
>>
>> If a compiler doesn't honor the "NOINLINE" attribute (or has an empty
>> definition for the NOINLIN macro like xlC), he can potentially inline
>> all the various template instances of CHeapObj<>:: new at all call
>> sites, if their implementation is available. This is exactly what has
>> happened on AIX/xlC before change 8199275 with the effect that the
>> resulting object files contained no single instance of the
>> corresponding new operators.
>>
>> After change 8199275, the template definition of the CHeapObj<>:: new
>> operators aren't available any more at all call sites (because the
>> inclusion of allocation.inline.hpp was removed from some other .hpp
>> files which where included transitively before). As a result, the xlC
>> compiler will emit calls to the corresponding instantiations instead
>> of inlining them. But at all other call sites of the corresponding
>> operators, the operator instantiations are still inlined (because xlC
>> does not support "NOINLINE"), so we end up with link errors in
>> libjvm.so because of missing CHeapObj<>::new instances.
>>
>>   As a general rule of thumb, we should always make template method
>> definitions available at all call sites, by placing them into
>> corresponding .inline.hpp files and including them appropriately.
>> Otherwise, we might end up without the required instantiations at link
>> time.
>>
>> Unfortunately, there's no compile time check to enforce this
>> requirement. But we can misuse the "inline" keyword here, by
>> attributing template functions/methods as "inline". This way, the
>> compiler will warn us, if a template definition isn't available at a
>> specific call site. Of course this trick doesn't work if we
>> specifically want to define template functions/methods which shouldn't
>> be inlined, like in the current case :)
>>