Re: [PULL 2/2] core/register: Specify instance_size in the TypeInfo

[Eduardo Habkost]

On Tue, Sep 29, 2020 at 01:55:35PM +0100, Peter Maydell wrote:
> On Sun, 27 Sep 2020 at 15:00, Alistair Francis <alistair.francis@wdc.com> 
> wrote:
> >
> > Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Message-Id: 
> > <4cf1beb7dafb9143c261d266557d3173bf160524.1598376594.git.alistair.francis@wdc.com>
> > ---
> > @@ -269,13 +258,18 @@ static RegisterInfoArray 
> > *register_init_block(DeviceState *owner,
> >          int index = rae[i].addr / data_size;
> >          RegisterInfo *r = &ri[index];
> >
> > -        *r = (RegisterInfo) {
> > -            .data = data + data_size * index,
> > -            .data_size = data_size,
> > -            .access = &rae[i],
> > -            .opaque = owner,
> > -        };
> > -        register_init(r);
> > +        if (data + data_size * index == 0 || !&rae[i]) {
> > +            continue;
> 
> Coverity thinks (CID 1432800) that this is dead code, because
> "data + data_size * index" can never be NULL[*]. What was this
> intending to test for ? (maybe data == NULL? Missing dereference
> operator ?)

I believe the original check in the old register_init() function
were just to make the function more flexible by allowing NULL
arguments, but it was always unnecessary.  We have 4 callers of
register_init_block*() and neither rae or data are NULL on those
calls.

> 
> [*] The C spec is quite strict about what valid pointer arithmetic
> is; in particular adding to a NULL pointer is undefined behaviour,
> and pointer arithmetic that overflows and wraps around is
> undefined behaviour, so there's no way to get a 0 result from
> "ptr + offset" without the expression being UB.
> 
> thanks
> -- PMM
> 

-- 
Eduardo