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

[Stefan Karlsson]

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?

Thanks,
StefanK