Request or comment: 8071690: Include local HotSpot headers before the system headers

[David Holmes]

Hi Goetz,

On 28/01/2015 6:26 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> but by enforcing that rule, the effect is limited to the header
> that includes other system headers.  You can fix the problem
> by adding globalDefinitions.hpp in that one header foo.h, and whoever
> includes foo.h get's it right.

Maybe we need to back up a step - what is the rule about header files 
including other header files?

David

> If fcntl.h had been added to foo.h in first place, all files that include
> foo.h must include globalDefinitions.hpp before it ... not very easy to
> catch.
>
> Best regards,
>     Goetz.
>
>
>
>
>
> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of David Holmes
> Sent: Wednesday, January 28, 2015 5:22 AM
> To: Volker Simonis; HotSpot Open Source Developers
> Subject: Re: Request or comment: 8071690: Include local HotSpot headers before the system headers
>
> Hi Volker,
>
> On 28/01/2015 3:29 AM, Volker Simonis wrote:
>> Hi,
>>
>> I've just opened "8071690: Include local HotSpot headers before the
>> system headers" but before I start working on it I'd like to hear what
>> others think about the problem. If there's a consensus that it will be
>> worth while doing this change I'll be happy to start working on it.
>
> As I wrote in the bug report:
>
> I don't see how you can apply this as a general rule - or at least not
> without some further rules. If local foo.h depends on a system header
> then it will #include it, so a file that #includes foo.h can't control
> the order of that system header include.
>
> We need to recognize (and detect!) where we have implicit ordering
> constraints but I don't think a general rule actually helps with that.
> And there may be cases where we rely on a system include coming first eg:
>
> #ifndef SOME_SYSTEM_THING
>     #define SOME_SYSTEM_THING xxx
> #endif
>
> David
> -----
>
>> https://bugs.openjdk.java.net/browse/JDK-8071690
>>
>> The following description is copied from the bug report for your convenience:
>>
>> There's no general rule in which order header files should be included
>> but I think a good hint is to include local, project specific headers
>> before system headers. This is mainly for two reasons:
>>
>> 1. It prevents that dependencies from local header files to system
>> headers are hidden because a local header is included after a system
>> header by chance. Instead, every local header should explicitly
>> specify the system headers it depends on.
>>
>> 2. It enables the definition of local macros which control the
>> behaviour of the system headers which are included later on.
>>
>> Point two may be considered bad style, but we actually use it for
>> example in src/share/vm/utilities/globalDefinitions.hpp where we
>> define "__STDC_FORMAT_MACROS" before we include "<inttypes.h>" in the
>> compiler specific global definitions file.
>>
>> "__STDC_FORMAT_MACROS" controls the definition of the printf format
>> macros in "<inttypes.h>" but this can only work if "<inttypes.h>" is
>> really included AFTER the definition of "__STDC_FORMAT_MACROS". And
>> this can only wok if every file includes the local HotSpot headers
>> before any system headers, because otherwise "<inttypes.h>" may be
>> indirectly included by a system header before we had a chance to
>> define "__STDC_FORMAT_MACROS".
>>
>> This is exactly what happened after the integration of 8050807 which
>> added the system include "<fcntl.h>" to vmError.cpp as follows:
>>
>> #include <fcntl.h>
>> #include "precompiled.hpp"
>> #include "code/codeCache.hpp"
>>
>> This change broke the build on AIX because "<fcntl.h>" indirectly
>> included "<inttypes.h>" BEFORE we defined "__STDC_FORMAT_MACROS".
>>
>> To prevent such errors in the future I propose to change all HotSpot
>> source files to always include the system headers AFTER the inclusion
>> of the project specific HotSpot headers.
>>
>> I’ve attached a small Pythin script to this bug report which can be
>> used as follows to detect the files which are currently violating this
>> rule:
>>
>> find src/ \( -name "*.cpp" -o -name "*.hpp" \) -type f -exec python
>> include.py {} \;
>>
>> src/os/solaris/dtrace/generateJvmOffsets.cpp: system header #include
>> <proc_service.h> included before local header #include
>> "code/codeBlob.hpp"
>> src/os/windows/vm/decoder_windows.hpp: system header #include
>> <imagehlp.h> included before local header #include
>> "utilities/decoder.hpp"
>> src/os_cpu/bsd_zero/vm/os_bsd_zero.cpp: system header # include
>> <pthread_np.h> included before local header #include
>> "assembler_zero.inline.hpp"
>> src/share/vm/adlc/adlc.hpp: system header #include <iostream> included
>> before local header #include "string.h"
>> src/share/vm/libadt/port.hpp: system header #include <string.h>
>> included before local header #include "port_tandem.hpp"
>> src/share/vm/runtime/os.hpp: system header # include <setjmp.h>
>> included before local header # include "jvm_solaris.h"
>> src/share/vm/trace/traceDataTypes.hpp: system header #include
>> <stddef.h> included before local header #include
>> "utilities/globalDefinitions.hpp"
>> src/share/vm/utilities/dtrace.hpp: system header #include
>> <sys/types.h> included before local header #include
>> "dtracefiles/hotspot.h"
>> src/share/vm/utilities/elfFile.cpp: system header #include <new>
>> included before local header #include "memory/allocation.inline.hpp"
>> src/share/vm/utilities/elfFile.hpp: system header #include <stdio.h>
>> included before local header #include "globalDefinitions.hpp"
>> src/share/vm/utilities/vmError.cpp: system header #include <fcntl.h>
>> included before local header #include "precompiled.hpp"
>>
>> The script is written in Python intentionally such that it can be
>> easily added as an automated hook to jcheck to prevent violations of
>> this inclusion rule in the future.
>>
>> Of course we can also try to not rely on the inclusion order at all.
>> IMHO it actually seems that this is the cleanest solution, but it may
>> require moving some macro definitions from header files right into the
>> command line (e.g. -D__STDC_FORMAT_MACROS for the example above). By
>> the way, that's actually the way how I've fixed the above mentioned
>> compile error on AIX without the need to touch any shared files.
>>
>> What do you think?
>>
>> Regards,
>> Volker
>>