RFC: 8154079: Catch incorrectly included .inline.hpp files
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Apr 13 12:38:37 UTC 2016
Hi Per,
On Wed, Apr 13, 2016 at 2:33 PM, Per Liden <per.liden at oracle.com> wrote:
> Hi,
>
> On 2016-04-13 14:07, Thomas Stüfe wrote:
>
>> Hi,
>>
>> I assume this target would not get built for normal hotspot builds, but
>> would have to be built explicitly?
>>
>
> No, I'm thinking this would be part of the default hotspot build target.
> You would of course also be able to do it explicitly if needed.
>
>
Great!
Regards, Thomas
> This of course assumes that it doesn't add a lot to the build time, but
> that shouldn't be a problem as far as I can see.
>
> cheers,
> Per
>
>
>> 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 <mailto: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
>>
>>
>>
>>
More information about the hotspot-dev
mailing list