[PATCH for-7.0 0/4] qemu-common.h include cleanup

[PATCH for-7.0 0/4] qemu-common.h include cleanup

[Peter Maydell]

qemu-common.h has a comment at the top:

 * This file is supposed to be included only by .c files. No header file should
 * depend on qemu-common.h, as this would easily lead to circular header
 * dependencies.

We still have a few .h files which include it, though.  The first 3
patches in this series fix that: in 3 out of 4 cases we didn't need
the #include at all, and in the 4th case we can instead #include
qemu-common.h from just one .c file.

Patch 4 is just removing the #include from 8 files in hw/arm which
don't need it at all.  (Probably there are other files like this, but
I just did the Arm related ones.)

Tested by pushing to gitlab for the CI build.

-- PMM

Peter Maydell (4):
  include/hw/i386: Don't include qemu-common.h in .h files
  target/hexagon/cpu.h: don't include qemu-common.h
  target/rx/cpu.h: Don't include qemu-common.h
  hw/arm: Don't include qemu-common.h unnecessarily

 include/hw/i386/microvm.h     | 1 -
 include/hw/i386/x86.h         | 1 -
 target/hexagon/cpu.h          | 1 -
 target/rx/cpu.h               | 1 -
 hw/arm/boot.c                 | 1 -
 hw/arm/digic_boards.c         | 1 -
 hw/arm/highbank.c             | 1 -
 hw/arm/npcm7xx_boards.c       | 1 -
 hw/arm/sbsa-ref.c             | 1 -
 hw/arm/stm32f405_soc.c        | 1 -
 hw/arm/vexpress.c             | 1 -
 hw/arm/virt.c                 | 1 -
 linux-user/hexagon/cpu_loop.c | 1 +
 13 files changed, 1 insertion(+), 12 deletions(-)

-- 
2.25.1

[PATCH for-7.0 1/4] include/hw/i386: Don't include qemu-common.h in .h files

[Peter Maydell]

The qemu-common.h header is not supposed to be included from any
other header files, only from .c files (as documented in a comment at
the start of it).

include/hw/i386/x86.h and include/hw/i386/microvm.h break this rule.
In fact, the include is not required at all, so we can just drop it
from both files.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/i386/microvm.h | 1 -
 include/hw/i386/x86.h     | 1 -
 2 files changed, 2 deletions(-)

diff --git a/include/hw/i386/microvm.h b/include/hw/i386/microvm.h
index 4d9c732d4b2..efcbd926fd4 100644
--- a/include/hw/i386/microvm.h
+++ b/include/hw/i386/microvm.h
@@ -18,7 +18,6 @@
 #ifndef HW_I386_MICROVM_H
 #define HW_I386_MICROVM_H
 
-#include "qemu-common.h"
 #include "exec/hwaddr.h"
 #include "qemu/notify.h"
 
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index bb1cfb88966..a145a303703 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -17,7 +17,6 @@
 #ifndef HW_I386_X86_H
 #define HW_I386_X86_H
 
-#include "qemu-common.h"
 #include "exec/hwaddr.h"
 #include "qemu/notify.h"
 
-- 
2.25.1

[PATCH for-7.0 2/4] target/hexagon/cpu.h: don't include qemu-common.h

[Peter Maydell]

The qemu-common.h header is not supposed to be included from any
other header files, only from .c files (as documented in a comment at
the start of it).

Move the include to linux-user/hexagon/cpu_loop.c, which needs it for
the declaration of cpu_exec_step_atomic().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/hexagon/cpu.h          | 1 -
 linux-user/hexagon/cpu_loop.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index de121d950f2..58a0d3870bb 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -23,7 +23,6 @@ typedef struct CPUHexagonState CPUHexagonState;
 
 #include "fpu/softfloat-types.h"
 
-#include "qemu-common.h"
 #include "exec/cpu-defs.h"
 #include "hex_regs.h"
 #include "mmvec/mmvec.h"
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index 6b24cbaba93..e47f8348d56 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu-common.h"
 #include "qemu.h"
 #include "user-internals.h"
 #include "cpu_loop-common.h"
-- 
2.25.1

[PATCH for-7.0 3/4] target/rx/cpu.h: Don't include qemu-common.h

[Peter Maydell]

The qemu-common.h header is not supposed to be included from any
other header files, only from .c files (as documented in a comment at
the start of it).

Nothing actually relies on target/rx/cpu.h including it, so we can
just drop the include.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/rx/cpu.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index 4ac71aec370..657db84ef0a 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -20,7 +20,6 @@
 #define RX_CPU_H
 
 #include "qemu/bitops.h"
-#include "qemu-common.h"
 #include "hw/registerfields.h"
 #include "cpu-qom.h"
 
-- 
2.25.1

[PATCH for-7.0 4/4] hw/arm: Don't include qemu-common.h unnecessarily

[Peter Maydell]

A lot of C files in hw/arm include qemu-common.h when they don't
need anything from it. Drop the include lines.

omap1.c, pxa2xx.c and strongarm.c retain the include because they
use it for the prototype of qemu_get_timedate().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c           | 1 -
 hw/arm/digic_boards.c   | 1 -
 hw/arm/highbank.c       | 1 -
 hw/arm/npcm7xx_boards.c | 1 -
 hw/arm/sbsa-ref.c       | 1 -
 hw/arm/stm32f405_soc.c  | 1 -
 hw/arm/vexpress.c       | 1 -
 hw/arm/virt.c           | 1 -
 8 files changed, 8 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 74ad397b1ff..399f8e837ce 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -8,7 +8,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index b771a3d8b74..4093af09cb2 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -25,7 +25,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "hw/boards.h"
 #include "qemu/error-report.h"
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index c3cb315dbc6..4210894d814 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -18,7 +18,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qapi/error.h"
 #include "hw/sysbus.h"
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index dec7d16ae51..aff8c870420 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -24,7 +24,6 @@
 #include "hw/qdev-core.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qemu/units.h"
 #include "sysemu/blockdev.h"
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 358714bd3e8..dd944553f78 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -18,7 +18,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index 0019b7f4785..c07947d9f8b 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -24,7 +24,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qemu-common.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
 #include "hw/arm/stm32f405_soc.h"
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 58481c07629..3e6d63c7f96 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -23,7 +23,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "cpu.h"
 #include "hw/sysbus.h"
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 30da05dfe04..3e2144e31af 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -29,7 +29,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qemu/units.h"
 #include "qemu/option.h"
-- 
2.25.1

Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup

[Peter Maydell]

On Mon, 29 Nov 2021 at 20:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> qemu-common.h has a comment at the top:
>
>  * This file is supposed to be included only by .c files. No header file should
>  * depend on qemu-common.h, as this would easily lead to circular header
>  * dependencies.

As a side note, that comment was added back in 2012 when qemu-common.h
was bigger, included other headers, and did some of the work we currently
use osdep.h for. As it stands today qemu-common.h includes no other
files so it isn't a source of possible circular dependencies -- it's
just a grab-bag of miscellaneous prototypes that in an ideal world
would be in more focused individual headers[*]. So there's an argument
for deleting this comment...

[*] A cleanup that would be nice, and I'm about to send out a patchset
that splits out the rtc related functions; but the grab-bag at the
bottom of osdep.h is probably higher priority because that header
gets pulled in by an order of magnitude more C files.

-- PMM

Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup

[Richard Henderson]

On 11/29/21 9:05 PM, Peter Maydell wrote:
> qemu-common.h has a comment at the top:
> 
>   * This file is supposed to be included only by .c files. No header file should
>   * depend on qemu-common.h, as this would easily lead to circular header
>   * dependencies.
> 
> We still have a few .h files which include it, though.  The first 3
> patches in this series fix that: in 3 out of 4 cases we didn't need
> the #include at all, and in the 4th case we can instead #include
> qemu-common.h from just one .c file.
> 
> Patch 4 is just removing the #include from 8 files in hw/arm which
> don't need it at all.  (Probably there are other files like this, but
> I just did the Arm related ones.)
> 
> Tested by pushing to gitlab for the CI build.
> 
> -- PMM
> 
> Peter Maydell (4):
>    include/hw/i386: Don't include qemu-common.h in .h files
>    target/hexagon/cpu.h: don't include qemu-common.h
>    target/rx/cpu.h: Don't include qemu-common.h
>    hw/arm: Don't include qemu-common.h unnecessarily

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 29 Nov 2021 at 20:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> qemu-common.h has a comment at the top:
>>
>>  * This file is supposed to be included only by .c files. No header file should
>>  * depend on qemu-common.h, as this would easily lead to circular header
>>  * dependencies.
>
> As a side note, that comment was added back in 2012 when qemu-common.h
> was bigger, included other headers, and did some of the work we currently
> use osdep.h for. As it stands today qemu-common.h includes no other
> files so it isn't a source of possible circular dependencies -- it's
> just a grab-bag of miscellaneous prototypes that in an ideal world
> would be in more focused individual headers[*]. So there's an argument
> for deleting this comment...

First, thank you for this cleanup series.

The comment is from commit 04509ad939a:

    qemu-common.h: Comment about usage rules
    
    Every time we make a tiny change on a header file, we often find
    circular header dependency problems. To avoid this nightmare, we need to
    stop including qemu-common.h from other headers, and we should gradually
    move the declarations from the catch-all qemu-common.h header to their
    specific headers.
    
    This simply adds a comment documenting the rules about qemu-common.h,
    hoping that people will see it before including qemu-common.h from other
    header files, and before adding more declarations to qemu-common.h.
    
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

You're right: we've since moved all #include out of qemu-common.h, so
the risk of circular header dependency is gone, and the comment's claim
"this would easily lead to circular header dependencies" is no longer
correct.

In my opinion, including qemu-common.h in a header is a bad idea
regardless.  Headers should include the headers they need to make them
self-contained, and no more, because more risks slower compiles, in
particular frequent recompiles of pretty much everything.  Previously
discussed at

    https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg06291.html
    Message-ID: <877eac82il.fsf@dusky.pond.sub.org>

Nothing in qemu-common.h should be required to make another header
self-contained.

This is likely the case for other headers as well, which don't carry a
comment forbidding inclusion into headers.  You could argue that
qemu-common.h should not carry one, either.  I can accept that.  I'm not
attached to the comment.  I am interested in keeping unwanted #include
in headers under control.

> [*] A cleanup that would be nice, and I'm about to send out a patchset
> that splits out the rtc related functions; but the grab-bag at the
> bottom of osdep.h is probably higher priority because that header
> gets pulled in by an order of magnitude more C files.

By all "normal" ones, in fact.

Re: [PATCH for-7.0 3/4] target/rx/cpu.h: Don't include qemu-common.h

[Yoshinori Sato]

On Tue, 30 Nov 2021 05:05:09 +0900,
Peter Maydell wrote:
> 
> The qemu-common.h header is not supposed to be included from any
> other header files, only from .c files (as documented in a comment at
> the start of it).
> 
> Nothing actually relies on target/rx/cpu.h including it, so we can
> just drop the include.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>

> ---
>  target/rx/cpu.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/target/rx/cpu.h b/target/rx/cpu.h
> index 4ac71aec370..657db84ef0a 100644
> --- a/target/rx/cpu.h
> +++ b/target/rx/cpu.h
> @@ -20,7 +20,6 @@
>  #define RX_CPU_H
>  
>  #include "qemu/bitops.h"
> -#include "qemu-common.h"
>  #include "hw/registerfields.h"
>  #include "cpu-qom.h"
>  
> -- 
> 2.25.1
> 
> 

Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup

[Philippe Mathieu-Daudé]

On 11/29/21 21:05, Peter Maydell wrote:

> Peter Maydell (4):
>   include/hw/i386: Don't include qemu-common.h in .h files
>   target/hexagon/cpu.h: don't include qemu-common.h
>   target/rx/cpu.h: Don't include qemu-common.h
>   hw/arm: Don't include qemu-common.h unnecessarily

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

RE: [PATCH for-7.0 2/4] target/hexagon/cpu.h: don't include qemu-common.h

[Taylor Simpson]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Monday, November 29, 2021 2:05 PM
> To: qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Cc: Paolo Bonzini <pbonzini@redhat.com>; Sergio Lopez <slp@redhat.com>;
> Taylor Simpson <tsimpson@quicinc.com>; Yoshinori Sato
> <ysato@users.sourceforge.jp>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>
> Subject: [PATCH for-7.0 2/4] target/hexagon/cpu.h: don't include qemu-
> common.h
> 
> The qemu-common.h header is not supposed to be included from any other
> header files, only from .c files (as documented in a comment at the start of
> it).
> 
> Move the include to linux-user/hexagon/cpu_loop.c, which needs it for the
> declaration of cpu_exec_step_atomic().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/hexagon/cpu.h          | 1 -
>  linux-user/hexagon/cpu_loop.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>