RFC: 8154079: Catch incorrectly included .inline.hpp files
Per Liden
per.liden at oracle.com
Wed Apr 13 12:33:15 UTC 2016
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.
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