Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Peter Maydell]

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