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

[Mikael Vidstedt]

On 2015-01-28 02:26, Volker Simonis wrote:
> On Wed, Jan 28, 2015 at 10:29 AM, David Holmes <david.holmes at oracle.com> wrote:
>> 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?
>>
> I think the problem is that there is no general rule. There are just
> hints like "every header file should be self-contained", i.e. it
> should include all the other headers it depends on. Following this
> rule should allow any arbitrary inclusion order.
>
> But in our case, we rely on a specific macro ("__STDC_FORMAT_MACROS")
> being defined before we include a system header. So we have three
> possibilities:
>   1. define the macro at the top of every file before including anything.
>   2. define the macro on the command line (actually a more elegant version of 1.)
>   3. define the macro once in a local header and make sure the local
> header is always included before any system header.

I don't think that what I'm about to say can be relied on for all the 
platforms (compilers) we support, but just mentioning it anyway:

gcc provides an option which is a bit like a combination of all the 
options above - the "-include <file>" option will have the effect of 
including the specified header file as if "#include <file>" appeared as 
the first line in the primary source file. Again, not sure there are 
similar options for the other compilers, but perhaps interesting to keep 
in mind.

Cheers,
Mikael

>
> Solution 1 and 2 should still allow an arbitrary inclusion order if
> all headers are self-contained.
>
> The more I think about the problem the more I come to the conclusion
> that for this specific issue solution 2 (defining the macro on the
> command line) is the right solution. We already do this in other cases
> in order to control how and what will be included from the system
> headers (e.g. -D_GNU_SOURCE, -D_REENTRANT on Linux) and
> -D__STDC_FORMAT_MACROS would perfectly fit into this list. It is
> unclear to me why the latter was defined in a local header file.
>
> By the way, the example you mentioned, where we rely on a system
> include coming first:
>
> #ifndef SOME_SYSTEM_THING
>      #define SOME_SYSTEM_THING xxx
> #endif
>
> can be easily solved by having a local wrapper which only includes the
> system header and afterwards performs the required configuration
> steps. Other files of course will have to include the local wrapper
> instead of the system header.
>
> Regards,
> Volker
>
>> 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
>>>>