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

Volker Simonis volker.simonis at gmail.com
Tue Feb 26 02:04:34 PST 2013


Hi David,

Actually, we would have to include "atomic.inline.hpp" into os.hpp but
that's not possible because "atomic.inline.hpp" transitively includes
"os.hpp" so we would get a circular dependency.

And as Stefan pointed out ".. we should strive to avoid including
.inline.hpp files in .hpp files". I strongly agree with him. Inline
function definitions really belong into ".inline.hpp" files and not
into the plain ".hpp" files. Header files should only define the
interface and therefore they only have a minimum set of dependencies
(i.e. includes). On the other side, ".inline.hpp" files contain the
implementation of inline functions and they usually have a lot more
dependencies.

Somebody who only needs a class definition, just includes the
corresponding header file. Somebody who wants to inline a function
needs its implementation and has to include the corresponding
".inline.hpp" file. The problem with HotSpot is that over time (and
probably mostly for convenience reasons) a lot of inline functions
have been defined in the plain header files which required the
inclusion of a whole bunch of dependencies into these files.

We should really only define simple getter and setter functions as
inline functions in plain header files. Even the definition of other
simple inline methods which have no external dependencies in header
files is potentially bad because over time the methods may evolve and
may require external dependencies. But at that point developers often
forget (or don't want) to move them into the corresponding
".inline.hpp" files and as a result the dependency list of the plain
header files grows unnecessarily.

But that's all a different, much bigger change. Please notice that
Coleen already was so kind to file an RFE for this problem some time
ago: "8003990 : Clean up inline #includes"
(http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003990).

This change only wants to fix the non-PCH build an the named platforms.

Regards,
Volker

On Tue, Feb 26, 2013 at 10:10 AM, David Holmes <david.holmes at oracle.com> wrote:
> 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.
>>>>
>>>>
>>>>
>>>
>


More information about the hotspot-runtime-dev mailing list