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

[Stefan Karlsson]

On 2015-01-29 11:47, Volker Simonis wrote:
> On Thu, Jan 29, 2015 at 10:19 AM, Stefan Karlsson
> <stefan.karlsson at oracle.com> wrote:
>> On 2015-01-28 23:58, Mikael Vidstedt wrote:
>>>
>>> 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.
> Interesting option I didn't knew which further complicates things:)
>
> Just saw that there's also a "-imacros <file>" option which behaves
> like "-include" but throws away all of the <file> content with the
> exception of macro definitions.
>
> But I really think we should strive for a generic solution here which
> is as much as possible "standard" conformant.
>
>> It might also be worth considering having a header file that is always
>> included at the top of all files, and let that file contain a bare minimum
>> set of defines. A good starting point for such a file would be the
>> macros.hpp and selected parts of globalDefinitions.hpp.
>>
>> We would have to do something about precompiled.hpp, since no includes are
>> allowed to be put before the precompiled.hpp include line.
> Good point. In that case, precompiled.hpp would have to first include
> that new header as well.
>
>> A slightly
>> different topic, but maybe it's time to get rid of precompiled.hpp?
>>
> What do you mean? How could we do that without giving up the
> precompiled header build?

I actually meant that we could consider to stop building with 
precompiled headers, if it helps resolving some of the include file 
problems that we are often seeing. Removing the precompiled headers and 
changing our includes to be more precise might even lower the compile 
times of incremental builds.

StefanK

>
> Volker
>
>> StefanK
>>
>>
>>> 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
>>>>>>>