| Message ID | 20201019093401.2993833-1-liangpeng10@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | microbit_i2c: Fix coredump when dump-vmstate | expand |
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> > > Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer detection") > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > hw/i2c/microbit_i2c.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c > index 802473982082..e92f9f84ea81 100644 > --- a/hw/i2c/microbit_i2c.c > +++ b/hw/i2c/microbit_i2c.c > @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate = { > .fields = (VMStateField[]) { > VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, MICROBIT_I2C_NREGS), > VMSTATE_UINT32(read_idx, MicrobitI2CState), > + VMSTATE_END_OF_LIST() > }, > }; > >
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> > >> >> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer >> detection") >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> hw/i2c/microbit_i2c.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c >> index 802473982082..e92f9f84ea81 100644 >> --- a/hw/i2c/microbit_i2c.c >> +++ b/hw/i2c/microbit_i2c.c >> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate >> = { >> .fields = (VMStateField[]) { >> VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, >> MICROBIT_I2C_NREGS), >> VMSTATE_UINT32(read_idx, MicrobitI2CState), >> + VMSTATE_END_OF_LIST() >> }, >> }; >> > > > . Thanks! I'll try to add one.
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 :( Thanks for my colleague Biaoxiang Ye, who wrote a shell script to find the errors, I didn't find other similar errors. [1] https://lore.kernel.org/cocci/alpine.DEB.2.22.394.2010201143330.2736@hadrien/T/#t Thanks, Peng >> >> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer >> detection") >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >> --- >> hw/i2c/microbit_i2c.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c >> index 802473982082..e92f9f84ea81 100644 >> --- a/hw/i2c/microbit_i2c.c >> +++ b/hw/i2c/microbit_i2c.c >> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate >> = { >> .fields = (VMStateField[]) { >> VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, >> MICROBIT_I2C_NREGS), >> VMSTATE_UINT32(read_idx, MicrobitI2CState), >> + VMSTATE_END_OF_LIST() >> }, >> }; >> > > > .
On 10/20/20 1:17 PM, Peng Liang 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 :( Interesting. > > Thanks for my colleague Biaoxiang Ye, who wrote a shell script to find > the errors, I didn't find other similar errors. Thanks for giving it a try. We could commit and run the script in a gitlab-ci job to avoid such regressions. > > [1] > https://lore.kernel.org/cocci/alpine.DEB.2.22.394.2010201143330.2736@hadrien/T/#t > > Thanks, > Peng > >>> >>> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer >>> detection") >>> Reported-by: Euler Robot <euler.robot@huawei.com> >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com> >>> --- >>> hw/i2c/microbit_i2c.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c >>> index 802473982082..e92f9f84ea81 100644 >>> --- a/hw/i2c/microbit_i2c.c >>> +++ b/hw/i2c/microbit_i2c.c >>> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate >>> = { >>> .fields = (VMStateField[]) { >>> VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, >>> MICROBIT_I2C_NREGS), >>> VMSTATE_UINT32(read_idx, MicrobitI2CState), >>> + VMSTATE_END_OF_LIST() >>> }, >>> }; >>> >> >> >> . >
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
On 10/20/2020 7:27 PM, Philippe Mathieu-Daudé wrote: > On 10/20/20 1:17 PM, Peng Liang 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 :( > > Interesting. > >> >> Thanks for my colleague Biaoxiang Ye, who wrote a shell script to find >> the errors, I didn't find other similar errors. > > Thanks for giving it a try. We could commit and run the script > in a gitlab-ci job to avoid such regressions. > The script will report all fields not defined as compound literals as errors (the number is much smaller than that of all VMStateDescription.fields).
On 10/20/2020 7:27 PM, Peter Maydell wrote: > 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 > . > I found that field->flags is a bit-or field, so maybe all 0xf or other magic number is still meaningful? Can we use field->version_id or field->struct_version_id as the condition? I found they are all int type but used as non-negative, so can we use field->version_id/field->struct_version_id == magic number (for example, -1) as a sentinel?
On Mon, 19 Oct 2020 at 10:36, Peng Liang <liangpeng10@huawei.com> wrote: > > VMStateDescription.fields should be end with VMSTATE_END_OF_LIST(). > However, microbit_i2c_vmstate doesn't follow it. Let's change it. > > Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer detection") > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- Applied to target-arm.next, thanks. -- PMM
diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c index 802473982082..e92f9f84ea81 100644 --- a/hw/i2c/microbit_i2c.c +++ b/hw/i2c/microbit_i2c.c @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate = { .fields = (VMStateField[]) { VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, MICROBIT_I2C_NREGS), VMSTATE_UINT32(read_idx, MicrobitI2CState), + VMSTATE_END_OF_LIST() }, };
VMStateDescription.fields should be end with VMSTATE_END_OF_LIST(). However, microbit_i2c_vmstate doesn't follow it. Let's change it. Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer detection") Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- hw/i2c/microbit_i2c.c | 1 + 1 file changed, 1 insertion(+)