diff mbox series

[6/7] hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device

Message ID 20230208000743.79415-7-philmd@linaro.org
State Superseded
Headers show
Series hw/ide: Untangle ISA/PCI abuses of ide_init_ioport() | expand

Commit Message

Philippe Mathieu-Daudé Feb. 8, 2023, 12:07 a.m. UTC 
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(-)

Comments

Richard Henderson Feb. 8, 2023, 7:47 p.m. UTC | #1 
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~
Philippe Mathieu-Daudé Feb. 14, 2023, 10:18 a.m. UTC | #2 
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.
Richard Henderson Feb. 14, 2023, 6:49 p.m. UTC | #3 
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~
Philippe Mathieu-Daudé Feb. 15, 2023, 7:12 a.m. UTC | #4 
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 mbox series

Patch

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;
     }