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

[Per Liden]

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?

cheers,
Per

>
> Thanks,
> StefanK