[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 01/29] include: Make headers more self-contai
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/29] include: Make headers more self-contained |
Date: |
Wed, 07 Aug 2019 22:28:54 +0100 |
User-agent: |
mu4e 1.3.4; emacs 27.0.50 |
Markus Armbruster <address@hidden> writes:
> Alex Bennée <address@hidden> writes:
>
>> Markus Armbruster <address@hidden> writes:
>>
>>> 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 patch gets include/ closer to obeying 2.
>>>
>>> It's actually extracted from my "[RFC] Baby steps towards saner
>>> headers" series[2], which demonstrates a possible path towards
>>> checking 2 automatically. It passes the RFC test there.
>>>
>>> [1] Message-ID: <address@hidden>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html
>>> [2] Message-Id: <address@hidden>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg02715.html
>>>
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>> Reviewed-by: Alistair Francis <address@hidden>
>>> ---
>> <snip>
>>> include/exec/cputlb.h | 2 ++
>>> include/exec/exec-all.h | 1 +
>>> include/exec/ioport.h | 2 ++
>>> include/exec/memory-internal.h | 2 ++
>>> include/exec/ram_addr.h | 1 +
>>> include/exec/softmmu-semi.h | 2 ++
>>> include/exec/tb-hash.h | 2 ++
>>> include/exec/user/thunk.h | 1 +
>>> include/fpu/softfloat-macros.h | 2 ++
>> <snip>
>>>
>>> /*
>>> * bdrv_write_threshold_set:
>>> diff --git a/include/disas/disas.h b/include/disas/disas.h
>>> index 15da511f49..ba47e9197c 100644
>>> --- a/include/disas/disas.h
>>> +++ b/include/disas/disas.h
>>> @@ -1,6 +1,7 @@
>>> #ifndef QEMU_DISAS_H
>>> #define QEMU_DISAS_H
>>>
>>> +#include "exec/hwaddr.h"
>>>
>>> #ifdef NEED_CPU_H
>>> #include "cpu.h"
>>> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
>>> index 5373188be3..23abd71579 100644
>>> --- a/include/exec/cputlb.h
>>> +++ b/include/exec/cputlb.h
>>> @@ -19,6 +19,8 @@
>>> #ifndef CPUTLB_H
>>> #define CPUTLB_H
>>>
>>> +#include "exec/cpu-common.h"
>>> +
>>> #if !defined(CONFIG_USER_ONLY)
>>> /* cputlb.c */
>>> void tlb_protect_code(ram_addr_t ram_addr);
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index 16034ee651..135aeaab0d 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -20,6 +20,7 @@
>>> #ifndef EXEC_ALL_H
>>> #define EXEC_ALL_H
>>>
>>> +#include "cpu.h"
>>> #include "exec/tb-context.h"
>>> #include "sysemu/cpus.h"
>>>
>>> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
>>> index a298b89ce1..97feb296d2 100644
>>> --- a/include/exec/ioport.h
>>> +++ b/include/exec/ioport.h
>>> @@ -24,6 +24,8 @@
>>> #ifndef IOPORT_H
>>> #define IOPORT_H
>>>
>>> +#include "exec/memory.h"
>>> +
>>> #define MAX_IOPORTS (64 * 1024)
>>> #define IOPORTS_MASK (MAX_IOPORTS - 1)
>>>
>>> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
>>> index d1a9dd1ec8..ef4fb92371 100644
>>> --- a/include/exec/memory-internal.h
>>> +++ b/include/exec/memory-internal.h
>>> @@ -20,6 +20,8 @@
>>> #ifndef MEMORY_INTERNAL_H
>>> #define MEMORY_INTERNAL_H
>>>
>>> +#include "cpu.h"
>>> +
>>> #ifndef CONFIG_USER_ONLY
>>> static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>>> {
>>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>>> index b7b2e60ff6..a327a80cfe 100644
>>> --- a/include/exec/ram_addr.h
>>> +++ b/include/exec/ram_addr.h
>>> @@ -20,6 +20,7 @@
>>> #define RAM_ADDR_H
>>>
>>> #ifndef CONFIG_USER_ONLY
>>> +#include "cpu.h"
>>> #include "hw/xen/xen.h"
>>> #include "sysemu/tcg.h"
>>> #include "exec/ramlist.h"
>>> diff --git a/include/exec/softmmu-semi.h b/include/exec/softmmu-semi.h
>>> index 970837992e..fbcae88f4b 100644
>>> --- a/include/exec/softmmu-semi.h
>>> +++ b/include/exec/softmmu-semi.h
>>> @@ -10,6 +10,8 @@
>>> #ifndef SOFTMMU_SEMI_H
>>> #define SOFTMMU_SEMI_H
>>>
>>> +#include "cpu.h"
>>> +
>>> static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
>>> {
>>> uint64_t val;
>>> diff --git a/include/exec/tb-hash.h b/include/exec/tb-hash.h
>>> index 4f3a37d927..805235d321 100644
>>> --- a/include/exec/tb-hash.h
>>> +++ b/include/exec/tb-hash.h
>>> @@ -20,6 +20,8 @@
>>> #ifndef EXEC_TB_HASH_H
>>> #define EXEC_TB_HASH_H
>>>
>>> +#include "exec/cpu-defs.h"
>>> +#include "exec/exec-all.h"
>>> #include "qemu/xxhash.h"
>>>
>>> #ifdef CONFIG_SOFTMMU
>>> diff --git a/include/exec/user/thunk.h b/include/exec/user/thunk.h
>>> index 8d3af5a3be..d05a8a4dab 100644
>>> --- a/include/exec/user/thunk.h
>>> +++ b/include/exec/user/thunk.h
>>> @@ -20,6 +20,7 @@
>>> #define THUNK_H
>>>
>>> #include "cpu.h"
>>> +#include "exec/user/abitypes.h"
>>>
>>> /* types enums definitions */
>>
>> These all seem OK.
>>
>>>
>>> diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
>>> index c55aa6d174..be83a833ec 100644
>>> --- a/include/fpu/softfloat-macros.h
>>> +++ b/include/fpu/softfloat-macros.h
>>> @@ -82,6 +82,8 @@ this code that are retained.
>>> #ifndef FPU_SOFTFLOAT_MACROS_H
>>> #define FPU_SOFTFLOAT_MACROS_H
>>>
>>> +#include "fpu/softfloat.h"
>>> +
>>
>> What does softfloat-macros actually need from the core softfloat API?
>> These are lower level functions used by softfloat itself (and m68k for
>> it's own bit fiddling).
>
> I extracted this patch out of "[PATCH RFC v5 0/3] Baby steps towards
> saner headers". PATCH 1/3 creates make target "check-source", which is
> what I used to find headers that aren't self-contained. In this case:
>
> CC cris-softmmu/tests/headers-tgt/include/fpu/softfloat.o
> In file included from tests/headers-tgt/include/fpu/softfloat-macros.c:21:
> /work/armbru/qemu/include/fpu/softfloat-macros.h: In function
> ‘estimateDiv128To64’:
> /work/armbru/qemu/include/fpu/softfloat-macros.h:623:27: error: implicit
> declaration of function ‘LIT64’ [-Werror=implicit-function-declaration]
> 623 | if ( b <= a0 ) return LIT64( 0xFFFFFFFFFFFFFFFF );
The LIT64 definition should be moved to softfloat-types.h which is
already included by softfloat.h unless we already have a QEMU expansion
we should be using. The softfloat-macros.h can include softfloat-types.h
as well and we should only include the full softfloat.h if they need it.
Do you want me to spin up a patch?
> | ^~~~~
> /work/armbru/qemu/include/fpu/softfloat-macros.h:623:27: error: nested extern
> declaration of ‘LIT64’ [-Werror=nested-externs]
> /work/armbru/qemu/include/fpu/softfloat-macros.h: At top level:
> /work/armbru/qemu/include/fpu/softfloat-macros.h:761:15: error: unknown type
> name ‘flag’
> 761 | static inline flag eq128( uint64_t a0, uint64_t a1, uint64_t b0,
> uint64_t b1 )
> | ^~~~
> /work/armbru/qemu/include/fpu/softfloat-macros.h:774:15: error: unknown type
> name ‘flag’
> 774 | static inline flag le128( uint64_t a0, uint64_t a1, uint64_t b0,
> uint64_t b1 )
> | ^~~~
> /work/armbru/qemu/include/fpu/softfloat-macros.h:787:15: error: unknown type
> name ‘flag’
> 787 | static inline flag lt128( uint64_t a0, uint64_t a1, uint64_t b0,
> uint64_t b1 )
> | ^~~~
> /work/armbru/qemu/include/fpu/softfloat-macros.h:800:15: error: unknown type
> name ‘flag’
> 800 | static inline flag ne128( uint64_t a0, uint64_t a1, uint64_t b0,
> uint64_t b1 )
> | ^~~~
> cc1: all warnings being treated as errors
> make[1]: *** [/work/armbru/qemu/rules.mak:69:
> tests/headers-tgt/include/fpu/softfloat-macros.o] Error 1
--
Alex Bennée
- Re: [Qemu-devel] [PATCH v2 09/29] Include migration/qemu-file-types.h a lot less, (continued)
[Qemu-devel] [PATCH v2 13/29] Clean up inclusion of exec/cpu-common.h, Markus Armbruster, 2019/08/06
[Qemu-devel] [PATCH v2 11/29] typedefs: Separate incomplete types and function types, Markus Armbruster, 2019/08/06
[Qemu-devel] [PATCH v2 01/29] include: Make headers more self-contained, Markus Armbruster, 2019/08/06
[Qemu-devel] [PATCH v2 14/29] migration: Move the VMStateDescription typedef to typedefs.h, Markus Armbruster, 2019/08/06
[Qemu-devel] [PATCH v2 10/29] ide: Include hw/ide/internal a bit less outside hw/ide/, Markus Armbruster, 2019/08/06
[Qemu-devel] [PATCH v2 25/29] numa: Move remaining NUMA declarations from sysemu.h to numa.h, Markus Armbruster, 2019/08/06
[Qemu-devel] [PATCH v2 07/29] trace: Do not include qom/cpu.h into generated trace.h, Markus Armbruster, 2019/08/06