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

Wed Mar 14 13:23:43 UTC 2018

Cool! Thanks a lot,

Volker

On Wed, Mar 14, 2018 at 1:14 PM, Stefan Karlsson
<stefan.karlsson at oracle.com> wrote:
> On 2018-03-14 11:42, Volker Simonis wrote:
>>
>> Ahh, just wanted to send you a mail to ask about the build failures of
>> my submit-hs job on Solaris which I can't reproduce on our local
>> machines :)
>>
>> Yes, please go ahead and push my change. I'm sure I'll find another
>> one which I can finally push myself :)
>
>
> :) I've pushed the change now.
>
> StefanK
>
>
>>
>> Thanks,
>> Volker
>>
>>
>> On Wed, Mar 14, 2018 at 10:52 AM, Stefan Karlsson
>> <stefan.karlsson at oracle.com> wrote:
>>>
>>> Hi Volker,
>>>
>>> On 2018-03-13 18:13, 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.
>>>
>>>
>>>
>>> The submit job failed because of missing handles.inline.hpp includes in
>>> our
>>> closed JFR code. I've created closed patch to solve that. I can push both
>>> of
>>> these patches, unless you really want to push the open part yourself.
>>>
>>> Thanks,
>>> StefanK
>>>
>>>
>>>>
>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>