RFC: 8154079: Catch incorrectly included .inline.hpp files

Wed Apr 13 12:07:00 UTC 2016

Hi,

I assume this target would not get built for normal hotspot builds, but
would have to be built explicitly?

If yes, could we have this somehow be a part of hg jcheck?

Kind Regards, Thomas

On Wed, Apr 13, 2016 at 10:43 AM, Stefan Karlsson <
stefan.karlsson at oracle.com> wrote:

> Hi Per,
>
>
> On 2016-04-13 10:34, Per Liden wrote:
>
>> Hi Stefan,
>>
>> On 2016-04-12 18:40, Stefan Karlsson wrote:
>>
>>> Hi all,
>>>
>>> I would like to propose a patch to make it easier to find and clean up
>>> places where we include .inline.hpp files from .hpp files. So, that we
>>> start getting smaller include dependencies, with lower risk of circular
>>> include dependencies, and maybe even shorter compile times.
>>>
>>> The guidelines regarding file inclusions can be found at:
>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>>>
>>> "Files
>>>
>>>      Do not put non-trivial function implementations in .hpp files. If
>>> the implementation depends on other .hpp files, put it in a .cpp or a
>>> .inline.hpp file.
>>>      .inline.hpp files should only be included in .cpp or .inline.hpp
>>> files.
>>>      All .cpp files include precompiled.hpp as the first include line.
>>>      precompiled.hpp is just a build time optimization, so don't rely on
>>> it to resolve include problems.
>>>      Keep the include lines sorted.
>>>      Put conditional inclusions (#if ...) at the end of the include
>>> list."
>>>
>>> The code to enable the stricter .inline.hpp include check can be found
>>> in this small patch:
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.00.addInlineHppGuard/
>>>
>>> I'm using the fact that we are including almost all header files in the
>>> precompiled.hpp file. So, if I add a "scoped" define, called
>>> INLINE_HPP_GUARD,  in preompiled.hpp and add checks for this define in
>>> .inline.hpp files, the preprocessor will tell me when .inline.hpp
>>> includes come from .hpp files (directly or indirectly) rather than .cpp
>>> files. This requires that the .hpp file is reachable through
>>> precompiled.hpp and that we start remove .inline.hpp files from
>>> precompiled.hpp.
>>>
>>> I've tried this on a few .inline.hpp files. For example:
>>> thread.inline.hpp:
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.01.guardThreadInlineHpp
>>>
>>>
>>> The inlineHppGuard.hpp file is first included in thread.inline.hpp:
>>>
>>>   #include "runtime/atomic.inline.hpp"
>>>   #include "runtime/os.inline.hpp"
>>>   #include "runtime/thread.hpp"
>>> +#include "utilities/inlineHppGuard.hpp"
>>>
>>> Then when I compile (with precompiled headers enabled) I get:
>>> In file included from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/runtime/thread.inline.hpp:33:0,
>>>
>>>
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/runtime/handles.inline.hpp:29,
>>>
>>>
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/runtime/reflectionUtils.hpp:32,
>>>
>>>
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/classfile/systemDictionary.hpp:34,
>>>
>>>
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/ci/ciEnv.hpp:30,
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/ci/ciUtilities.hpp:28,
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/ci/ciNullObject.hpp:30,
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/ci/ciConstant.hpp:29,
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/ci/ciArray.hpp:29,
>>>                   from
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/precompiled/precompiled.hpp:37:
>>>
>>>
>>> /home/stefank/hg/jdk9/hs-rt/hotspot/src/share/vm/utilities/inlineHppGuard.hpp:44:2:
>>>
>>> error: #error ".inline.hpp file incorrectly included from .hpp file"
>>>
>>> That tells me that I probably need to go and fix the inclusion of
>>> reflectionUtils.hpp -> handles.inline.hpp first. It's not always enough
>>> to remove the inclusion of the .inline.hpp file, because the header
>>> might actually use the inline files. In those cases the code needs to be
>>> restructured so that we the "offending" functions out from the .hpp
>>> files to .cpp or .linline.hpp files. This might be tedious in the
>>> beginning, but will hopefully become easier to maintain when when more
>>> of these files get cleaned up.
>>>
>>> So, with this in place, after all incorrect includes have been fixed,
>>> whenever someone "incorrectly" adds an inclusion of thread.inline.hpp
>>> (directly or indirectly) to a .hpp file the preprocessor will complain.
>>>
>>> I have a patch set where I've tried this on different .inline.hpp files,
>>> and the following patches show the kind of work that is needed to fix
>>> the includes:
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.00.addInlineHppGuard/
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.01.guardThreadInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.02.guardHandleInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.03.guardOsInlineHpp/
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.04.guardMarkOopInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.05.guardOopInlineHpp/
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.06.guardFrameInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.07.guardHashtableInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.08.guardOrderAccessInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.09.guardBitMapInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.10.guardAtomicInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.11.guardKlassInlineHpp/
>>>
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.12.guardTypeArrayOopInlineHpp/
>>>
>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.01.all/
>>>
>>> It compiles on linux-x64 with and without precompiled headers, but I
>>> haven't taken the time to try to get it to compile on other platforms.
>>>
>>> So, is this worth doing to start fixing our include mess?
>>>
>>
>> +1, this looks like a nice way of gradually moving in the right direction.
>>
>> It would be nice to get these errors also when precompiled headers is
>> disabled. Instead of doing this through precompiled.hpp, how about adding a
>> make target which creates a temporary "all.hpp" that #includes all our .hpp
>> files (.inline.hpp excluded) and just runs the pre-processor on that file?
>>
>
> Sounds like a good idea to me!
>
> Thanks,
> StefanK
>
>
>> cheers,
>> Per
>>
>>
>>> Thanks,
>>> StefanK
>>>
>>
>