RFR(M): Fix non-PCH build on Linux, Windows and MacOS X
Stefan Karlsson
stefan.karlsson at oracle.com
Tue Feb 26 01:03:28 PST 2013
Volker,
Thanks for fixing this. See inlined:
On 02/25/2013 07:09 PM, Volker Simonis wrote:
> Recent changes have broken the no-PCH build again.
>
> The fix requires some bigger changes this time but before going into
> the details - PLEASE, PLEASE add a no-PCH build to your JPRT (or
> nightly build or whatever) system to catch such errors. On platforms
> which don't support PCH (i.e. Solaris) the normal build 'naturally'
> works and I would consider it 'good practice' for a project to be able
> to build without such an esoteric:) feature like precompiled headers.
> Actually this only means that all the dependencies of a compilation
> unit are explicitly recorded in that compilation unit (which I'd
> consider self-evident).
>
> Tested 'product' and 'jvmg' builds on Linux/x86_64, Linux/x86_32,
> Windows/x86_64, Solaris/Sparc_64, Solaris/Sparc_32, and MacOS
> X/x86_64.
>
> Please be so kind to open a BugID for this issue and review the change:
>
> http://cr.openjdk.java.net/~simonis/webrevs/fix_nonPCH_build/
BugID created: JDK-8008959.
>
> Regards,
> Volker
>
> PS: following some details of this change:
>
> The following places were easy to fix (just include the appropriate
> ".inline.hpp" files):
>
> src/share/vm/oops/symbol.hpp
I think we should strive to avoid including .inline.hpp files in .hpp
files. The inline.hpp files tend to bring a lot of extra dependencies
that are exposed to the includers of the .hpp file.
Could you move Symbol::increment_refcount() and
Symbol::decrement_refcount into a symbol.inline.hpp file instead? This
will have some ripple effects, but I think it's worth it.
Otherwise, this looks good to me.
thanks,
StefanK
>
> - uses Atomic inline functions (inc() and dec()) without including
> "runtime/atomic.inline.hpp"
>
> src/share/vm/memory/allocation.inline.hpp
>
> - uses Atomic inline functions (load() and store()) without including
> "runtime/atomic.inline.hpp"
>
> src/os/windows/vm/decoder_windows.cpp
>
> - uses Arguments::get_java_home() without including "runtime/arguments.hpp"
>
> src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
> src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp
> src/os_cpu/solaris_x86/vm/orderAccess_solaris_x86.inline.hpp
> src/os_cpu/solaris_sparc/vm/orderAccess_solaris_sparc.inline.hpp
> src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp
>
> - use the Atomic inline functions (load() and store()) without
> including "runtime/atomic.inline.hpp"
>
> src/os_cpu/bsd_x86/vm/atomic_bsd_x86.inline.hpp
> src/os_cpu/linux_x86/vm/atomic_linux_x86.inline.hpp
> src/os_cpu/linux_zero/vm/atomic_linux_zero.inline.hpp
> src/os_cpu/linux_sparc/vm/atomic_linux_sparc.inline.hpp
> src/os_cpu/windows_x86/vm/atomic_windows_x86.inline.hpp
> src/os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp
> src/os_cpu/solaris_sparc/vm/atomic_solaris_sparc.inline.hpp
> src/os_cpu/solaris_x86/vm/atomic_solaris_x86.inline.hpp
>
> - include "orderAccess_<os>_<cpu>.inline.hpp" without using any
> OrderAccess method. I removed this includes to avoid a cyclic
> dependency between "orderAccess_<os>_<cpu>.inline.hpp" and
> "atomic_<os>_<cpu>.inline.hpp" via "atomic.inline.hpp"
>
> src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
> src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp
> src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp
>
> - use os::is_MP() without including os.hpp
>
> The following fix was a little more cumbersome:
>
> src/os/bsd/vm/os_bsd.hpp
> src/os/linux/vm/os_linux.hpp
>
> - The methods "set_suspended()" and "clear_suspended()" use
> Atomic::cmpxchg without "atomic.inline.hpp" being included.
> Unfortunately "os_bsd.hpp" and "os_linux.hpp" are not self-contained
> files. They are themselves included into "os.hpp" right into the class
> definition of the "os" class. It is therefore not possible to simply
> include "atomic.inline.hpp" into "os_bsd.hpp" and "os_linux.hpp".
>
> - The natural solution was to transfer the implementations of
> "set_suspended()" and "clear_suspended()" to the corresponding
> "os_bsd.inline.hpp" and "os_linux.inline.hpp" files. Unfortunately
> this is not trivial as well, because both methods use preprocessor
> defined constants from their enclosing class "SuspendResume" which are
> defined right before there definition and undefined right thereafter.
> They are therefore not available in "os_bsd.inline.hpp" and
> "os_linux.inline.hpp".
>
> - The solution for the last problem is to change the mentioned
> preprocessor constants into an C++ enumeration which also results in a
> nicer overall code. The drawback is that all the users of the
> preprocessor constants have to be updated to use the new, qualified
> enum-values (i.e. 'os::Bsd::SuspendResume::SR_CONTINUE' instead of
> 'SR_CONTINUE'). It's a little more verbose but definitely a lot
> cleaner this way.
>
> I also did the following clean-up because I touched the corresponding
> files anyway:
>
> src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp
>
> - remove "#pragma warning(disable: 4035)" because there don't seem to
> be any missing return statement in this file anymore
>
> src/os/bsd/vm/os_bsd.inline.hpp
> src/os/linux/vm/os_linux.inline.hpp
> src/os/solaris/vm/os_solaris.inline.hpp
> src/os/windows/vm/os_windows.inline.hpp
>
> - include both "atomic.hpp" and "atomic.inline.hpp". This is
> unnecessary, because "XXX.inline.hpp" always includes "XXX.hpp"
>
> Finally, I've also updated the copyright line to 2013 in every file
> touched by this change.
More information about the hotspot-runtime-dev
mailing list