[PATCH v3 0/1] Clean up includes

[PATCH v3 0/1] Clean up includes

[Markus Armbruster]

Back in 2016, we discussed[1] rules for headers, and these were
generally liked:

1. Have a carefully curated header that's included everywhere first.  We
   got that already thanks to Peter: osdep.h.

2. Headers should normally include everything they need beyond osdep.h.
   If exceptions are needed for some reason, they must be documented in
   the header.  If all that's needed from a header is typedefs, put
   those into qemu/typedefs.h instead of including the header.

3. Cyclic inclusion is forbidden.

This series fixes a number of rule violations.

Together with

    [PATCH v2 0/4] hw/ppc: Clean up includes
    [PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes
        in master as commit 9d94c21363..881e019770
    [PATCH v2 0/3] block: Clean up includes
    [PATCH v3 0/5] coroutine: Clean up includes

just three inclusion loops remain reachable from include/:

    target/microblaze/cpu.h target/microblaze/mmu.h

    target/nios2/cpu.h target/nios2/mmu.h

    target/riscv/cpu.h target/riscv/pmp.h

Breaking them would be nice, but I'm out of steam.

v3:
* Rebased, old PATCH 1+2+4 are in master as commit
  881e019770..f07ceffdf5
* PATCH 1: Fix bsd-user

v2:
* Rebased
* PATCH 3: v1 posted separately
* PATCH 4: New

[1] Message-ID: <87h9g8j57d.fsf@blackfin.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html

Markus Armbruster (1):
  include: Don't include qemu/osdep.h

 bsd-user/qemu.h                 | 1 -
 crypto/block-luks-priv.h        | 1 -
 include/hw/cxl/cxl_host.h       | 1 -
 include/hw/input/pl050.h        | 1 -
 include/hw/tricore/triboard.h   | 1 -
 include/qemu/userfaultfd.h      | 1 -
 net/vmnet_int.h                 | 1 -
 qga/cutils.h                    | 1 -
 target/hexagon/hex_arch_types.h | 1 -
 target/hexagon/mmvec/macros.h   | 1 -
 target/riscv/pmu.h              | 1 -
 bsd-user/arm/signal.c           | 1 +
 bsd-user/arm/target_arch_cpu.c  | 2 ++
 bsd-user/freebsd/os-sys.c       | 1 +
 bsd-user/i386/signal.c          | 1 +
 bsd-user/x86_64/signal.c        | 1 +
 qga/cutils.c                    | 3 ++-
 17 files changed, 8 insertions(+), 12 deletions(-)

-- 
2.39.0

[PATCH v3 1/1] include: Don't include qemu/osdep.h

[Markus Armbruster]

docs/devel/style.rst mandates:

    The "qemu/osdep.h" header contains preprocessor macros that affect
    the behavior of core system headers like <stdint.h>.  It must be
    the first include so that core system headers included by external
    libraries get the preprocessor macros that QEMU depends on.

    Do not include "qemu/osdep.h" from header files since the .c file
    will have already included it.

A few violations have crept in.  Fix them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 bsd-user/qemu.h                 | 1 -
 crypto/block-luks-priv.h        | 1 -
 include/hw/cxl/cxl_host.h       | 1 -
 include/hw/input/pl050.h        | 1 -
 include/hw/tricore/triboard.h   | 1 -
 include/qemu/userfaultfd.h      | 1 -
 net/vmnet_int.h                 | 1 -
 qga/cutils.h                    | 1 -
 target/hexagon/hex_arch_types.h | 1 -
 target/hexagon/mmvec/macros.h   | 1 -
 target/riscv/pmu.h              | 1 -
 bsd-user/arm/signal.c           | 1 +
 bsd-user/arm/target_arch_cpu.c  | 2 ++
 bsd-user/freebsd/os-sys.c       | 1 +
 bsd-user/i386/signal.c          | 1 +
 bsd-user/x86_64/signal.c        | 1 +
 qga/cutils.c                    | 3 ++-
 17 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index be6105385e..0ceecfb6df 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -17,7 +17,6 @@
 #ifndef QEMU_H
 #define QEMU_H
 
-#include "qemu/osdep.h"
 #include "cpu.h"
 #include "qemu/units.h"
 #include "exec/cpu_ldst.h"
diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h
index 90a20d432b..1066df0307 100644
--- a/crypto/block-luks-priv.h
+++ b/crypto/block-luks-priv.h
@@ -18,7 +18,6 @@
  *
  */
 
-#include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/bswap.h"
 
diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
index a1b662ce40..c9bc9c7c50 100644
--- a/include/hw/cxl/cxl_host.h
+++ b/include/hw/cxl/cxl_host.h
@@ -7,7 +7,6 @@
  * COPYING file in the top-level directory.
  */
 
-#include "qemu/osdep.h"
 #include "hw/cxl/cxl.h"
 #include "hw/boards.h"
 
diff --git a/include/hw/input/pl050.h b/include/hw/input/pl050.h
index 89ec4fafc9..4cb8985f31 100644
--- a/include/hw/input/pl050.h
+++ b/include/hw/input/pl050.h
@@ -10,7 +10,6 @@
 #ifndef HW_PL050_H
 #define HW_PL050_H
 
-#include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "hw/input/ps2.h"
diff --git a/include/hw/tricore/triboard.h b/include/hw/tricore/triboard.h
index 094c8bd563..4fdd2d7d97 100644
--- a/include/hw/tricore/triboard.h
+++ b/include/hw/tricore/triboard.h
@@ -18,7 +18,6 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "sysemu/sysemu.h"
diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
index 6b74f92792..55c95998e8 100644
--- a/include/qemu/userfaultfd.h
+++ b/include/qemu/userfaultfd.h
@@ -13,7 +13,6 @@
 #ifndef USERFAULTFD_H
 #define USERFAULTFD_H
 
-#include "qemu/osdep.h"
 #include "exec/hwaddr.h"
 #include <linux/userfaultfd.h>
 
diff --git a/net/vmnet_int.h b/net/vmnet_int.h
index adf6e8c20d..d0b90594f2 100644
--- a/net/vmnet_int.h
+++ b/net/vmnet_int.h
@@ -10,7 +10,6 @@
 #ifndef VMNET_INT_H
 #define VMNET_INT_H
 
-#include "qemu/osdep.h"
 #include "vmnet_int.h"
 #include "clients.h"
 
diff --git a/qga/cutils.h b/qga/cutils.h
index f0f30a7d28..2bfaf554a8 100644
--- a/qga/cutils.h
+++ b/qga/cutils.h
@@ -1,7 +1,6 @@
 #ifndef CUTILS_H_
 #define CUTILS_H_
 
-#include "qemu/osdep.h"
 
 int qga_open_cloexec(const char *name, int flags, mode_t mode);
 
diff --git a/target/hexagon/hex_arch_types.h b/target/hexagon/hex_arch_types.h
index 885f68f760..52a7f2b2f3 100644
--- a/target/hexagon/hex_arch_types.h
+++ b/target/hexagon/hex_arch_types.h
@@ -18,7 +18,6 @@
 #ifndef HEXAGON_HEX_ARCH_TYPES_H
 #define HEXAGON_HEX_ARCH_TYPES_H
 
-#include "qemu/osdep.h"
 #include "mmvec/mmvec.h"
 #include "qemu/int128.h"
 
diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index 8c864e8c68..1201d778d0 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -18,7 +18,6 @@
 #ifndef HEXAGON_MMVEC_MACROS_H
 #define HEXAGON_MMVEC_MACROS_H
 
-#include "qemu/osdep.h"
 #include "qemu/host-utils.h"
 #include "arch.h"
 #include "mmvec/system_ext_mmvec.h"
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 3004ce37b6..0c819ca983 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -16,7 +16,6 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "cpu.h"
 #include "qemu/main-loop.h"
diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
index 2b1dd745d1..9734407543 100644
--- a/bsd-user/arm/signal.c
+++ b/bsd-user/arm/signal.c
@@ -17,6 +17,7 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu/osdep.h"
 #include "qemu.h"
 
 /*
diff --git a/bsd-user/arm/target_arch_cpu.c b/bsd-user/arm/target_arch_cpu.c
index 02bf9149d5..fe38ae2210 100644
--- a/bsd-user/arm/target_arch_cpu.c
+++ b/bsd-user/arm/target_arch_cpu.c
@@ -16,6 +16,8 @@
  *  You should have received a copy of the GNU General Public License
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
+
+#include "qemu/osdep.h"
 #include "target_arch.h"
 
 void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index 309e27b9d6..1676ec10f8 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -17,6 +17,7 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu/osdep.h"
 #include "qemu.h"
 #include "target_arch_sysarch.h"
 
diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
index 5dd975ce56..a3131047b8 100644
--- a/bsd-user/i386/signal.c
+++ b/bsd-user/i386/signal.c
@@ -17,6 +17,7 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu/osdep.h"
 #include "qemu.h"
 
 /*
diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
index c3875bc4c6..46cb865180 100644
--- a/bsd-user/x86_64/signal.c
+++ b/bsd-user/x86_64/signal.c
@@ -16,6 +16,7 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "qemu/osdep.h"
 #include "qemu.h"
 
 /*
diff --git a/qga/cutils.c b/qga/cutils.c
index b8e142ef64..b21bcf3683 100644
--- a/qga/cutils.c
+++ b/qga/cutils.c
@@ -2,8 +2,9 @@
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
-#include "cutils.h"
 
+#include "qemu/osdep.h"
+#include "cutils.h"
 #include "qapi/error.h"
 
 /**
-- 
2.39.0

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Michael S. Tsirkin]

On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> docs/devel/style.rst mandates:
> 
>     The "qemu/osdep.h" header contains preprocessor macros that affect
>     the behavior of core system headers like <stdint.h>.  It must be
>     the first include so that core system headers included by external
>     libraries get the preprocessor macros that QEMU depends on.
> 
>     Do not include "qemu/osdep.h" from header files since the .c file
>     will have already included it.
> 
> A few violations have crept in.  Fix them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

With my awesome grep skillz I found one more:
$ grep -r --include='*.h' qemu/osdep.h
include/block/graph-lock.h:#include "qemu/osdep.h"

Looks like all C files must include qemu/osdep.h, no?
How about

1- add -include qemu/osdep.h on compile command line
   drop #include "qemu/osdep.h" from C files
2- drop double include guards, replace with a warning.

following patch implements part 2:


qemu/osdep: don't include it from headers

doing so will lead to trouble eventually - instead of
working around such cases make it more likely it will fail.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 7d059ad526..e4a60f911c 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -24,7 +24,12 @@
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
-#ifndef QEMU_OSDEP_H
+#ifdef QEMU_OSDEP_H
+#warning "Never include qemu/osdep.h from a header!"
+#endif
+
+static inline void qemu_osdep_never_include_from_header(void) {}
+
 #define QEMU_OSDEP_H
 
 #include "config-host.h"
@@ -714,5 +719,3 @@ static inline int platform_does_not_support_system(const char *command)
 #ifdef __cplusplus
 }
 #endif
-
-#endif

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Michael S. Tsirkin]

On Thu, Jan 12, 2023 at 08:51:32AM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > docs/devel/style.rst mandates:
> > 
> >     The "qemu/osdep.h" header contains preprocessor macros that affect
> >     the behavior of core system headers like <stdint.h>.  It must be
> >     the first include so that core system headers included by external
> >     libraries get the preprocessor macros that QEMU depends on.
> > 
> >     Do not include "qemu/osdep.h" from header files since the .c file
> >     will have already included it.
> > 
> > A few violations have crept in.  Fix them.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> 
> With my awesome grep skillz I found one more:
> $ grep -r --include='*.h' qemu/osdep.h
> include/block/graph-lock.h:#include "qemu/osdep.h"

Also:
$ grep -r --include='*.inc' qemu/osdep.h
ui/vnc-enc-zrle.c.inc:#include "qemu/osdep.h"
crypto/akcipher-nettle.c.inc:#include "qemu/osdep.h"
crypto/akcipher-gcrypt.c.inc:#include "qemu/osdep.h"
crypto/rsakey-nettle.c.inc:#include "qemu/osdep.h"
crypto/cipher-gnutls.c.inc:#include "qemu/osdep.h"
target/xtensa/core-dc233c/xtensa-modules.c.inc:#include "qemu/osdep.h"
target/xtensa/core-sample_controller/xtensa-modules.c.inc:#include "qemu/osdep.h"
target/xtensa/core-de212/xtensa-modules.c.inc:#include "qemu/osdep.h"
target/xtensa/core-dc232b/xtensa-modules.c.inc:#include "qemu/osdep.h"
target/xtensa/core-fsf/xtensa-modules.c.inc:#include "qemu/osdep.h"
target/cris/translate_v10.c.inc:#include "qemu/osdep.h"



> Looks like all C files must include qemu/osdep.h, no?
> How about
> 
> 1- add -include qemu/osdep.h on compile command line
>    drop #include "qemu/osdep.h" from C files
> 2- drop double include guards, replace with a warning.
> 
> following patch implements part 2:
> 
> 
> qemu/osdep: don't include it from headers
> 
> doing so will lead to trouble eventually - instead of
> working around such cases make it more likely it will fail.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 7d059ad526..e4a60f911c 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -24,7 +24,12 @@
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
>   */
> -#ifndef QEMU_OSDEP_H
> +#ifdef QEMU_OSDEP_H
> +#warning "Never include qemu/osdep.h from a header!"
> +#endif
> +
> +static inline void qemu_osdep_never_include_from_header(void) {}
> +
>  #define QEMU_OSDEP_H
>  
>  #include "config-host.h"
> @@ -714,5 +719,3 @@ static inline int platform_does_not_support_system(const char *command)
>  #ifdef __cplusplus
>  }
>  #endif
> -
> -#endif

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Jan 12, 2023 at 08:51:32AM -0500, Michael S. Tsirkin wrote:
>> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
>> > docs/devel/style.rst mandates:
>> > 
>> >     The "qemu/osdep.h" header contains preprocessor macros that affect
>> >     the behavior of core system headers like <stdint.h>.  It must be
>> >     the first include so that core system headers included by external
>> >     libraries get the preprocessor macros that QEMU depends on.
>> > 
>> >     Do not include "qemu/osdep.h" from header files since the .c file
>> >     will have already included it.
>> > 
>> > A few violations have crept in.  Fix them.
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
>> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> 
>> With my awesome grep skillz I found one more:
>> $ grep -r --include='*.h' qemu/osdep.h
>> include/block/graph-lock.h:#include "qemu/osdep.h"

Crept in after I prepared my v1.  I neglected to re-check.

> Also:
> $ grep -r --include='*.inc' qemu/osdep.h
> ui/vnc-enc-zrle.c.inc:#include "qemu/osdep.h"
> crypto/akcipher-nettle.c.inc:#include "qemu/osdep.h"
> crypto/akcipher-gcrypt.c.inc:#include "qemu/osdep.h"
> crypto/rsakey-nettle.c.inc:#include "qemu/osdep.h"
> crypto/cipher-gnutls.c.inc:#include "qemu/osdep.h"
> target/xtensa/core-dc233c/xtensa-modules.c.inc:#include "qemu/osdep.h"
> target/xtensa/core-sample_controller/xtensa-modules.c.inc:#include "qemu/osdep.h"
> target/xtensa/core-de212/xtensa-modules.c.inc:#include "qemu/osdep.h"
> target/xtensa/core-dc232b/xtensa-modules.c.inc:#include "qemu/osdep.h"
> target/xtensa/core-fsf/xtensa-modules.c.inc:#include "qemu/osdep.h"
> target/cris/translate_v10.c.inc:#include "qemu/osdep.h"

Good point.  Looks like I successfully supressed all memory of .inc.

>> Looks like all C files must include qemu/osdep.h, no?

I remember there are a few exceptions, but I don't remember which .c
they are.  Hmm... see commit 4bd802b209cff612d1a99674a91895b735be8630.

>> How about
>> 
>> 1- add -include qemu/osdep.h on compile command line
>>    drop #include "qemu/osdep.h" from C files

Then you need to encode the exceptions in the build system.  Which might
not be a bad thing.

>> 2- drop double include guards, replace with a warning.
>> 
>> following patch implements part 2:
>> 
>> 
>> qemu/osdep: don't include it from headers
>> 
>> doing so will lead to trouble eventually - instead of
>> working around such cases make it more likely it will fail.
>> 
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> 
>> ---
>> 
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 7d059ad526..e4a60f911c 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -24,7 +24,12 @@
>>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>   * See the COPYING file in the top-level directory.
>>   */
>> -#ifndef QEMU_OSDEP_H
>> +#ifdef QEMU_OSDEP_H
>> +#warning "Never include qemu/osdep.h from a header!"
>> +#endif
>> +
>> +static inline void qemu_osdep_never_include_from_header(void) {}
>> +

Why do you need the function, too?

>>  #define QEMU_OSDEP_H
>>  
>>  #include "config-host.h"
>> @@ -714,5 +719,3 @@ static inline int platform_does_not_support_system(const char *command)
>>  #ifdef __cplusplus
>>  }
>>  #endif
>> -
>> -#endif

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 08:51:26AM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > docs/devel/style.rst mandates:
> > 
> >     The "qemu/osdep.h" header contains preprocessor macros that affect
> >     the behavior of core system headers like <stdint.h>.  It must be
> >     the first include so that core system headers included by external
> >     libraries get the preprocessor macros that QEMU depends on.
> > 
> >     Do not include "qemu/osdep.h" from header files since the .c file
> >     will have already included it.
> > 
> > A few violations have crept in.  Fix them.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> 
> With my awesome grep skillz I found one more:
> $ grep -r --include='*.h' qemu/osdep.h
> include/block/graph-lock.h:#include "qemu/osdep.h"
> 
> Looks like all C files must include qemu/osdep.h, no?

Yes, and IMHO that is/was a mistake, as it means our other header
files are not self-contained, which prevents developer tools from
reporting useful bugs when you're editting.

For example, if you have clangd integrated into your editor, it will
warn you as you're editting if you've referenced a function / type
that doesn't exist in the file, or anything it includes. This is made
completely useless for QEMU .h files though, as they're all incomplete,
only the .c files have the full headers.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Peter Maydell]

On Thu, 12 Jan 2023 at 15:14, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jan 12, 2023 at 08:51:26AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > > docs/devel/style.rst mandates:
> > >
> > >     The "qemu/osdep.h" header contains preprocessor macros that affect
> > >     the behavior of core system headers like <stdint.h>.  It must be
> > >     the first include so that core system headers included by external
> > >     libraries get the preprocessor macros that QEMU depends on.
> > >
> > >     Do not include "qemu/osdep.h" from header files since the .c file
> > >     will have already included it.
> > >
> > > A few violations have crept in.  Fix them.
> > >
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
> > With my awesome grep skillz I found one more:
> > $ grep -r --include='*.h' qemu/osdep.h
> > include/block/graph-lock.h:#include "qemu/osdep.h"
> >
> > Looks like all C files must include qemu/osdep.h, no?
>
> Yes, and IMHO that is/was a mistake, as it means our other header
> files are not self-contained, which prevents developer tools from
> reporting useful bugs when you're editting.

The underlying requirement is "osdep.h must be included
before any system header file". "Always first in every .c file"
is an easy way to achieve that, and "never in any .h file" is
then not mandatory but falls out from the fact that any
such include is pointless and only serves to increase
the compilation time (and to increase the chances that
you don't notice that you forgot osdep.h in your .c file).

If there's a better way to do this (e.g. one which meant
that it was a compile error to put osdep includes in the
wrong place or to omit them) then that would certainly
save us periodically having to do this kind of fixup commit.

thanks
-- PMM

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 03:58:56PM +0000, Peter Maydell wrote:
> On Thu, 12 Jan 2023 at 15:14, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 12, 2023 at 08:51:26AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > > > docs/devel/style.rst mandates:
> > > >
> > > >     The "qemu/osdep.h" header contains preprocessor macros that affect
> > > >     the behavior of core system headers like <stdint.h>.  It must be
> > > >     the first include so that core system headers included by external
> > > >     libraries get the preprocessor macros that QEMU depends on.
> > > >
> > > >     Do not include "qemu/osdep.h" from header files since the .c file
> > > >     will have already included it.
> > > >
> > > > A few violations have crept in.  Fix them.
> > > >
> > > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > With my awesome grep skillz I found one more:
> > > $ grep -r --include='*.h' qemu/osdep.h
> > > include/block/graph-lock.h:#include "qemu/osdep.h"
> > >
> > > Looks like all C files must include qemu/osdep.h, no?
> >
> > Yes, and IMHO that is/was a mistake, as it means our other header
> > files are not self-contained, which prevents developer tools from
> > reporting useful bugs when you're editting.
> 
> The underlying requirement is "osdep.h must be included
> before any system header file". "Always first in every .c file"
> is an easy way to achieve that, and "never in any .h file" is
> then not mandatory but falls out from the fact that any
> such include is pointless and only serves to increase
> the compilation time (and to increase the chances that
> you don't notice that you forgot osdep.h in your .c file).
> 
> If there's a better way to do this (e.g. one which meant
> that it was a compile error to put osdep includes in the
> wrong place or to omit them) then that would certainly
> save us periodically having to do this kind of fixup commit.

I think the challenge is that osdep.h is too big as it exists
today. The stuff the needs to come before system headers is
actually little more than config-host.h and a few #defines
most of which are specific to windows. If those critical
#defines went into config-host.h, then we could have a rule
'config-host.h' must be included in all .c files as the first
thing. All the header files could just reference the specific
system headers they care about instead of making everything
from osdep.h visible in their namespace.  Still this would be
quite a lot of work to adapt to at this point.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Peter Maydell]

On Thu, 12 Jan 2023 at 16:08, Daniel P. Berrangé <berrange@redhat.com> wrote:
> I think the challenge is that osdep.h is too big as it exists
> today. The stuff the needs to come before system headers is
> actually little more than config-host.h and a few #defines
> most of which are specific to windows. If those critical
> #defines went into config-host.h, then we could have a rule
> 'config-host.h' must be included in all .c files as the first
> thing.

This doesn't seem much different to the rules we have today,
except you've renamed osdep.h to config-host.h...

> All the header files could just reference the specific
> system headers they care about instead of making everything
> from osdep.h visible in their namespace.  Still this would be
> quite a lot of work to adapt to at this point.

It certainly does have more in it than strictly necessary,
though we have thinned it out quite a bit from when we
first put in the convention. A lot of the functions at
the tail end of the file could be moved out into their
own headers, for instance -- patches welcome ;-)

> All the header files could just reference the specific
> system headers they care about instead of making everything
> from osdep.h visible in their namespace.

There are some complicated things in there, not always
limited to Windows. Also where there is some header
that needs a platform-specific workaround I prefer that
that header is pulled in by osdep.h. This avoids the
failure mode of "developer working on Linux directly
includes some-system-header.h; works fine on their machine,
but doesn't work on oddball-platform where the header
needs a workaround". (For instance, handling "sys/mman.h
on this system doesn't define MAP_ANONYMOUS", or the
backcompat stuff in glib-compat.h.)

thanks
-- PMM

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 04:20:36PM +0000, Peter Maydell wrote:
> On Thu, 12 Jan 2023 at 16:08, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > I think the challenge is that osdep.h is too big as it exists
> > today. The stuff the needs to come before system headers is
> > actually little more than config-host.h and a few #defines
> > most of which are specific to windows. If those critical
> > #defines went into config-host.h, then we could have a rule
> > 'config-host.h' must be included in all .c files as the first
> > thing.
> 
> This doesn't seem much different to the rules we have today,
> except you've renamed osdep.h to config-host.h...

If the QEMU header files all contain #includes for the
system headers they rely on, then when tools are validating
code in the header, they can stand a better chance of being
able to resolve all the types. Though it'll still fail if
some of the system header pieces only get exposed as a result
of config-host.h macros, but that's relatively few, compared
to today where amost nothing resolves if yuo validate the
headers files in isolation.


> > All the header files could just reference the specific
> > system headers they care about instead of making everything
> > from osdep.h visible in their namespace.
> 
> There are some complicated things in there, not always
> limited to Windows. Also where there is some header
> that needs a platform-specific workaround I prefer that
> that header is pulled in by osdep.h. This avoids the
> failure mode of "developer working on Linux directly
> includes some-system-header.h; works fine on their machine,
> but doesn't work on oddball-platform where the header
> needs a workaround". (For instance, handling "sys/mman.h
> on this system doesn't define MAP_ANONYMOUS", or the
> backcompat stuff in glib-compat.h.)

Yeah, its not entirely straightforward, though our CI will catch
that on our most important target platforms.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Peter Maydell]

On Thu, 12 Jan 2023 at 16:30, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jan 12, 2023 at 04:20:36PM +0000, Peter Maydell wrote:
> > On Thu, 12 Jan 2023 at 16:08, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > I think the challenge is that osdep.h is too big as it exists
> > > today. The stuff the needs to come before system headers is
> > > actually little more than config-host.h and a few #defines
> > > most of which are specific to windows. If those critical
> > > #defines went into config-host.h, then we could have a rule
> > > 'config-host.h' must be included in all .c files as the first
> > > thing.
> >
> > This doesn't seem much different to the rules we have today,
> > except you've renamed osdep.h to config-host.h...
>
> If the QEMU header files all contain #includes for the
> system headers they rely on, then when tools are validating
> code in the header, they can stand a better chance of being
> able to resolve all the types. Though it'll still fail if
> some of the system header pieces only get exposed as a result
> of config-host.h macros, but that's relatively few, compared
> to today where amost nothing resolves if yuo validate the
> headers files in isolation.

Yeah, but I don't want QEMU header files to contain
lots of includes for system header files, because of...

> > There are some complicated things in there, not always
> > limited to Windows. Also where there is some header
> > that needs a platform-specific workaround I prefer that
> > that header is pulled in by osdep.h. This avoids the
> > failure mode of "developer working on Linux directly
> > includes some-system-header.h; works fine on their machine,
> > but doesn't work on oddball-platform where the header
> > needs a workaround". (For instance, handling "sys/mman.h
> > on this system doesn't define MAP_ANONYMOUS", or the
> > backcompat stuff in glib-compat.h.)

...this. So we'd have to have config-host.h include all
the system headers we're working around anyway.

-- PMM

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Jonathan Cameron via]

On Thu, 12 Jan 2023 12:50:05 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> docs/devel/style.rst mandates:
> 
>     The "qemu/osdep.h" header contains preprocessor macros that affect
>     the behavior of core system headers like <stdint.h>.  It must be
>     the first include so that core system headers included by external
>     libraries get the preprocessor macros that QEMU depends on.
> 
>     Do not include "qemu/osdep.h" from header files since the .c file
>     will have already included it.
> 
> A few violations have crept in.  Fix them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

For the CXL one.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  bsd-user/qemu.h                 | 1 -
>  crypto/block-luks-priv.h        | 1 -
>  include/hw/cxl/cxl_host.h       | 1 -
>  include/hw/input/pl050.h        | 1 -
>  include/hw/tricore/triboard.h   | 1 -
>  include/qemu/userfaultfd.h      | 1 -
>  net/vmnet_int.h                 | 1 -
>  qga/cutils.h                    | 1 -
>  target/hexagon/hex_arch_types.h | 1 -
>  target/hexagon/mmvec/macros.h   | 1 -
>  target/riscv/pmu.h              | 1 -
>  bsd-user/arm/signal.c           | 1 +
>  bsd-user/arm/target_arch_cpu.c  | 2 ++
>  bsd-user/freebsd/os-sys.c       | 1 +
>  bsd-user/i386/signal.c          | 1 +
>  bsd-user/x86_64/signal.c        | 1 +
>  qga/cutils.c                    | 3 ++-
>  17 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index be6105385e..0ceecfb6df 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -17,7 +17,6 @@
>  #ifndef QEMU_H
>  #define QEMU_H
>  
> -#include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "qemu/units.h"
>  #include "exec/cpu_ldst.h"
> diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h
> index 90a20d432b..1066df0307 100644
> --- a/crypto/block-luks-priv.h
> +++ b/crypto/block-luks-priv.h
> @@ -18,7 +18,6 @@
>   *
>   */
>  
> -#include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/bswap.h"
>  
> diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
> index a1b662ce40..c9bc9c7c50 100644
> --- a/include/hw/cxl/cxl_host.h
> +++ b/include/hw/cxl/cxl_host.h
> @@ -7,7 +7,6 @@
>   * COPYING file in the top-level directory.
>   */
>  
> -#include "qemu/osdep.h"
>  #include "hw/cxl/cxl.h"
>  #include "hw/boards.h"
>  
> diff --git a/include/hw/input/pl050.h b/include/hw/input/pl050.h
> index 89ec4fafc9..4cb8985f31 100644
> --- a/include/hw/input/pl050.h
> +++ b/include/hw/input/pl050.h
> @@ -10,7 +10,6 @@
>  #ifndef HW_PL050_H
>  #define HW_PL050_H
>  
> -#include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "hw/input/ps2.h"
> diff --git a/include/hw/tricore/triboard.h b/include/hw/tricore/triboard.h
> index 094c8bd563..4fdd2d7d97 100644
> --- a/include/hw/tricore/triboard.h
> +++ b/include/hw/tricore/triboard.h
> @@ -18,7 +18,6 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/boards.h"
>  #include "sysemu/sysemu.h"
> diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
> index 6b74f92792..55c95998e8 100644
> --- a/include/qemu/userfaultfd.h
> +++ b/include/qemu/userfaultfd.h
> @@ -13,7 +13,6 @@
>  #ifndef USERFAULTFD_H
>  #define USERFAULTFD_H
>  
> -#include "qemu/osdep.h"
>  #include "exec/hwaddr.h"
>  #include <linux/userfaultfd.h>
>  
> diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> index adf6e8c20d..d0b90594f2 100644
> --- a/net/vmnet_int.h
> +++ b/net/vmnet_int.h
> @@ -10,7 +10,6 @@
>  #ifndef VMNET_INT_H
>  #define VMNET_INT_H
>  
> -#include "qemu/osdep.h"
>  #include "vmnet_int.h"
>  #include "clients.h"
>  
> diff --git a/qga/cutils.h b/qga/cutils.h
> index f0f30a7d28..2bfaf554a8 100644
> --- a/qga/cutils.h
> +++ b/qga/cutils.h
> @@ -1,7 +1,6 @@
>  #ifndef CUTILS_H_
>  #define CUTILS_H_
>  
> -#include "qemu/osdep.h"
>  
>  int qga_open_cloexec(const char *name, int flags, mode_t mode);
>  
> diff --git a/target/hexagon/hex_arch_types.h b/target/hexagon/hex_arch_types.h
> index 885f68f760..52a7f2b2f3 100644
> --- a/target/hexagon/hex_arch_types.h
> +++ b/target/hexagon/hex_arch_types.h
> @@ -18,7 +18,6 @@
>  #ifndef HEXAGON_HEX_ARCH_TYPES_H
>  #define HEXAGON_HEX_ARCH_TYPES_H
>  
> -#include "qemu/osdep.h"
>  #include "mmvec/mmvec.h"
>  #include "qemu/int128.h"
>  
> diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
> index 8c864e8c68..1201d778d0 100644
> --- a/target/hexagon/mmvec/macros.h
> +++ b/target/hexagon/mmvec/macros.h
> @@ -18,7 +18,6 @@
>  #ifndef HEXAGON_MMVEC_MACROS_H
>  #define HEXAGON_MMVEC_MACROS_H
>  
> -#include "qemu/osdep.h"
>  #include "qemu/host-utils.h"
>  #include "arch.h"
>  #include "mmvec/system_ext_mmvec.h"
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 3004ce37b6..0c819ca983 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -16,7 +16,6 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "cpu.h"
>  #include "qemu/main-loop.h"
> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> index 2b1dd745d1..9734407543 100644
> --- a/bsd-user/arm/signal.c
> +++ b/bsd-user/arm/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  
>  /*
> diff --git a/bsd-user/arm/target_arch_cpu.c b/bsd-user/arm/target_arch_cpu.c
> index 02bf9149d5..fe38ae2210 100644
> --- a/bsd-user/arm/target_arch_cpu.c
> +++ b/bsd-user/arm/target_arch_cpu.c
> @@ -16,6 +16,8 @@
>   *  You should have received a copy of the GNU General Public License
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
> +
> +#include "qemu/osdep.h"
>  #include "target_arch.h"
>  
>  void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> index 309e27b9d6..1676ec10f8 100644
> --- a/bsd-user/freebsd/os-sys.c
> +++ b/bsd-user/freebsd/os-sys.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  #include "target_arch_sysarch.h"
>  
> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
> index 5dd975ce56..a3131047b8 100644
> --- a/bsd-user/i386/signal.c
> +++ b/bsd-user/i386/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  
>  /*
> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
> index c3875bc4c6..46cb865180 100644
> --- a/bsd-user/x86_64/signal.c
> +++ b/bsd-user/x86_64/signal.c
> @@ -16,6 +16,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  
>  /*
> diff --git a/qga/cutils.c b/qga/cutils.c
> index b8e142ef64..b21bcf3683 100644
> --- a/qga/cutils.c
> +++ b/qga/cutils.c
> @@ -2,8 +2,9 @@
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
>   */
> -#include "cutils.h"
>  
> +#include "qemu/osdep.h"
> +#include "cutils.h"
>  #include "qapi/error.h"
>  
>  /**

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Jonathan Cameron]

On Thu, 12 Jan 2023 12:50:05 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> docs/devel/style.rst mandates:
> 
>     The "qemu/osdep.h" header contains preprocessor macros that affect
>     the behavior of core system headers like <stdint.h>.  It must be
>     the first include so that core system headers included by external
>     libraries get the preprocessor macros that QEMU depends on.
> 
>     Do not include "qemu/osdep.h" from header files since the .c file
>     will have already included it.
> 
> A few violations have crept in.  Fix them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

For the CXL one.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  bsd-user/qemu.h                 | 1 -
>  crypto/block-luks-priv.h        | 1 -
>  include/hw/cxl/cxl_host.h       | 1 -
>  include/hw/input/pl050.h        | 1 -
>  include/hw/tricore/triboard.h   | 1 -
>  include/qemu/userfaultfd.h      | 1 -
>  net/vmnet_int.h                 | 1 -
>  qga/cutils.h                    | 1 -
>  target/hexagon/hex_arch_types.h | 1 -
>  target/hexagon/mmvec/macros.h   | 1 -
>  target/riscv/pmu.h              | 1 -
>  bsd-user/arm/signal.c           | 1 +
>  bsd-user/arm/target_arch_cpu.c  | 2 ++
>  bsd-user/freebsd/os-sys.c       | 1 +
>  bsd-user/i386/signal.c          | 1 +
>  bsd-user/x86_64/signal.c        | 1 +
>  qga/cutils.c                    | 3 ++-
>  17 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index be6105385e..0ceecfb6df 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -17,7 +17,6 @@
>  #ifndef QEMU_H
>  #define QEMU_H
>  
> -#include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "qemu/units.h"
>  #include "exec/cpu_ldst.h"
> diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h
> index 90a20d432b..1066df0307 100644
> --- a/crypto/block-luks-priv.h
> +++ b/crypto/block-luks-priv.h
> @@ -18,7 +18,6 @@
>   *
>   */
>  
> -#include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/bswap.h"
>  
> diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
> index a1b662ce40..c9bc9c7c50 100644
> --- a/include/hw/cxl/cxl_host.h
> +++ b/include/hw/cxl/cxl_host.h
> @@ -7,7 +7,6 @@
>   * COPYING file in the top-level directory.
>   */
>  
> -#include "qemu/osdep.h"
>  #include "hw/cxl/cxl.h"
>  #include "hw/boards.h"
>  
> diff --git a/include/hw/input/pl050.h b/include/hw/input/pl050.h
> index 89ec4fafc9..4cb8985f31 100644
> --- a/include/hw/input/pl050.h
> +++ b/include/hw/input/pl050.h
> @@ -10,7 +10,6 @@
>  #ifndef HW_PL050_H
>  #define HW_PL050_H
>  
> -#include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "hw/input/ps2.h"
> diff --git a/include/hw/tricore/triboard.h b/include/hw/tricore/triboard.h
> index 094c8bd563..4fdd2d7d97 100644
> --- a/include/hw/tricore/triboard.h
> +++ b/include/hw/tricore/triboard.h
> @@ -18,7 +18,6 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/boards.h"
>  #include "sysemu/sysemu.h"
> diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
> index 6b74f92792..55c95998e8 100644
> --- a/include/qemu/userfaultfd.h
> +++ b/include/qemu/userfaultfd.h
> @@ -13,7 +13,6 @@
>  #ifndef USERFAULTFD_H
>  #define USERFAULTFD_H
>  
> -#include "qemu/osdep.h"
>  #include "exec/hwaddr.h"
>  #include <linux/userfaultfd.h>
>  
> diff --git a/net/vmnet_int.h b/net/vmnet_int.h
> index adf6e8c20d..d0b90594f2 100644
> --- a/net/vmnet_int.h
> +++ b/net/vmnet_int.h
> @@ -10,7 +10,6 @@
>  #ifndef VMNET_INT_H
>  #define VMNET_INT_H
>  
> -#include "qemu/osdep.h"
>  #include "vmnet_int.h"
>  #include "clients.h"
>  
> diff --git a/qga/cutils.h b/qga/cutils.h
> index f0f30a7d28..2bfaf554a8 100644
> --- a/qga/cutils.h
> +++ b/qga/cutils.h
> @@ -1,7 +1,6 @@
>  #ifndef CUTILS_H_
>  #define CUTILS_H_
>  
> -#include "qemu/osdep.h"
>  
>  int qga_open_cloexec(const char *name, int flags, mode_t mode);
>  
> diff --git a/target/hexagon/hex_arch_types.h b/target/hexagon/hex_arch_types.h
> index 885f68f760..52a7f2b2f3 100644
> --- a/target/hexagon/hex_arch_types.h
> +++ b/target/hexagon/hex_arch_types.h
> @@ -18,7 +18,6 @@
>  #ifndef HEXAGON_HEX_ARCH_TYPES_H
>  #define HEXAGON_HEX_ARCH_TYPES_H
>  
> -#include "qemu/osdep.h"
>  #include "mmvec/mmvec.h"
>  #include "qemu/int128.h"
>  
> diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
> index 8c864e8c68..1201d778d0 100644
> --- a/target/hexagon/mmvec/macros.h
> +++ b/target/hexagon/mmvec/macros.h
> @@ -18,7 +18,6 @@
>  #ifndef HEXAGON_MMVEC_MACROS_H
>  #define HEXAGON_MMVEC_MACROS_H
>  
> -#include "qemu/osdep.h"
>  #include "qemu/host-utils.h"
>  #include "arch.h"
>  #include "mmvec/system_ext_mmvec.h"
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 3004ce37b6..0c819ca983 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -16,7 +16,6 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "cpu.h"
>  #include "qemu/main-loop.h"
> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> index 2b1dd745d1..9734407543 100644
> --- a/bsd-user/arm/signal.c
> +++ b/bsd-user/arm/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  
>  /*
> diff --git a/bsd-user/arm/target_arch_cpu.c b/bsd-user/arm/target_arch_cpu.c
> index 02bf9149d5..fe38ae2210 100644
> --- a/bsd-user/arm/target_arch_cpu.c
> +++ b/bsd-user/arm/target_arch_cpu.c
> @@ -16,6 +16,8 @@
>   *  You should have received a copy of the GNU General Public License
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
> +
> +#include "qemu/osdep.h"
>  #include "target_arch.h"
>  
>  void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> index 309e27b9d6..1676ec10f8 100644
> --- a/bsd-user/freebsd/os-sys.c
> +++ b/bsd-user/freebsd/os-sys.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  #include "target_arch_sysarch.h"
>  
> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
> index 5dd975ce56..a3131047b8 100644
> --- a/bsd-user/i386/signal.c
> +++ b/bsd-user/i386/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  
>  /*
> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
> index c3875bc4c6..46cb865180 100644
> --- a/bsd-user/x86_64/signal.c
> +++ b/bsd-user/x86_64/signal.c
> @@ -16,6 +16,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  
>  /*
> diff --git a/qga/cutils.c b/qga/cutils.c
> index b8e142ef64..b21bcf3683 100644
> --- a/qga/cutils.c
> +++ b/qga/cutils.c
> @@ -2,8 +2,9 @@
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
>   */
> -#include "cutils.h"
>  
> +#include "qemu/osdep.h"
> +#include "cutils.h"
>  #include "qapi/error.h"
>  
>  /**

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Michael S. Tsirkin]

On Thu, Jan 12, 2023 at 03:47:19PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Jan 12, 2023 at 08:51:32AM -0500, Michael S. Tsirkin wrote:
> >> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> >> > docs/devel/style.rst mandates:
> >> > 
> >> >     The "qemu/osdep.h" header contains preprocessor macros that affect
> >> >     the behavior of core system headers like <stdint.h>.  It must be
> >> >     the first include so that core system headers included by external
> >> >     libraries get the preprocessor macros that QEMU depends on.
> >> > 
> >> >     Do not include "qemu/osdep.h" from header files since the .c file
> >> >     will have already included it.
> >> > 
> >> > A few violations have crept in.  Fix them.
> >> > 
> >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> >> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> >> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >> 
> >> With my awesome grep skillz I found one more:
> >> $ grep -r --include='*.h' qemu/osdep.h
> >> include/block/graph-lock.h:#include "qemu/osdep.h"
> 
> Crept in after I prepared my v1.  I neglected to re-check.
> 
> > Also:
> > $ grep -r --include='*.inc' qemu/osdep.h
> > ui/vnc-enc-zrle.c.inc:#include "qemu/osdep.h"
> > crypto/akcipher-nettle.c.inc:#include "qemu/osdep.h"
> > crypto/akcipher-gcrypt.c.inc:#include "qemu/osdep.h"
> > crypto/rsakey-nettle.c.inc:#include "qemu/osdep.h"
> > crypto/cipher-gnutls.c.inc:#include "qemu/osdep.h"
> > target/xtensa/core-dc233c/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > target/xtensa/core-sample_controller/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > target/xtensa/core-de212/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > target/xtensa/core-dc232b/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > target/xtensa/core-fsf/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > target/cris/translate_v10.c.inc:#include "qemu/osdep.h"
> 
> Good point.  Looks like I successfully supressed all memory of .inc.
> 
> >> Looks like all C files must include qemu/osdep.h, no?
> 
> I remember there are a few exceptions, but I don't remember which .c
> they are.  Hmm... see commit 4bd802b209cff612d1a99674a91895b735be8630.
> 
> >> How about
> >> 
> >> 1- add -include qemu/osdep.h on compile command line
> >>    drop #include "qemu/osdep.h" from C files
> 
> Then you need to encode the exceptions in the build system.  Which might
> not be a bad thing.
> 
> >> 2- drop double include guards, replace with a warning.
> >> 
> >> following patch implements part 2:
> >> 
> >> 
> >> qemu/osdep: don't include it from headers
> >> 
> >> doing so will lead to trouble eventually - instead of
> >> working around such cases make it more likely it will fail.
> >> 
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> 
> >> ---
> >> 
> >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> >> index 7d059ad526..e4a60f911c 100644
> >> --- a/include/qemu/osdep.h
> >> +++ b/include/qemu/osdep.h
> >> @@ -24,7 +24,12 @@
> >>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>   * See the COPYING file in the top-level directory.
> >>   */
> >> -#ifndef QEMU_OSDEP_H
> >> +#ifdef QEMU_OSDEP_H
> >> +#warning "Never include qemu/osdep.h from a header!"
> >> +#endif
> >> +
> >> +static inline void qemu_osdep_never_include_from_header(void) {}
> >> +
> 
> Why do you need the function, too?

This seems to give a bit more info if header does get included
twice: instead of just a warning on the second include compiler says
definition is duplicated and then shows where the first definition was.
OTOH first one was almost for sure from the proper first include so
maybe we don't care. Let me drop this.

> >>  #define QEMU_OSDEP_H
> >>  
> >>  #include "config-host.h"
> >> @@ -714,5 +719,3 @@ static inline int platform_does_not_support_system(const char *command)
> >>  #ifdef __cplusplus
> >>  }
> >>  #endif
> >> -
> >> -#endif

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Michael S. Tsirkin]

On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> docs/devel/style.rst mandates:
> 
>     The "qemu/osdep.h" header contains preprocessor macros that affect
>     the behavior of core system headers like <stdint.h>.  It must be
>     the first include so that core system headers included by external
>     libraries get the preprocessor macros that QEMU depends on.
> 
>     Do not include "qemu/osdep.h" from header files since the .c file
>     will have already included it.
> 
> A few violations have crept in.  Fix them.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

here's v2 - can be applied after you fix all instances of this.
Feel free to use.

--->

qemu/osdep: we don't include it from headers

doing so will lead to trouble eventually - instead of silently working
around such cases make it more likely it will fail.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

-- 

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 7d059ad526..3ddeb7fd41 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -24,7 +24,9 @@
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
-#ifndef QEMU_OSDEP_H
+#ifdef QEMU_OSDEP_H
+#warning "Never include qemu/osdep.h from a header!"
+#else
 #define QEMU_OSDEP_H
 
 #include "config-host.h"

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Michael S. Tsirkin]

On Thu, Jan 12, 2023 at 03:58:56PM +0000, Peter Maydell wrote:
> On Thu, 12 Jan 2023 at 15:14, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 12, 2023 at 08:51:26AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > > > docs/devel/style.rst mandates:
> > > >
> > > >     The "qemu/osdep.h" header contains preprocessor macros that affect
> > > >     the behavior of core system headers like <stdint.h>.  It must be
> > > >     the first include so that core system headers included by external
> > > >     libraries get the preprocessor macros that QEMU depends on.
> > > >
> > > >     Do not include "qemu/osdep.h" from header files since the .c file
> > > >     will have already included it.
> > > >
> > > > A few violations have crept in.  Fix them.
> > > >
> > > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > With my awesome grep skillz I found one more:
> > > $ grep -r --include='*.h' qemu/osdep.h
> > > include/block/graph-lock.h:#include "qemu/osdep.h"
> > >
> > > Looks like all C files must include qemu/osdep.h, no?
> >
> > Yes, and IMHO that is/was a mistake, as it means our other header
> > files are not self-contained, which prevents developer tools from
> > reporting useful bugs when you're editting.
> 
> The underlying requirement is "osdep.h must be included
> before any system header file". "Always first in every .c file"
> is an easy way to achieve that, and "never in any .h file" is
> then not mandatory but falls out from the fact that any
> such include is pointless and only serves to increase
> the compilation time (and to increase the chances that
> you don't notice that you forgot osdep.h in your .c file).
> 
> If there's a better way to do this (e.g. one which meant
> that it was a compile error to put osdep includes in the
> wrong place or to omit them) then that would certainly
> save us periodically having to do this kind of fixup commit.
> 
> thanks
> -- PMM

yes I just posted a patch that will catch most (though not all)
such cases. if we switch to -include it will catch all of them
but there seems to be some resistance to this idea.

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 12:37:46PM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 12, 2023 at 03:47:19PM +0100, Markus Armbruster wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > 
> > > On Thu, Jan 12, 2023 at 08:51:32AM -0500, Michael S. Tsirkin wrote:
> > >> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > >> > docs/devel/style.rst mandates:
> > >> > 
> > >> >     The "qemu/osdep.h" header contains preprocessor macros that affect
> > >> >     the behavior of core system headers like <stdint.h>.  It must be
> > >> >     the first include so that core system headers included by external
> > >> >     libraries get the preprocessor macros that QEMU depends on.
> > >> > 
> > >> >     Do not include "qemu/osdep.h" from header files since the .c file
> > >> >     will have already included it.
> > >> > 
> > >> > A few violations have crept in.  Fix them.
> > >> > 
> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > >> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > >> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > >> 
> > >> With my awesome grep skillz I found one more:
> > >> $ grep -r --include='*.h' qemu/osdep.h
> > >> include/block/graph-lock.h:#include "qemu/osdep.h"
> > 
> > Crept in after I prepared my v1.  I neglected to re-check.
> > 
> > > Also:
> > > $ grep -r --include='*.inc' qemu/osdep.h
> > > ui/vnc-enc-zrle.c.inc:#include "qemu/osdep.h"
> > > crypto/akcipher-nettle.c.inc:#include "qemu/osdep.h"
> > > crypto/akcipher-gcrypt.c.inc:#include "qemu/osdep.h"
> > > crypto/rsakey-nettle.c.inc:#include "qemu/osdep.h"
> > > crypto/cipher-gnutls.c.inc:#include "qemu/osdep.h"
> > > target/xtensa/core-dc233c/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > target/xtensa/core-sample_controller/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > target/xtensa/core-de212/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > target/xtensa/core-dc232b/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > target/xtensa/core-fsf/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > target/cris/translate_v10.c.inc:#include "qemu/osdep.h"
> > 
> > Good point.  Looks like I successfully supressed all memory of .inc.
> > 
> > >> Looks like all C files must include qemu/osdep.h, no?
> > 
> > I remember there are a few exceptions, but I don't remember which .c
> > they are.  Hmm... see commit 4bd802b209cff612d1a99674a91895b735be8630.
> > 
> > >> How about
> > >> 
> > >> 1- add -include qemu/osdep.h on compile command line
> > >>    drop #include "qemu/osdep.h" from C files
> > 
> > Then you need to encode the exceptions in the build system.  Which might
> > not be a bad thing.
> > 
> > >> 2- drop double include guards, replace with a warning.
> > >> 
> > >> following patch implements part 2:
> > >> 
> > >> 
> > >> qemu/osdep: don't include it from headers
> > >> 
> > >> doing so will lead to trouble eventually - instead of
> > >> working around such cases make it more likely it will fail.
> > >> 
> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >> 
> > >> ---
> > >> 
> > >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > >> index 7d059ad526..e4a60f911c 100644
> > >> --- a/include/qemu/osdep.h
> > >> +++ b/include/qemu/osdep.h
> > >> @@ -24,7 +24,12 @@
> > >>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > >>   * See the COPYING file in the top-level directory.
> > >>   */
> > >> -#ifndef QEMU_OSDEP_H
> > >> +#ifdef QEMU_OSDEP_H
> > >> +#warning "Never include qemu/osdep.h from a header!"
> > >> +#endif
> > >> +
> > >> +static inline void qemu_osdep_never_include_from_header(void) {}
> > >> +
> > 
> > Why do you need the function, too?
> 
> This seems to give a bit more info if header does get included
> twice: instead of just a warning on the second include compiler says
> definition is duplicated and then shows where the first definition was.
> OTOH first one was almost for sure from the proper first include so
> maybe we don't care. Let me drop this.

FWIW, if we want to simplify our header guards, we could replace the

  #ifndef FOO_H
  #define FOO_H

  ....

  #endif /* FOO_H */

with merely

  #pragma once

at the top of each header.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

[Michael S. Tsirkin]

On Thu, Jan 12, 2023 at 05:44:31PM +0000, Daniel P. Berrangé wrote:
> On Thu, Jan 12, 2023 at 12:37:46PM -0500, Michael S. Tsirkin wrote:
> > On Thu, Jan 12, 2023 at 03:47:19PM +0100, Markus Armbruster wrote:
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > 
> > > > On Thu, Jan 12, 2023 at 08:51:32AM -0500, Michael S. Tsirkin wrote:
> > > >> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > > >> > docs/devel/style.rst mandates:
> > > >> > 
> > > >> >     The "qemu/osdep.h" header contains preprocessor macros that affect
> > > >> >     the behavior of core system headers like <stdint.h>.  It must be
> > > >> >     the first include so that core system headers included by external
> > > >> >     libraries get the preprocessor macros that QEMU depends on.
> > > >> > 
> > > >> >     Do not include "qemu/osdep.h" from header files since the .c file
> > > >> >     will have already included it.
> > > >> > 
> > > >> > A few violations have crept in.  Fix them.
> > > >> > 
> > > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > >> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > >> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > > >> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > >> 
> > > >> With my awesome grep skillz I found one more:
> > > >> $ grep -r --include='*.h' qemu/osdep.h
> > > >> include/block/graph-lock.h:#include "qemu/osdep.h"
> > > 
> > > Crept in after I prepared my v1.  I neglected to re-check.
> > > 
> > > > Also:
> > > > $ grep -r --include='*.inc' qemu/osdep.h
> > > > ui/vnc-enc-zrle.c.inc:#include "qemu/osdep.h"
> > > > crypto/akcipher-nettle.c.inc:#include "qemu/osdep.h"
> > > > crypto/akcipher-gcrypt.c.inc:#include "qemu/osdep.h"
> > > > crypto/rsakey-nettle.c.inc:#include "qemu/osdep.h"
> > > > crypto/cipher-gnutls.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-dc233c/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-sample_controller/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-de212/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-dc232b/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-fsf/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/cris/translate_v10.c.inc:#include "qemu/osdep.h"
> > > 
> > > Good point.  Looks like I successfully supressed all memory of .inc.
> > > 
> > > >> Looks like all C files must include qemu/osdep.h, no?
> > > 
> > > I remember there are a few exceptions, but I don't remember which .c
> > > they are.  Hmm... see commit 4bd802b209cff612d1a99674a91895b735be8630.
> > > 
> > > >> How about
> > > >> 
> > > >> 1- add -include qemu/osdep.h on compile command line
> > > >>    drop #include "qemu/osdep.h" from C files
> > > 
> > > Then you need to encode the exceptions in the build system.  Which might
> > > not be a bad thing.
> > > 
> > > >> 2- drop double include guards, replace with a warning.
> > > >> 
> > > >> following patch implements part 2:
> > > >> 
> > > >> 
> > > >> qemu/osdep: don't include it from headers
> > > >> 
> > > >> doing so will lead to trouble eventually - instead of
> > > >> working around such cases make it more likely it will fail.
> > > >> 
> > > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >> 
> > > >> ---
> > > >> 
> > > >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > >> index 7d059ad526..e4a60f911c 100644
> > > >> --- a/include/qemu/osdep.h
> > > >> +++ b/include/qemu/osdep.h
> > > >> @@ -24,7 +24,12 @@
> > > >>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > >>   * See the COPYING file in the top-level directory.
> > > >>   */
> > > >> -#ifndef QEMU_OSDEP_H
> > > >> +#ifdef QEMU_OSDEP_H
> > > >> +#warning "Never include qemu/osdep.h from a header!"
> > > >> +#endif
> > > >> +
> > > >> +static inline void qemu_osdep_never_include_from_header(void) {}
> > > >> +
> > > 
> > > Why do you need the function, too?
> > 
> > This seems to give a bit more info if header does get included
> > twice: instead of just a warning on the second include compiler says
> > definition is duplicated and then shows where the first definition was.
> > OTOH first one was almost for sure from the proper first include so
> > maybe we don't care. Let me drop this.
> 
> FWIW, if we want to simplify our header guards, we could replace the
> 
>   #ifndef FOO_H
>   #define FOO_H
> 
>   ....
> 
>   #endif /* FOO_H */
> 
> with merely
> 
>   #pragma once
> 
> at the top of each header.
> 
> With regards,
> Daniel

Will break this trick, won't it?

-- 
MST