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

Volker Simonis volker.simonis at gmail.com
Wed Mar 14 10:30:02 UTC 2018

On Tue, Mar 13, 2018 at 10:47 PM, <coleen.phillimore at oracle.com> wrote:

This looks good to me too. Thanks, Coleen

Thanks!

ps. can you test out my patch on ppc and the others for 8199263: Split interfaceSupport.hpp to not require including .inline.hpp files http://cr.openjdk.java.net/~coleenp/8199263.02/webrev/index.html

Sure, I'll do it today and let you know...

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 allocateinstancehandle 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 allocateinstancehandle 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 allocateinstancehandle 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 allocateinstance into allocateinstancehandle, 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