* [PATCH] microbit_i2c: Fix coredump when dump-vmstate
@ 2020-10-19 9:34 Peng Liang
2020-10-19 10:35 ` Philippe Mathieu-Daudé
2020-10-20 14:36 ` Peter Maydell
0 siblings, 2 replies; 9+ messages in thread
From: Peng Liang @ 2020-10-19 9:34 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: liangpeng10, peter.maydell, joel, xiexiangyou, zhang.zhanghailiang
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(+)
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()
},
};
--
2.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-19 9:34 [PATCH] microbit_i2c: Fix coredump when dump-vmstate Peng Liang
@ 2020-10-19 10:35 ` Philippe Mathieu-Daudé
2020-10-19 14:30 ` Peng Liang
2020-10-20 11:17 ` Peng Liang
2020-10-20 14:36 ` Peter Maydell
1 sibling, 2 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-19 10:35 UTC (permalink / raw)
To: Peng Liang, qemu-arm, qemu-devel
Cc: peter.maydell, joel, xiexiangyou, zhang.zhanghailiang
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()
> },
> };
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-19 10:35 ` Philippe Mathieu-Daudé
@ 2020-10-19 14:30 ` Peng Liang
2020-10-20 11:17 ` Peng Liang
1 sibling, 0 replies; 9+ messages in thread
From: Peng Liang @ 2020-10-19 14:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-arm, qemu-devel
Cc: peter.maydell, joel, xiexiangyou, zhang.zhanghailiang
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.
--
Thanks,
Peng
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-19 10:35 ` Philippe Mathieu-Daudé
2020-10-19 14:30 ` Peng Liang
@ 2020-10-20 11:17 ` Peng Liang
2020-10-20 11:27 ` Philippe Mathieu-Daudé
2020-10-20 11:27 ` Peter Maydell
1 sibling, 2 replies; 9+ messages in thread
From: Peng Liang @ 2020-10-20 11:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-arm, qemu-devel
Cc: peter.maydell, joel, xiexiangyou, zhang.zhanghailiang
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()
>> },
>> };
>>
>
>
> .
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-20 11:17 ` Peng Liang
@ 2020-10-20 11:27 ` Philippe Mathieu-Daudé
2020-10-20 12:07 ` Peng Liang
2020-10-20 11:27 ` Peter Maydell
1 sibling, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-20 11:27 UTC (permalink / raw)
To: Peng Liang, qemu-arm, qemu-devel
Cc: peter.maydell, joel, xiexiangyou, zhang.zhanghailiang
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()
>>> },
>>> };
>>>
>>
>>
>> .
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-20 11:27 ` Philippe Mathieu-Daudé
@ 2020-10-20 12:07 ` Peng Liang
0 siblings, 0 replies; 9+ messages in thread
From: Peng Liang @ 2020-10-20 12:07 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-arm, qemu-devel
Cc: peter.maydell, joel, xiexiangyou, zhang.zhanghailiang
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).
--
Thanks,
Peng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-20 11:17 ` Peng Liang
2020-10-20 11:27 ` Philippe Mathieu-Daudé
@ 2020-10-20 11:27 ` Peter Maydell
2020-10-20 12:19 ` Peng Liang
1 sibling, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-10-20 11:27 UTC (permalink / raw)
To: Peng Liang
Cc: zhanghailiang, QEMU Developers, Xiangyou Xie, qemu-arm,
Joel Stanley, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-20 11:27 ` Peter Maydell
@ 2020-10-20 12:19 ` Peng Liang
0 siblings, 0 replies; 9+ messages in thread
From: Peng Liang @ 2020-10-20 12:19 UTC (permalink / raw)
To: Peter Maydell
Cc: zhanghailiang, QEMU Developers, Xiangyou Xie, qemu-arm,
Joel Stanley, Philippe Mathieu-Daudé
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?
--
Thanks,
Peng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
2020-10-19 9:34 [PATCH] microbit_i2c: Fix coredump when dump-vmstate Peng Liang
2020-10-19 10:35 ` Philippe Mathieu-Daudé
@ 2020-10-20 14:36 ` Peter Maydell
1 sibling, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2020-10-20 14:36 UTC (permalink / raw)
To: Peng Liang
Cc: zhanghailiang, qemu-arm, QEMU Developers, Xiangyou Xie, Joel Stanley
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-20 14:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-19 9:34 [PATCH] microbit_i2c: Fix coredump when dump-vmstate Peng Liang
2020-10-19 10:35 ` Philippe Mathieu-Daudé
2020-10-19 14:30 ` Peng Liang
2020-10-20 11:17 ` Peng Liang
2020-10-20 11:27 ` Philippe Mathieu-Daudé
2020-10-20 12:07 ` Peng Liang
2020-10-20 11:27 ` Peter Maydell
2020-10-20 12:19 ` Peng Liang
2020-10-20 14:36 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).