diff mbox

[qom,v3,1/4] cpu: Add wrapper to the set-pc() hook

Message ID 558C2A8A.8070202@suse.de
State New
Headers show

Commit Message

Andreas Färber June 25, 2015, 4:21 p.m. UTC 
Am 25.06.2015 um 13:21 schrieb Peter Maydell:
> On 25 June 2015 at 12:12, Andreas Färber <afaerber@suse.de> wrote:
>> Am 24.06.2015 um 21:11 schrieb Peter Maydell:
>>> On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> 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.

Okay, dropping the assertion:

Cheers,
Andreas
diff mbox

Patch

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index e7ee6c6..5db1ea3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -610,7 +610,6 @@  static inline void cpu_set_pc(CPUState *cpu, vaddr addr)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);

-    g_assert(cc->set_pc != NULL);
     cc->set_pc(cpu, addr);
 }