RFR(S/M): 8199472: Fix non-PCH build after JDK-8199319

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue Mar 13 21:47:40 UTC 2018

This looks good to me too.

ps. can you test out my patch on ppc and the others for 8199263: Split 
interfaceSupport.hpp to not require including .inline.hpp files

On 3/13/18 1:13 PM, Volker Simonis wrote:
> Hi,
> please find the new webrev here:
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8199472.v2/
> I've moved allocate_instance_handle to instanceKlass.cpp as requested
> and updated some copyrights. The change is currently running through
> the new submit-hs repo testing.
> If you're OK with the new version and the tests succeed I'll push the
> change tomorrow.
> Best regards,
> Volker
> On Tue, Mar 13, 2018 at 10:16 AM, Stefan Karlsson
> <stefan.karlsson at oracle.com> wrote:
>> Hi Volker,
>> 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 :)
>> Yay!
>> StefanK
>>> Thanks,
>>> Volker
>>> On Mon, Mar 12, 2018 at 8:42 PM,  <coleen.phillimore at oracle.com> wrote:
>>>> Hi this looks good except:
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8199472/src/hotspot/share/oops/instanceKlass.inline.hpp.udiff.html
>>>> 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).
>>>> Thanks,
>>>> Coleen
>>>> On 3/12/18 3:34 PM, Volker Simonis wrote:
>>>>> Hi,
>>>>> can I please have a review and a sponsor for the following fix:
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8199472/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8199472
>>>>> 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
>>>>> constructors...
>>>>> Thank you and best regards,
>>>>> Volker

More information about the hotspot-dev mailing list