Re: Function-like macro with the same name as a typedef confuses Coccinelle

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Thu, 2 Apr 2020 at 13:06, Markus Armbruster <address@hidden> 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;