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

[David Holmes]

Hi Volker,

Sorry only have time for a very quick response right now. If the 
os_xx.hpp files need the atomic_xxx files then I would think that if 
os.hpp included atomic.hpp then it should all come together.

David

On 26/02/2013 6:43 PM, Volker Simonis wrote:
> Hi David,
>
> thank you for the comments. Please find my answers inline:
>
> On Tue, Feb 26, 2013 at 3:03 AM, David Holmes <david.holmes at oracle.com> wrote:
>> Volker, thanks for the report and the time spent on the patch. Unfortunately
>> I am very reluctant to accept significant cpp changes for something that
>> should be purely solvable by appropriate header file inclusion. So hopefully
>> when this is examined more closely the "right" fix will become evident.
>
> First of all I don't think that the patch contains significant
> changes. I've only replaced some preprocessor defines with an
> enumeration which could be considered an improvement by itself.
>
> Second, I don't think that in this case we have other possibilities to
> fix the issue. As I've tried to explain, the problem is that
> "os_bsd.hpp" and "os_linux.hpp" are not self-contained files. They are
> themselves included into "os.hpp" right into the middle of the "os"
> class definition (actually, they define the inner classes (i.e.
> "Linux" and "Bsd") of the "os" class. It is therefore simply not
> possible to include "atomic.inline.hpp" (actually to include any file)
> into "os_bsd.hpp" and "os_linux.hpp".
>
> The current use of the preprocessor defined "SR_XXX" constants in
> "os_{linux,bsd}.cpp" is error prone anyway and could easily be broken
> by somebody else defining a macro with the same name (although I agree
> that this is unlikely). Also, I wouldn't consider moving two inline
> functions from "os_{linux,bsd}.hpp" to "os_{linux,bsd}.inline.hpp"
> (where they actually belong to) a major change.
>
> I would therefore kindly ask you to reconsider reviewing the change.
>
>> Also there may be some recent uses of atomic functions that have now been
>> reversed and so might fix some of the problems encountered.
>>
>
> What do you mean by "now have been reversed". I synced hotspot-rt
> yesterday just before sending the webrev and couldn't see any such
> changes which would affect my fix.
>
>> And Volker is right that we should have a no PCH build as part of JPRT
>> testing. How to do that is another matter :(
>>
>
> Is it really that comlicated to add "USE_PRECOMPILED_HEADER=0" to the
> build command line :)
>
>> Anyone have a tool or tip/trick for generating a tree of the includes for a
>> given cpp file?
>>
>
> Just use "gcc -H"
>
>> David
>>
>>
>> On 26/02/2013 4:58 AM, Christian Thalinger 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
>>>
>>> -- 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.
>>>
>>>
>>