RFR(S/M): 8199472: Fix non-PCH build after JDK-8199319
stefan.karlsson at oracle.com
Tue Mar 13 09:16:19 UTC 2018
On 2018-03-13 10:12, Volker Simonis wrote:
> Hi Coleen, Stefan,
> sure I'm open for suggestions :)
> As you both ask for the same thing, I'll prepare a new webrev with
> allocate_instance_handle moved to instanceKlass.cpp. In my initial
> patch I just didn't wanted to change the current inlining behaviour
> but if you both think that allocate_instance_handle is not performance
> critical I'm happy to clean that up.
I don't think it's critical to get it inlined. With that said, I think
the compiler will inline allocate_instance into
allocate_instance_handle, so you'll most likely only get one call anyway.
> With the brand new submit-hs repo posted by Jesper just a few hours
> ago, I'll be also able to push this myself, so no more need for a
> sponsor :)
> On Mon, Mar 12, 2018 at 8:42 PM, <coleen.phillimore at oracle.com> wrote:
>> Hi this looks good except:
>> Can you move this a function in instanceKlass.cpp and would this eliminate
>> the changes that add include instanceKlass.inline.hpp ?
>> If Stefan is not still online, I'll sponsor this for you.
>> I have a follow-on related change
>> https://bugs.openjdk.java.net/browse/JDK-8199263 which is quickly expanding
>> due to transitive includes that I hope you can help me test out (when I get
>> it to compile on solaris).
>> On 3/12/18 3:34 PM, Volker Simonis wrote:
>>> can I please have a review and a sponsor for the following fix:
>>> The number changes files is "M" but the fix is actually "S" :)
>>> Here come the gory details:
>>> Change "8199319: Remove handles.inline.hpp include from
>>> reflectionUtils.hpp" breaks the non-PCH build (at least on Ubuntu
>>> 16.04 with gcc 5.4.0). If you configure with
>>> "--disable-precompiled-headers" you will get a whole lot of undefined
>>> reference for "Handle::Handle(Thread*, oopDesc*)" - see bug report.
>>> It seems that newer versions of GCC (and possibly other compilers as
>>> well) don't emit any code for inline functions if these functions can
>>> be inlined at all potential call sites.
>>> The problem in this special case is that "Handle::Handle(Thread*,
>>> oopDesc*)" is not declared "inline" in "handles.hpp", but its
>>> definition in "handles.inline.hpp" is declared "inline". This leads to
>>> a situation, where compilation units which only include "handles.hpp"
>>> will emit a call to "Handle::Handle(Thread*, oopDesc*)" while
>>> compilation units which include "handles.inline.hpp" will try to
>>> inline "Handle::Handle(Thread*, oopDesc*)". If all the inlining
>>> attempts are successful, no instance of "Handle::Handle(Thread*,
>>> oopDesc*)" will be generated in any of the object files. This will
>>> lead to the link errors listed in the .
>>> The quick fix for this issue is to include "handles.inline.hpp" into
>>> all the compilation units with undefined references (listed below).
>>> The correct fix (realized in this RFR) is to declare
>>> "Handle::Handle(Thread*, oopDesc*)" inline in "handles.hpp". This will
>>> lead to warnings (which are treated as errors) if the inline
>>> definition is not available at a call site and will avoid linking
>>> error due to compiler optimizations. Unfortunately this requires a
>>> whole lot of follow-up changes, because "handles.hpp" defines some
>>> derived classes of "Handle" which all have implicitly inline
>>> constructors which all reference the base class
>>> "Handle::Handle(Thread*, oopDesc*)" constructor. So the constructors
>>> of the derived classes have to be explicitly declared inline in
>>> "handles.hpp" and their implementation has to be moved to
>>> "handles.inline.hpp". This change again triggers other changes for all
>>> files which relayed on the derived Handle classes having inline
>>> Thank you and best regards,
More information about the hotspot-dev