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

[David Holmes]

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
>