Message ID | 20230208000743.79415-7-philmd@linaro.org |
---|---|
State | New |
Headers | show |
Series | hw/ide: Untangle ISA/PCI abuses of ide_init_ioport() | expand |
On 2/7/23 14:07, Philippe Mathieu-Daudé wrote: > The previous commit removed the single call to > isa_register_portio_list() with dev=NULL. To be > sure we won't reintroduce such weird (ab)use, > add an assertion. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> I wonder how much use of __attribute__((nonnull)) we should be making. I realize we'd probably want to add -fno-delete-null-pointer-checks if we make too much use of that. r~
On 8/2/23 20:47, Richard Henderson wrote: > On 2/7/23 14:07, Philippe Mathieu-Daudé wrote: >> The previous commit removed the single call to >> isa_register_portio_list() with dev=NULL. To be >> sure we won't reintroduce such weird (ab)use, >> add an assertion. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > I wonder how much use of __attribute__((nonnull)) we should be making. __attribute__((nonnull)) is compile-time, but seems weaker than the good old runtime assert(): void a0(void *ptr) { assert(ptr); } __attribute__((nonnull)) void a1(void *ptr) { // can no use assert(ptr) because compiler warning } void b0(void *x) { a(NULL); // runtime assertion } void b(void *x) { a1(NULL); // compile error } void c0(void *x) { a0(x); } void c1(void *x) { a1(x); } void d0(void *x) { c0(NULL); // runtime assertion } void d1(void *x) { c1(NULL); // no compile error, no assertion! } > I realize we'd probably want to add -fno-delete-null-pointer-checks if > we make too much use of that.
On 2/14/23 00:18, Philippe Mathieu-Daudé wrote: > __attribute__((nonnull)) void a1(void *ptr) > { > // can no use assert(ptr) because compiler warning > } I briefly glossed over that... >> I realize we'd probably want to add -fno-delete-null-pointer-checks if we make too much ... here. The compiler warning should go away with the right flag. r~
On 14/2/23 19:49, Richard Henderson wrote: > On 2/14/23 00:18, Philippe Mathieu-Daudé wrote: >> __attribute__((nonnull)) void a1(void *ptr) >> { >> // can no use assert(ptr) because compiler warning >> } > > I briefly glossed over that... > >>> I realize we'd probably want to add -fno-delete-null-pointer-checks >>> if we make too much > > ... here. The compiler warning should go away with the right flag. Doh, got it now!
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c index 95fc1ba5f7..3d1996c115 100644 --- a/hw/isa/isa-bus.c +++ b/hw/isa/isa-bus.c @@ -107,7 +107,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan) static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport) { - if (dev && (dev->ioport_id == 0 || ioport < dev->ioport_id)) { + if (dev->ioport_id == 0 || ioport < dev->ioport_id) { dev->ioport_id = ioport; } } @@ -123,6 +123,8 @@ int isa_register_portio_list(ISADevice *dev, const MemoryRegionPortio *pio_start, void *opaque, const char *name) { + assert(dev); + if (!isabus) { return -ENODEV; }
The previous commit removed the single call to isa_register_portio_list() with dev=NULL. To be sure we won't reintroduce such weird (ab)use, add an assertion. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/isa/isa-bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)