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

[Stefan Karlsson]

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