Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

[Peter Maydell]

On Tue, 20 Oct 2020 at 12:17, Peng Liang <liangpeng10@huawei.com> wrote:
>
> On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
> > On 10/19/20 11:34 AM, Peng Liang wrote:
> >> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
> >> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
> >
> > It might be easy to add a Coccinelle script to avoid future errors.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
>
> I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
> end of VMStateDescription.fields.  For those who are not defined as
> compound literals, it works well.  However, I cannot make it work for
> those defined as compound literals.  And Julia doesn't think compound
> literals are supported currently[1].  So maybe currently it's hard to
> check the error using Coccinelle :(

I think we could probably significantly increase the chances that
people find "missing terminator" errors in the course of normal
debugging of their device if we made the terminator be something
other than "is field->name NULL". That condition is quite likely
to be satisfied by accident shortly after the real end-of-data
(because zeroes are easy to find in memory), whereas if the condition
is "field->flags is a magic number", for instance, then the chances of
it being satisfied by accident are very low, and so a simple "loop
through the field array until we find the end" is pretty likely to
hang/crash. (If we don't already have such a loop we might need to
add one in debug mode when a vmstate is registered.)

(This is why the REGINFO_SENTINEL used for Arm cpreg arrays is
not a simple all-zeroes value, incidentally.)

thanks
-- PMM