[Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Liviu Ionescu]

I just updated GNU ARM Eclipse QEMU to 2.5.1 and initially I had some problems, main() failed quite early, in the first call to `find_default_machine()`.

After several debug sessions, I identified the problem to be a null pointer when a referred interface is not defined. In my Cortex-M specific configuration, `arm/boot.c` was not included in the build, but TYPE_ARM_LINUX_BOOT_IF was referred by TYPE_ARM_GIC_COMMON, the parent of my NVIC object.

I guess the problem is in `object.c:type_initialize()`, which does not check the pointer returned by:

`TypeImpl *t = type_get_by_name(ti->interfaces[i].typename)` 

and calls 

`type_initialize_interface(ti, t, t);` 

with the null pointers.


Normally an assert would be enough, but I don't know exactly which of your asserts better fit here, so I would abstain from submitting a patch.


Regards,

Liviu

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Peter Maydell]

On 2 April 2016 at 23:15, Liviu Ionescu <ilg@livius.net> wrote:
> I just updated GNU ARM Eclipse QEMU to 2.5.1 and initially
>I had some problems, main() failed quite early, in the first
> call to `find_default_machine()`.
>
> After several debug sessions, I identified the problem to
> be a null pointer when a referred interface is not defined.
> In my Cortex-M specific configuration, `arm/boot.c` was not
> included in the build, but TYPE_ARM_LINUX_BOOT_IF was referred
> by TYPE_ARM_GIC_COMMON, the parent of my NVIC object.
>
> I guess the problem is in `object.c:type_initialize()`, which
> does not check the pointer returned by:
>
> `TypeImpl *t = type_get_by_name(ti->interfaces[i].typename)`
>
> and calls
>
> `type_initialize_interface(ti, t, t);`
>
> with the null pointers.

Yeah, referring to an interface that doesn't exist is a
program bug (or in this case a build config error, though
since hw/arm/boot.o is in obj-y it should always be built),
but we could assert on it rather than just crashing.
g_assert() will do.

thanks
-- PMM

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Liviu Ionescu]

> On 03 Apr 2016, at 15:28, Peter Maydell <peter.maydell@linaro.org> wrote:

> since hw/arm/boot.o is in obj-y it should always be built,

not necessarily, in my build configuration I have if's that exclude most of the files non related to Cortex-M. 

in previous versions boot.o was not needed. in 2.5.1 you introduced a non-obvious dependency to it and the build passed, but the program crashed.

> but we could assert on it rather than just crashing.

my suggestion is to assert, it is easier to debug a failed assertion.


regards,

Liviu

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Peter Maydell]

On 3 April 2016 at 16:40, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 03 Apr 2016, at 15:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> since hw/arm/boot.o is in obj-y it should always be built,
>
> not necessarily, in my build configuration I have if's that
> exclude most of the files non related to Cortex-M.

boot.o is in obj-y; it is (I think) impossible to
build a v7M supporting QEMU without boot.o unless you've modified
the makefiles for some reason. That would not be a configuration
issue or a bug in QEMU, but a problem with your local modifications.

> in previous versions boot.o was not needed. in 2.5.1 you
> introduced a non-obvious dependency to it and the build
> passed, but the program crashed.
>
>> but we could assert on it rather than just crashing.
>
> my suggestion is to assert, it is easier to debug a failed
> assertion.

Yes, I agree.

thanks
-- PMM

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Liviu Ionescu]

> On 03 Apr 2016, at 19:43, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 3 April 2016 at 16:40, Liviu Ionescu <ilg@livius.net> wrote:
>> 
>>> On 03 Apr 2016, at 15:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 
>>> since hw/arm/boot.o is in obj-y it should always be built,
>> 
>> not necessarily, in my build configuration I have if's that
>> exclude most of the files non related to Cortex-M.
> 
> boot.o is in obj-y; it is (I think) impossible to
> build a v7M supporting QEMU without boot.o unless you've modified
> the makefiles for some reason.

that's nothing more than the usual make magic, most of the makefiles look like this:


ifeq ($(CONFIG_GNU_ARM_ECLIPSE),n)
obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
obj-$(CONFIG_DIGIC) += digic_boards.o
obj-y += integratorcp.o mainstone.o musicpal.o nseries.o
obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
obj-$(CONFIG_ACPI) += virt-acpi-build.o
obj-y += netduino2.o
obj-y += sysbus-fdt.o

obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
obj-$(CONFIG_DIGIC) += digic.o
obj-y += omap1.o omap2.o strongarm.o
obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
endif


regards,

Liviu

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Peter Maydell]

On 3 April 2016 at 17:57, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 03 Apr 2016, at 19:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 3 April 2016 at 16:40, Liviu Ionescu <ilg@livius.net> wrote:
>>>
>>>> On 03 Apr 2016, at 15:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>>> since hw/arm/boot.o is in obj-y it should always be built,
>>>
>>> not necessarily, in my build configuration I have if's that
>>> exclude most of the files non related to Cortex-M.
>>
>> boot.o is in obj-y; it is (I think) impossible to
>> build a v7M supporting QEMU without boot.o unless you've modified
>> the makefiles for some reason.
>
> that's nothing more than the usual make magic, most of the makefiles look like this:
>
>
> ifeq ($(CONFIG_GNU_ARM_ECLIPSE),n)

This is you changing QEMU to not compile a file that was
previously always compiled. If you do that then it's
not surprising if things might break when you move to an
upstream version, that's all.

Ideally we'd have more CONFIG_SOME_DEVICE and CONFIG_SOME_BOARD
checks here the way we do for the more recent boards like
the Xilinx and iMX ones, then you could disable boards
you didn't want in the config file.

thanks
-- PMM

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Liviu Ionescu]

> On 03 Apr 2016, at 20:01, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> ... This is you changing QEMU to not compile a file that was
> previously always compiled. If you do that then it's
> not surprising if things might break when you move to an
> upstream version, that's all.

well, to summarise, I notified you that in certain conditions, due to an non-obvious dependency that does not break the build when not met, the existing code crashes at run time with a segmentation fault, and I suggested that an assert might help developers to save some debugging time. 

if you want to add this assert, ok; if you don't, and prefer to permanently blame my fork for various reasons, it's also fine, I don't mind, I fix it in my fork and move forward.


regards,

Liviu

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Peter Maydell]

On 3 April 2016 at 18:56, Liviu Ionescu <ilg@livius.net> wrote:
> well, to summarise, I notified you that in certain conditions,
> due to an non-obvious dependency that does not break the build
> when not met, the existing code crashes at run time with a
> segmentation fault, and I suggested that an assert might help
> developers to save some debugging time.

Which I completely agree with. If you want to send a patch
to add the assert I'm happy to review it.

thanks
-- PMM

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Liviu Ionescu]

> On 04 Apr 2016, at 00:30, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 3 April 2016 at 18:56, Liviu Ionescu <ilg@livius.net> wrote:
>> well, to summarise, I notified you that in certain conditions,
>> due to an non-obvious dependency that does not break the build
>> when not met, the existing code crashes at run time with a
>> segmentation fault, and I suggested that an assert might help
>> developers to save some debugging time.
> 
> Which I completely agree with. If you want to send a patch
> to add the assert I'm happy to review it.

I guess the original report was clear enough for an active commiter to easily understand the problem and fix it, in significantly less iterations that I would need to produce a compliant and acceptable patch (reaching this point already took 9 messages). :-(

regards,

Liviu

Re: [Qemu-devel] segmentation fault in object.c:type_initialize_interface() if interface not defined

[Peter Maydell]

On 3 April 2016 at 22:42, Liviu Ionescu <ilg@livius.net> wrote:
>> On 04 Apr 2016, at 00:30, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Which I completely agree with. If you want to send a patch
>> to add the assert I'm happy to review it.
>
> I guess the original report was clear enough for an active commiter
> to easily understand the problem and fix it, in significantly less
> iterations that I would need to produce a compliant and acceptable
> patch (reaching this point already took 9 messages). :-(

There are many many bits of suboptimal code, things that
could be improved and outright bugs in QEMU. Fixing any
of them takes time, which is why as an open source project
we encourage people to contribute the fixes they personally
run into and find they need. (This is pretty much what anybody
who is an active committer is already doing.)

thanks
-- PMM