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

[Volker Simonis]

The standard format macros like PRId32, PRIi64, etc. are specified by
C99 in <inttypes.h> but C99 states the these macros should only be
defined in C++ if __STDC_FORMAT_MACROS is set before the inclusion of
<inttypes.h> (see footnote 185 on page 198 in
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf)

On Wed, Jan 28, 2015 at 11:50 AM, David Holmes <david.holmes at oracle.com> wrote:
> On 28/01/2015 8:26 PM, 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.
>>
>> 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.
>
>
> I'm a bit confused about this whole STDC_FORMAT_MACRO business. Here is what
> I see:
>
> ./share/vm/utilities/globalDefinitions.hpp:#ifndef __STDC_FORMAT_MACROS
> ./share/vm/utilities/globalDefinitions.hpp:#define __STDC_FORMAT_MACROS
> ./os/aix/vm/loadlib_aix.cpp:#ifndef __STDC_FORMAT_MACROS
> ./os/aix/vm/loadlib_aix.cpp:#define __STDC_FORMAT_MACROS
> ./os/aix/vm/loadlib_aix.cpp:// are only defined if '__STDC_FORMAT_MACROS' is
> defined!
> ./os/aix/vm/jvm_aix.h:#if !defined(__STDC_FORMAT_MACROS)
> ./os/aix/vm/jvm_aix.h:#  define __STDC_FORMAT_MACROS          1
> ./os/bsd/vm/jvm_bsd.h:# if !defined(__STDC_FORMAT_MACROS)
> ./os/bsd/vm/jvm_bsd.h:#  define __STDC_FORMAT_MACROS          1
>
> I thought the double underscore macros were only for compiler use - so what
> normally sets it and what does it get used for?
>
> David
> -----
>
>
>> 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
>>>>>
>>>
>