[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hoo
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook |
Date: |
Thu, 25 Jun 2015 12:21:58 +0100 |
On 25 June 2015 at 12:12, Andreas Färber <address@hidden> wrote:
> Am 24.06.2015 um 21:11 schrieb Peter Maydell:
>> On 24 June 2015 at 19:09, Andreas Färber <address@hidden> wrote:
>>> + g_assert(cc->set_pc != NULL);
>>> + cc->set_pc(cpu, addr);
>>> }
>>
>> Do we need this assert? If it would have fired
>> then we'll just crash immediately calling the null pointer,
>> so it's not like it's guarding against a more subtle failure
>> at a later point...
>
> There seemed uncertainty whether all corner cases of the 17 targets
> implement set_pc for all subclasses. By my reading, g_assert() calls
> g_assertion_message_expr(), which is marked G_GNUC_NORETURN - and I
> assume it to abort after printing the message, raising a signal and
> either exiting the process or falling back to an attached debugger.
>
> It may be unnecessary, but I don't see it calling the null pointer here.
What I mean is:
* with the assert, QEMU will die in this function if cc->set_pc
is NULL, in an easily debuggable way
* without the assert, QEMU will still die in this function if
cc->set_pc is NULL, in an easily debuggable way
So the assert doesn't hurt, but it doesn't really gain anything IMHO.
Assertions are most useful when they turn something that would be
a really confusing failure much later in execution into an easily
debuggable crash earlier on, I think.
Still, this is a very minor thing, so it's a personal taste/style
question, as you say. You can leave the assert in or remove it,
whichever you prefer.
thanks
-- PMM
[Qemu-devel] [PATCH qom v3 2/4] gdbstub: Use cpu_set_pc() helper, Peter Crosthwaite, 2015/06/23
[Qemu-devel] [PATCH qom v3 3/4] arm: boot: Use cpu_set_pc(), Peter Crosthwaite, 2015/06/23
[Qemu-devel] [PATCH qom v3 4/4] microblaze: boot: Use cpu_set_pc(), Peter Crosthwaite, 2015/06/23