* Function-like macro with the same name as a typedef confuses Coccinelle
@ 2020-04-02 12:06 Markus Armbruster
2020-04-02 12:21 ` Peter Maydell
2020-04-02 12:29 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 7+ messages in thread
From: Markus Armbruster @ 2020-04-02 12:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Vladimir Sementsov-Ogievskiy, Paolo Bonzini
I discovered that Vladimir's auto-propagated-errp.cocci leaves
hw/arm/armsse.c unchanged, even though it clearly should change it.
Running spatch with --debug prints (among lots of other things)
let's go
-----------------------------------------------------------------------
-----------------------------------------------------------------------
parse error
= error in hw/arm/armsse.c; set verbose_parsing for more info
badcount: 7
bad: }
bad:
bad: static void nsccfg_handler(void *opaque, int n, int level)
bad: {
BAD:!!!!! ARMSSE *s = ARMSSE(opaque);
bad:
bad: s->nsccfg = level;
bad: }
parse error
Alright, what's wrong with this? ARMSSE is both a function-like macro
and a typedef:
#define ARMSSE(obj) OBJECT_CHECK(ARMSSE, (obj), TYPE_ARMSSE)
typedef struct ARMSSE {
...
} ARMSSE;
This appears to spook Coccinelle badly enough to skip code using ARMSSE.
If I rename the macro to ARMSSE_() just for testing, it produces the
transformation I'd expect.
Grepping for typedef names is bothersome, so I used ctags -x to build a
cross reference table, and searched that for function-like macros
clashing with typedefs. Result:
include/hw/arm/armsse.h:#define ARMSSE(obj) OBJECT_CHECK(ARMSSE, (obj), TYPE_ARMSSE)
include/hw/arm/armsse.h:} ARMSSE;
include/hw/block/swim.h:#define SWIM(obj) OBJECT_CHECK(SWIM, (obj), TYPE_SWIM)
include/hw/block/swim.h:} SWIM;
target/rx/cpu-qom.h:#define RXCPU(obj) \
target/rx/cpu.h:typedef struct RXCPU RXCPU;
target/s390x/translate.c:#define BD(N, BB, BD) { BB, 4, 0, FLD_C_b##N, FLD_O_b##N }, \
hw/audio/ac97.c:} BD;
The last one is a name clash in unrelated files; should not bother
Coccinelle.
The first three are due to QOM. By convention, the name of the
function-like macro to convert to the QOM type is the QOM type in
ALL_CAPS. Clash when the QOM type is ALL_CAPS already.
Clearly, Coccinelle is getting spooked to easily. Regardless, three
questions:
1. Are ALL_CAPS typedef names a good idea? We shout macros to tell
readers "beware, possibly magic". Shouting other stuff as well
undermines that.
2. The compiler is quite cool with us using the same name for a
function-like macro and a not-function-like non-macro. But is it a good
idea?
3. Do we want to work around Coccinelle's parsing flaw? Having it skip
so much code is clearly suboptimal, and potentially dangerous. The
obvious work-around is to rename the three problematic types so they
contain at least one lower-case letter.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Function-like macro with the same name as a typedef confuses Coccinelle
2020-04-02 12:06 Function-like macro with the same name as a typedef confuses Coccinelle Markus Armbruster
@ 2020-04-02 12:21 ` Peter Maydell
2020-04-02 13:30 ` Markus Armbruster
2020-04-02 12:29 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2020-04-02 12:21 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, QEMU Developers
On Thu, 2 Apr 2020 at 13:06, Markus Armbruster <armbru@redhat.com> wrote:
>
> I discovered that Vladimir's auto-propagated-errp.cocci leaves
> hw/arm/armsse.c unchanged, even though it clearly should change it.
> Running spatch with --debug prints (among lots of other things)
> Clearly, Coccinelle is getting spooked to easily.
Is it worth asking on the coccinelle mailing list about whether
coccinelle could be made to be less picky in this area ?
> Regardless, three questions:
>
> 1. Are ALL_CAPS typedef names a good idea? We shout macros to tell
> readers "beware, possibly magic". Shouting other stuff as well
> undermines that.
>
> 2. The compiler is quite cool with us using the same name for a
> function-like macro and a not-function-like non-macro. But is it a good
> idea?
Probably not a great idea, and if we really only do it 3 times
it's not too hard to change I suppose. I think this basically
arises when the natural name for the struct happens to be all
uppercase already because the device name is an acronym. We
don't usually titlecase acronyms in structure names (eg
we say 'SCSIBus', not 'ScsiBus'), and (legacy exceptions aside)
we don't usually tack on a trailing 'State' or 'Device'
to the main device state struct these days -- so if your device's
natural name is an acronym then the struct ends up all-caps.
If we don't like all-caps struct names then ideally we'd
adjust one or the other of those conventions so we have a
consistent way to avoid them.
For 'ARMSSE' we could I suppose rename it 'ArmSSE', which would
be in line with current corporate branding but out of line with
most of the other places we use 'ARM' in a struct name :-)
Q: how many all-upper-case typedefs do we have in total (whether
they have a clashing macro or not)? Your argument 1 would
suggest we should look to change them all, not merely the ones
Coccinelle trips over.
thanks
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Function-like macro with the same name as a typedef confuses Coccinelle
2020-04-02 12:06 Function-like macro with the same name as a typedef confuses Coccinelle Markus Armbruster
2020-04-02 12:21 ` Peter Maydell
@ 2020-04-02 12:29 ` Philippe Mathieu-Daudé
2020-04-02 13:47 ` Markus Armbruster
1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-02 12:29 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Peter Maydell, Vladimir Sementsov-Ogievskiy, Paolo Bonzini
On 4/2/20 2:06 PM, Markus Armbruster wrote:
> I discovered that Vladimir's auto-propagated-errp.cocci leaves
> hw/arm/armsse.c unchanged, even though it clearly should change it.
> Running spatch with --debug prints (among lots of other things)
>
> let's go
> -----------------------------------------------------------------------
> -----------------------------------------------------------------------
> parse error
> = error in hw/arm/armsse.c; set verbose_parsing for more info
> badcount: 7
> bad: }
> bad:
> bad: static void nsccfg_handler(void *opaque, int n, int level)
> bad: {
> BAD:!!!!! ARMSSE *s = ARMSSE(opaque);
> bad:
> bad: s->nsccfg = level;
> bad: }
> parse error
>
> Alright, what's wrong with this? ARMSSE is both a function-like macro
> and a typedef:
>
> #define ARMSSE(obj) OBJECT_CHECK(ARMSSE, (obj), TYPE_ARMSSE)
>
> typedef struct ARMSSE {
> ...
> } ARMSSE;
>
> This appears to spook Coccinelle badly enough to skip code using ARMSSE.
>
> If I rename the macro to ARMSSE_() just for testing, it produces the
> transformation I'd expect.
>
> Grepping for typedef names is bothersome, so I used ctags -x to build a
> cross reference table, and searched that for function-like macros
> clashing with typedefs. Result:
>
> include/hw/arm/armsse.h:#define ARMSSE(obj) OBJECT_CHECK(ARMSSE, (obj), TYPE_ARMSSE)
> include/hw/arm/armsse.h:} ARMSSE;
> include/hw/block/swim.h:#define SWIM(obj) OBJECT_CHECK(SWIM, (obj), TYPE_SWIM)
> include/hw/block/swim.h:} SWIM;
> target/rx/cpu-qom.h:#define RXCPU(obj) \
> target/rx/cpu.h:typedef struct RXCPU RXCPU;
> target/s390x/translate.c:#define BD(N, BB, BD) { BB, 4, 0, FLD_C_b##N, FLD_O_b##N }, \
> hw/audio/ac97.c:} BD;
>
> The last one is a name clash in unrelated files; should not bother
> Coccinelle.
>
> The first three are due to QOM. By convention, the name of the
> function-like macro to convert to the QOM type is the QOM type in
> ALL_CAPS. Clash when the QOM type is ALL_CAPS already.
To add to this list, another problem I'm having is with QOM interfaces.
For example this line:
isa_bus_dma(bus, ISADMA(isa1), ISADMA(isa2));
The definitions are:
#define ISADMA(obj) INTERFACE_CHECK(IsaDma, (obj), TYPE_ISADMA)
typedef struct IsaDma IsaDma;
This is used as opaque pointer, so it compiles fine, but coccinelle is
confused because the actual 'struct IsaDma' is never defined.
The only 'documentation' I found about this is commit 00ed3da9b5 which
describes this as 'common practice'.
>
> Clearly, Coccinelle is getting spooked to easily. Regardless, three
> questions:
>
> 1. Are ALL_CAPS typedef names a good idea? We shout macros to tell
> readers "beware, possibly magic". Shouting other stuff as well
> undermines that.
>
> 2. The compiler is quite cool with us using the same name for a
> function-like macro and a not-function-like non-macro. But is it a good
> idea?
>
> 3. Do we want to work around Coccinelle's parsing flaw? Having it skip
> so much code is clearly suboptimal, and potentially dangerous. The
> obvious work-around is to rename the three problematic types so they
> contain at least one lower-case letter.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Function-like macro with the same name as a typedef confuses Coccinelle
2020-04-02 12:21 ` Peter Maydell
@ 2020-04-02 13:30 ` Markus Armbruster
2020-04-07 11:58 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2020-04-02 13:30 UTC (permalink / raw)
To: Peter Maydell
Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, QEMU Developers
Peter Maydell <peter.maydell@linaro.org> writes:
> On Thu, 2 Apr 2020 at 13:06, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> I discovered that Vladimir's auto-propagated-errp.cocci leaves
>> hw/arm/armsse.c unchanged, even though it clearly should change it.
>> Running spatch with --debug prints (among lots of other things)
>
>> Clearly, Coccinelle is getting spooked to easily.
>
> Is it worth asking on the coccinelle mailing list about whether
> coccinelle could be made to be less picky in this area ?
I guess we owe them the feedback. I'll look into minimizing the
reproducer.
>> Regardless, three questions:
>>
>> 1. Are ALL_CAPS typedef names a good idea? We shout macros to tell
>> readers "beware, possibly magic". Shouting other stuff as well
>> undermines that.
>>
>> 2. The compiler is quite cool with us using the same name for a
>> function-like macro and a not-function-like non-macro. But is it a good
>> idea?
>
> Probably not a great idea, and if we really only do it 3 times
> it's not too hard to change I suppose. I think this basically
> arises when the natural name for the struct happens to be all
> uppercase already because the device name is an acronym. We
> don't usually titlecase acronyms in structure names (eg
> we say 'SCSIBus', not 'ScsiBus'), and (legacy exceptions aside)
> we don't usually tack on a trailing 'State' or 'Device'
> to the main device state struct these days -- so if your device's
> natural name is an acronym then the struct ends up all-caps.
Plausible.
> If we don't like all-caps struct names then ideally we'd
> adjust one or the other of those conventions so we have a
> consistent way to avoid them.
I don't like the State suffix, it's four characters carrying zero bits
of information. The Device suffix at least tells me it's (supposedly)
an instance of TYPE_DEVICE.
I'd prefer title-casing acronyms when needed to avoid confusion with
macros.
> For 'ARMSSE' we could I suppose rename it 'ArmSSE', which would
> be in line with current corporate branding but out of line with
> most of the other places we use 'ARM' in a struct name :-)
Pick your poison :)
> Q: how many all-upper-case typedefs do we have in total (whether
> they have a clashing macro or not)? Your argument 1 would
> suggest we should look to change them all, not merely the ones
> Coccinelle trips over.
$ egrep -c '^[A-Z_]+\s+typedef' cxref
116
$ egrep '^[A-Z_]+\s+typedef' cxref
ACPIGPE typedef 107 include/hw/acpi/acpi.h typedef struct ACPIGPE ACPIGPE;
ACPIREGS typedef 108 include/hw/acpi/acpi.h typedef struct ACPIREGS ACPIREGS;
AES_KEY typedef 11 include/crypto/aes.h typedef struct aes_key_st AES_KEY;
AHCI_SG typedef 292 hw/ide/ahci_internal.h } QEMU_PACKED AHCI_SG;
ARMCPU typedef 57 target/arm/cpu-qom.h typedef struct ARMCPU ARMCPU;
ARMSSE typedef 218 include/hw/arm/armsse.h } ARMSSE;
ARMSSECPUID typedef 39 include/hw/misc/armsse-cpuid.h } ARMSSECPUID;
ARMSSEMHU typedef 42 include/hw/misc/armsse-mhu.h } ARMSSEMHU;
BD typedef 147 hw/audio/ac97.c } BD;
BIOS_TABLES_TEST typedef 77 tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h } BIOS_TABLES_TEST;
CCID_BULK_IN typedef 190 hw/usb/dev-smartcard-reader.c } CCID_BULK_IN;
CD typedef 490 hw/arm/smmuv3-internal.h } CD;
CHS typedef 515 tests/qtest/hd-geo-test.c } CHS;
CHST typedef 49 tests/qtest/hd-geo-test.c } CHST;
CIW typedef 40 include/hw/s390x/css.h } QEMU_PACKED CIW;
CMB typedef 64 include/hw/s390x/css.h } QEMU_PACKED CMB;
CMBE typedef 77 include/hw/s390x/css.h } QEMU_PACKED CMBE;
CMSDKAPBTIMER typedef 37 include/hw/timer/cmsdk-apb-timer.h } CMSDKAPBTIMER;
CMSDKAPBUART typedef 45 include/hw/char/cmsdk-apb-uart.h } CMSDKAPBUART;
CMS_VTOC typedef 188 pc-bios/s390-ccw/bootmap.h } __attribute__ ((packed)) CMS_VTOC;
CORE_ADDR typedef 1647 disas/hppa.c typedef unsigned int CORE_ADDR;
CPUTLB typedef 222 include/exec/cpu-defs.h } CPUTLB;
CPUTLB typedef 230 include/exec/cpu-defs.h typedef struct CPUTLB { } CPUTLB;
CRISCPU typedef 53 target/cris/cpu-qom.h typedef struct CRISCPU CRISCPU;
CRW typedef 201 include/hw/s390x/ioinst.h } CRW;
CURLAIOCB typedef 97 block/curl.c } CURLAIOCB;
DMAAIOCB typedef 85 dma-helpers.c } DMAAIOCB;
EHCI_STATES typedef 69 hw/usb/hcd-ehci.c } EHCI_STATES;
FIS typedef 355 tests/qtest/libqos/ahci.h } __attribute__((__packed__)) FIS;
FM_OPL typedef 90 hw/audio/fmopl.h } FM_OPL;
FPCR typedef 41 linux-user/arm/nwfpe/fpsr.h typedef unsigned int FPCR; /* type for floating point control register */
FPREG typedef 56 linux-user/arm/nwfpe/fpa11.h } FPREG;
FPSR typedef 40 linux-user/arm/nwfpe/fpsr.h typedef unsigned int FPSR; /* type for floating point status register */
GA_WTSINFOA typedef 1955 qga/commands-win32.c } GA_WTSINFOA;
GO typedef 110 include/hw/s390x/event-facility.h } QEMU_PACKED GO;
GUID typedef 18 contrib/elf2dmp/pdb.h } GUID;
HPPACPU typedef 50 target/hppa/cpu-qom.h typedef struct HPPACPU HPPACPU;
IDEDMA typedef 24 include/hw/ide/internal.h typedef struct IDEDMA IDEDMA;
IMAGE_DATA_DIRECTORY typedef 48 contrib/elf2dmp/pe.h } __attribute__ ((packed)) IMAGE_DATA_DIRECTORY;
IMAGE_DEBUG_DIRECTORY typedef 100 contrib/elf2dmp/pe.h } __attribute__ ((packed)) IMAGE_DEBUG_DIRECTORY;
IMAGE_DOS_HEADER typedef 33 contrib/elf2dmp/pe.h } __attribute__ ((packed)) IMAGE_DOS_HEADER;
IMAGE_FILE_HEADER typedef 43 contrib/elf2dmp/pe.h } __attribute__ ((packed)) IMAGE_FILE_HEADER;
IPMIBT typedef 66 include/hw/ipmi/ipmi_bt.h } IPMIBT;
IPMIKCS typedef 69 include/hw/ipmi/ipmi_kcs.h } IPMIKCS;
IRB typedef 132 include/hw/s390x/ioinst.h } IRB;
IRQMP typedef 64 hw/intc/grlib_irqmp.c } IRQMP;
LDL_VTOC typedef 156 pc-bios/s390-ccw/bootmap.h } __attribute__ ((packed)) LDL_VTOC;
LI typedef 91 include/libdecnumber/decNumberLocal.h typedef long int LI; /* for printf arguments only */
MDB typedef 126 include/hw/s390x/event-facility.h } QEMU_PACKED MDB;
MDBO typedef 121 include/hw/s390x/event-facility.h } QEMU_PACKED MDBO;
MDMSU typedef 157 include/hw/s390x/event-facility.h } QEMU_PACKED MDMSU;
MIPSCPU typedef 55 target/mips/cpu-qom.h typedef struct MIPSCPU MIPSCPU;
MSGUID typedef 81 block/vhdx.h } MSGUID;
MTO typedef 97 include/hw/s390x/event-facility.h } QEMU_PACKED MTO;
NCQFIS typedef 451 tests/qtest/libqos/ahci.h } __attribute__((__packed__)) NCQFIS;
NFSRPC typedef 76 block/nfs.c } NFSRPC;
OPL_CH typedef 54 hw/audio/fmopl.h } OPL_CH;
OPL_SLOT typedef 38 hw/audio/fmopl.h }OPL_SLOT;
OPL_TIMERHANDLER typedef 5 hw/audio/fmopl.h typedef void (*OPL_TIMERHANDLER)(void *param, int channel, double interval_Sec);
ORB typedef 142 include/hw/s390x/ioinst.h } ORB;
PDB_DS_HEADER typedef 34 contrib/elf2dmp/pdb.h } PDB_DS_HEADER;
PDB_DS_ROOT typedef 48 contrib/elf2dmp/pdb.h } PDB_DS_ROOT;
PDB_DS_TOC typedef 39 contrib/elf2dmp/pdb.h } PDB_DS_TOC;
PDB_STREAM_INDEXES typedef 193 contrib/elf2dmp/pdb.h } PDB_STREAM_INDEXES;
PDB_STREAM_INDEXES_OLD typedef 179 contrib/elf2dmp/pdb.h } PDB_STREAM_INDEXES_OLD;
PDB_SYMBOLS typedef 170 contrib/elf2dmp/pdb.h } PDB_SYMBOLS;
PDB_SYMBOLS_OLD typedef 149 contrib/elf2dmp/pdb.h } PDB_SYMBOLS_OLD;
PDB_SYMBOL_FILE typedef 110 contrib/elf2dmp/pdb.h } PDB_SYMBOL_FILE;
PDB_SYMBOL_FILE_EX typedef 124 contrib/elf2dmp/pdb.h } PDB_SYMBOL_FILE_EX;
PDB_SYMBOL_IMPORT typedef 138 contrib/elf2dmp/pdb.h } PDB_SYMBOL_IMPORT;
PDB_SYMBOL_RANGE typedef 85 contrib/elf2dmp/pdb.h } PDB_SYMBOL_RANGE;
PDB_SYMBOL_RANGE_EX typedef 97 contrib/elf2dmp/pdb.h } PDB_SYMBOL_RANGE_EX;
PDB_SYMBOL_SOURCE typedef 130 contrib/elf2dmp/pdb.h } PDB_SYMBOL_SOURCE;
PDB_TYPES typedef 75 contrib/elf2dmp/pdb.h } PDB_TYPES;
PDB_TYPES_OLD typedef 57 contrib/elf2dmp/pdb.h } PDB_TYPES_OLD;
PMCW typedef 98 include/hw/s390x/ioinst.h } PMCW;
PRD typedef 473 tests/qtest/libqos/ahci.h } __attribute__((__packed__)) PRD;
PSW typedef 17 pc-bios/s390-ccw/s390-arch.h } __attribute__ ((aligned(8))) PSW;
PSW typedef 46 target/s390x/cpu.h } PSW;
PTR typedef 12 include/disas/dis-asm.h typedef void *PTR;
QEDAIOCB typedef 150 block/qed.h } QEDAIOCB;
QEMUBH typedef 97 include/qemu/typedefs.h typedef struct QEMUBH QEMUBH;
QEMUFIFO typedef 68 ui/console.c } QEMUFIFO;
QFWCFG typedef 18 tests/qtest/libqos/fw_cfg.h typedef struct QFWCFG QFWCFG;
QJSON typedef 109 include/qemu/typedefs.h typedef struct QJSON QJSON;
QOHCI_PCI typedef 17 tests/qtest/usb-hcd-ohci-test.c typedef struct QOHCI_PCI QOHCI_PCI;
QSDHCI typedef 25 tests/qtest/libqos/sdhci.h typedef struct QSDHCI QSDHCI;
QSDHCI_PCI typedef 27 tests/qtest/libqos/sdhci.h typedef struct QSDHCI_PCI QSDHCI_PCI;
QSDHCI_PCI typedef 28 tests/qtest/libqos/x86_64_pc-machine.c typedef struct QSDHCI_PCI QSDHCI_PCI;
RADOSCB typedef 99 block/rbd.c } RADOSCB;
RBDAIOCB typedef 91 block/rbd.c } RBDAIOCB;
RING_IDX typedef 52 include/hw/xen/interface/io/ring.h typedef unsigned int RING_IDX;
RISCVCPU typedef 275 target/riscv/cpu.h } RISCVCPU;
RXCPU typedef 118 target/rx/cpu.h typedef struct RXCPU RXCPU;
SCCB typedef 65 pc-bios/s390-ccw/sclp.h } __attribute__((packed)) SCCB;
SCCB typedef 181 include/hw/s390x/sclp.h } QEMU_PACKED SCCB;
SCHIB typedef 124 include/hw/s390x/ioinst.h } QEMU_PACKED SCHIB;
SCSW typedef 28 include/hw/s390x/ioinst.h } SCSW;
SDBFIS typedef 389 hw/ide/ahci_internal.h } QEMU_PACKED SDBFIS;
SPARCCPU typedef 56 target/sparc/cpu-qom.h typedef struct SPARCCPU SPARCCPU;
STE typedef 485 hw/arm/smmuv3-internal.h } STE;
SWIM typedef 75 include/hw/block/swim.h } SWIM;
TCR typedef 164 target/arm/cpu.h } TCR;
TPMPPI typedef 21 hw/tpm/tpm_ppi.h } TPMPPI;
TZMPC typedef 43 include/hw/misc/tz-mpc.h typedef struct TZMPC TZMPC;
TZMSC typedef 77 include/hw/misc/tz-msc.h } TZMSC;
TZPPC typedef 75 include/hw/misc/tz-ppc.h typedef struct TZPPC TZPPC;
UART typedef 94 hw/char/grlib_apbuart.c } UART;
UHCI_QH typedef 156 hw/usb/hcd-uhci.c } UHCI_QH;
UHCI_TD typedef 151 hw/usb/hcd-uhci.c } UHCI_TD;
URI typedef 77 include/qemu/uri.h } URI;
VFIOBAR typedef 56 hw/vfio/pci.h } VFIOBAR;
VFIOVGA typedef 69 hw/vfio/pci.h } VFIOVGA;
VXHSAIOCB typedef 47 block/vxhs.c } VXHSAIOCB;
XENCONS_RING_IDX typedef 30 include/hw/xen/interface/io/console.h typedef uint32_t XENCONS_RING_IDX;
XHCITRB typedef 144 hw/usb/hcd-xhci.c } XHCITRB;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Function-like macro with the same name as a typedef confuses Coccinelle
2020-04-02 12:29 ` Philippe Mathieu-Daudé
@ 2020-04-02 13:47 ` Markus Armbruster
0 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2020-04-02 13:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Vladimir Sementsov-Ogievskiy, qemu-devel, Paolo Bonzini
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 4/2/20 2:06 PM, Markus Armbruster wrote:
>> I discovered that Vladimir's auto-propagated-errp.cocci leaves
>> hw/arm/armsse.c unchanged, even though it clearly should change it.
>> Running spatch with --debug prints (among lots of other things)
>>
>> let's go
>> -----------------------------------------------------------------------
>> -----------------------------------------------------------------------
>> parse error
>> = error in hw/arm/armsse.c; set verbose_parsing for more info
>> badcount: 7
>> bad: }
>> bad:
>> bad: static void nsccfg_handler(void *opaque, int n, int level)
>> bad: {
>> BAD:!!!!! ARMSSE *s = ARMSSE(opaque);
>> bad:
>> bad: s->nsccfg = level;
>> bad: }
>> parse error
>>
>> Alright, what's wrong with this? ARMSSE is both a function-like macro
>> and a typedef:
>>
>> #define ARMSSE(obj) OBJECT_CHECK(ARMSSE, (obj), TYPE_ARMSSE)
>>
>> typedef struct ARMSSE {
>> ...
>> } ARMSSE;
>>
>> This appears to spook Coccinelle badly enough to skip code using ARMSSE.
>>
>> If I rename the macro to ARMSSE_() just for testing, it produces the
>> transformation I'd expect.
>>
>> Grepping for typedef names is bothersome, so I used ctags -x to build a
>> cross reference table, and searched that for function-like macros
>> clashing with typedefs. Result:
>>
>> include/hw/arm/armsse.h:#define ARMSSE(obj) OBJECT_CHECK(ARMSSE, (obj), TYPE_ARMSSE)
>> include/hw/arm/armsse.h:} ARMSSE;
>> include/hw/block/swim.h:#define SWIM(obj) OBJECT_CHECK(SWIM, (obj), TYPE_SWIM)
>> include/hw/block/swim.h:} SWIM;
>> target/rx/cpu-qom.h:#define RXCPU(obj) \
>> target/rx/cpu.h:typedef struct RXCPU RXCPU;
>> target/s390x/translate.c:#define BD(N, BB, BD) { BB, 4, 0, FLD_C_b##N, FLD_O_b##N }, \
>> hw/audio/ac97.c:} BD;
>>
>> The last one is a name clash in unrelated files; should not bother
>> Coccinelle.
>>
>> The first three are due to QOM. By convention, the name of the
>> function-like macro to convert to the QOM type is the QOM type in
>> ALL_CAPS. Clash when the QOM type is ALL_CAPS already.
>
> To add to this list, another problem I'm having is with QOM interfaces.
>
> For example this line:
>
> isa_bus_dma(bus, ISADMA(isa1), ISADMA(isa2));
>
> The definitions are:
>
> #define ISADMA(obj) INTERFACE_CHECK(IsaDma, (obj), TYPE_ISADMA)
> typedef struct IsaDma IsaDma;
>
> This is used as opaque pointer, so it compiles fine, but coccinelle is
> confused because the actual 'struct IsaDma' is never defined.
Can you give me an example where Coccinelle gets confused by it?
> The only 'documentation' I found about this is commit 00ed3da9b5 which
> describes this as 'common practice'.
We discussed this in the thread
Subject: Issues around TYPE_INTERFACE
To: qemu-devel@nongnu.org
Date: Tue, 12 Mar 2019 11:50:54 +0100
Message-ID: <87h8c82woh.fsf@dusky.pond.sub.org>
Perhaps start with Paolo's reply:
Message-ID: <fdaa779c-ed79-647b-6038-3df2353fe502@redhat.com>
https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg00749.html
I just suggested to have a future docs/devel/qom.rst cover the topic:
Subject: Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Message-ID: <87369m3t18.fsf@dusky.pond.sub.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Function-like macro with the same name as a typedef confuses Coccinelle
2020-04-02 13:30 ` Markus Armbruster
@ 2020-04-07 11:58 ` Markus Armbruster
2020-04-07 13:47 ` Eric Blake
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2020-04-07 11:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, QEMU Developers
Markus Armbruster <armbru@redhat.com> writes:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Thu, 2 Apr 2020 at 13:06, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> I discovered that Vladimir's auto-propagated-errp.cocci leaves
>>> hw/arm/armsse.c unchanged, even though it clearly should change it.
>>> Running spatch with --debug prints (among lots of other things)
>>
>>> Clearly, Coccinelle is getting spooked to easily.
>>
>> Is it worth asking on the coccinelle mailing list about whether
>> coccinelle could be made to be less picky in this area ?
>
> I guess we owe them the feedback. I'll look into minimizing the
> reproducer.
https://systeme.lip6.fr/pipermail/cocci/2020-April/007097.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Function-like macro with the same name as a typedef confuses Coccinelle
2020-04-07 11:58 ` Markus Armbruster
@ 2020-04-07 13:47 ` Eric Blake
0 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2020-04-07 13:47 UTC (permalink / raw)
To: Markus Armbruster, Peter Maydell
Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, QEMU Developers
On 4/7/20 6:58 AM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Thu, 2 Apr 2020 at 13:06, Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>> I discovered that Vladimir's auto-propagated-errp.cocci leaves
>>>> hw/arm/armsse.c unchanged, even though it clearly should change it.
>>>> Running spatch with --debug prints (among lots of other things)
>>>
>>>> Clearly, Coccinelle is getting spooked to easily.
>>>
>>> Is it worth asking on the coccinelle mailing list about whether
>>> coccinelle could be made to be less picky in this area ?
>>
>> I guess we owe them the feedback. I'll look into minimizing the
>> reproducer.
>
> https://systeme.lip6.fr/pipermail/cocci/2020-April/007097.html
Wow - firefox refused to connect to that site until I downgraded to
permitting TLS 1.1. We should also tell them about their configuration
bug in not supporting TLS 1.2 for their archives.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-07 13:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 12:06 Function-like macro with the same name as a typedef confuses Coccinelle Markus Armbruster
2020-04-02 12:21 ` Peter Maydell
2020-04-02 13:30 ` Markus Armbruster
2020-04-07 11:58 ` Markus Armbruster
2020-04-07 13:47 ` Eric Blake
2020-04-02 12:29 ` Philippe Mathieu-Daudé
2020-04-02 13:47 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.