| Message ID | 20170907201335.13956-9-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07.09.2017 22:13, David Hildenbrand wrote: > Implemented in sclp.c, so let's move it to the right include file. > Fix up one include. Do a forward declaration of CPUS390XState to fix the > two sclp consoles complaining. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/hw/s390x/sclp.h | 2 ++ > target/s390x/cpu.h | 1 - > target/s390x/misc_helper.c | 1 + > 3 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index a72d096081..4b86a8a293 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > +typedef struct CPUS390XState CPUS390XState; > +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example: https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html https://stackoverflow.com/questions/8367646/redefinition-of-typedef All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead: https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs Thomas
Thomas Huth <thuth@redhat.com> writes: > On 07.09.2017 22:13, David Hildenbrand wrote: >> Implemented in sclp.c, so let's move it to the right include file. >> Fix up one include. Do a forward declaration of CPUS390XState to fix the >> two sclp consoles complaining. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> include/hw/s390x/sclp.h | 2 ++ >> target/s390x/cpu.h | 1 - >> target/s390x/misc_helper.c | 1 + >> 3 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index a72d096081..4b86a8a293 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> +typedef struct CPUS390XState CPUS390XState; >> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > That's dangerous and likely does not work with certain versions of GCC. > You can't do a "forward declaration" with typedef in C, I'm afraid. See > for example: > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html > https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html > https://stackoverflow.com/questions/8367646/redefinition-of-typedef > > All this typedef'ing in QEMU is pretty bad ... we run into this problem > again and again. include/qemu/typedefs.h is just a work-around for this. > I know people like typedefs for some reasons (I used to do that, too, > before I realized the trouble with them), but IMHO we should rather > adopt the typedef-related rules from the kernel coding conventions instead: > > https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs I prefer the kernel style for typedefs myself. But it's strictly a matter of style. Excessive typedeffing makes code harder to understand, it isn't wrong. The part that's wrong is defining things more than once, and that applies to everything, not just typedefs. Sometimes you get away with defining something more than once. It's still wrong. You're not supposed to define the same variable more than once, either (although many C compilers let you get away with it, see -fno-common). You define it in *one* place. If you need to declare it, declare it in *one* place, and make sure that place is in scope at the definition, so the compiler can check the two match. Likewise, you're not supposed to define the same struct tag more than once, and you should declare it in just one place. Likewise, you're not supposed to define (with typedef) the same type more than once. There is no such thing as a typedef declaration. qemu/typedefs.h is not a work-around for a typedef-happy style. Its purpose is breaking inclusion cycles. Secondary purpose is reducing the need for non-cyclic includes. If we didn't typedef, we'd still put our struct declarations there.
On 08.09.2017 14:29, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 07.09.2017 22:13, David Hildenbrand wrote: >>> Implemented in sclp.c, so let's move it to the right include file. >>> Fix up one include. Do a forward declaration of CPUS390XState to fix the >>> two sclp consoles complaining. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> include/hw/s390x/sclp.h | 2 ++ >>> target/s390x/cpu.h | 1 - >>> target/s390x/misc_helper.c | 1 + >>> 3 files changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>> index a72d096081..4b86a8a293 100644 >>> --- a/include/hw/s390x/sclp.h >>> +++ b/include/hw/s390x/sclp.h >>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>> void sclp_service_interrupt(uint32_t sccb); >>> void raise_irq_cpu_hotplug(void); >>> +typedef struct CPUS390XState CPUS390XState; >>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >> >> That's dangerous and likely does not work with certain versions of GCC. >> You can't do a "forward declaration" with typedef in C, I'm afraid. See >> for example: >> >> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html >> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html >> https://stackoverflow.com/questions/8367646/redefinition-of-typedef >> >> All this typedef'ing in QEMU is pretty bad ... we run into this problem >> again and again. include/qemu/typedefs.h is just a work-around for this. >> I know people like typedefs for some reasons (I used to do that, too, >> before I realized the trouble with them), but IMHO we should rather >> adopt the typedef-related rules from the kernel coding conventions instead: >> >> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs > > I prefer the kernel style for typedefs myself. But it's strictly a > matter of style. Excessive typedeffing makes code harder to understand, > it isn't wrong. The part that's wrong is defining things more than > once, and that applies to everything, not just typedefs. > > Sometimes you get away with defining something more than once. It's > still wrong. > > You're not supposed to define the same variable more than once, either > (although many C compilers let you get away with it, see -fno-common). > You define it in *one* place. If you need to declare it, declare it in > *one* place, and make sure that place is in scope at the definition, so > the compiler can check the two match. > > Likewise, you're not supposed to define the same struct tag more than > once, and you should declare it in just one place. AFAIK it's perfect valid C to do a forward declaration of a struct multiple times by just writing e.g. "struct CPUS390XState;" somewhere in your code. This is also what is done in various Linux kernel headers all over the place. > Likewise, you're not supposed to define (with typedef) the same type > more than once. There is no such thing as a typedef declaration. > > qemu/typedefs.h is not a work-around for a typedef-happy style. Its > purpose is breaking inclusion cycles. Secondary purpose is reducing the > need for non-cyclic includes. If we didn't typedef, we'd still put our > struct declarations there. No, since it's not required for struct forward declarations, only to avoid multiple typedef definitions. Thomas
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index a72d096081..4b86a8a293 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); void sclp_service_interrupt(uint32_t sccb); void raise_irq_cpu_hotplug(void); +typedef struct CPUS390XState CPUS390XState; +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 3aa2e46aac..032d1de2e8 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, /* outside of target/s390x/ */ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); -int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index b142db71c6..8b07535b02 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -35,6 +35,7 @@ #include "sysemu/sysemu.h" #include "hw/s390x/ebcdic.h" #include "hw/s390x/s390-virtio-hcall.h" +#include "hw/s390x/sclp.h" #endif /* #define DEBUG_HELPER */
Implemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of CPUS390XState to fix the two sclp consoles complaining. Signed-off-by: David Hildenbrand <david@redhat.com> --- include/hw/s390x/sclp.h | 2 ++ target/s390x/cpu.h | 1 - target/s390x/misc_helper.c | 1 + 3 files changed, 3 insertions(+), 1 deletion(-)