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

Wed Apr 13 08:43:58 UTC 2016

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