On 28/10/2021 08:31, Roger Pau Monné wrote: > On Wed, Oct 27, 2021 at 09:07:13PM +0100, Andrew Cooper wrote: >> GCC master (nearly version 12) complains: >> >> hvm.c: In function 'hvm_gsi_eoi': >> hvm.c:905:10: error: the comparison will always evaluate as 'true' for the >> address of 'dpci' will never be NULL [-Werror=address] >> 905 | if ( !pirq_dpci(pirq) ) >> | ^ >> In file included from /local/xen.git/xen/include/xen/irq.h:73, >> from /local/xen.git/xen/include/xen/pci.h:13, >> from /local/xen.git/xen/include/asm/hvm/io.h:22, >> from /local/xen.git/xen/include/asm/hvm/domain.h:27, >> from /local/xen.git/xen/include/asm/domain.h:7, >> from /local/xen.git/xen/include/xen/domain.h:8, >> from /local/xen.git/xen/include/xen/sched.h:11, >> from /local/xen.git/xen/include/xen/event.h:12, >> from hvm.c:20: >> /local/xen.git/xen/include/asm/irq.h:140:34: note: 'dpci' declared here >> 140 | struct hvm_pirq_dpci dpci; >> | ^~~~ >> >> The location marker is unhelpfully positioned and upstream may get around to >> fixing it. The complaint is intended to be: >> >> if ( !((pirq) ? &(pirq)->arch.hvm.dpci : NULL) ) >> ^~~~~~~~~~~~~~~~~~~~~~ >> >> which is a hint that the code is should be simplified to just: >> >> if ( !pirq ) > I likely need more coffee, but doesn't this exploit how the macro > (pirq_dpci) is currently coded?
The way pirq_dpci() is currently coded, this is nonsense, which GCC is now highlighting. It would be a very different thing if the logic said: struct pirq *pirq = pirq_info(d, gsi); struct hvm_pirq_dpci *dpci = pirq_dpci(pirq); /* Check if GSI is actually mapped. */ if ( !dpci ) return; but it doesn't. Getting a non-null pirq pointer from pirq_info(d, gsi) does identify that the GSI is mapped, and the dpci nested structure is not used in this function. I would expect this property to remain true even if we alter the data layout. ~Andrew
