RFR(M): Fix non-PCH build on Linux, Windows and MacOS X

Volker Simonis volker.simonis at gmail.com
Tue Feb 26 00:19:24 PST 2013


On Mon, Feb 25, 2013 at 7:58 PM, Christian Thalinger
<christian.thalinger at oracle.com> wrote:
>
> On Feb 25, 2013, at 10:09 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>
>> Recent changes have broken the no-PCH build again.
>
> Is this also broken for you?
>
> http://bugs.sun.com/view_bug.do?bug_id=8005084
>

Yes, that's exactly the problem (more precisely, a subset of the problem.)

The first issue reported by 8005084 has been fixed already by
subsequent changes (i.e. the inclusion of "asm/codeBuffer.hpp" into
"assembler.hpp").

The other issues in 8005084 (and some more) are fixed by my changes.
Should I resend the RFR with the bug id '8005084'?

Can you please review it?

Thank you and best regards,
Volker

> -- Chris
>
>>
>> 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/
>>
>> 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
>>
>> - 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