[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: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook |
Date: |
Thu, 25 Jun 2015 18:21:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
Am 25.06.2015 um 13:21 schrieb 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.
Okay, dropping the assertion:
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);
}
Cheers,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
[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
Re: [Qemu-devel] [PATCH qom v3 0/4] qom-cpu: Wrap set_pc hook and use in bootloaders, Andreas Färber, 2015/06/24