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

[Volker Simonis]

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?

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