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

[Andreas Färber]

Am 24.06.2015 um 21:11 schrieb Peter Maydell:
> On 24 June 2015 at 19:09, Andreas Färber <address@hidden> wrote:
>> s/set-pc/set_pc/
>>
>> Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:
>>> Add a wrapper around the CPUClass::set_pc() hook.
>>>
>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>> ---
>>> changed since v2:
>>> drop "qom" from commit message subject.
>>> Add () to functions in commit messages.
>>> Drop error argument
>>> ---
>>>  include/qom/cpu.h | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>
>> Queuing on qom-cpu-next with the following change:
>>
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -604,16 +604,14 @@ static inline void cpu_unaligned_access(CPUState
>> *cpu, vaddr addr,
>>   * @cpu: The CPU to set the program counter for.
>>   * @addr: Program counter value.
>>   *
>> - * Set the program counter for a CPU. If there is no available
>> implementation
>> - * no action occurs.
>> + * Sets the program counter for a CPU.
>>   */
>>  static inline void cpu_set_pc(CPUState *cpu, vaddr addr)
>>  {
>>      CPUClass *cc = CPU_GET_CLASS(cpu);
>>
>> -    if (cc->set_pc) {
>> -        cc->set_pc(cpu, addr);
>> -    }
>> +    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.

We don't seem to have a clear line of when or whether to add such
assertions, so I can certainly drop the assertion line again.

Right now we have the qom-test iterating over (nearly) all machines, but
I'm not aware of all CPU models of all targets being covered by QTest
yet that would give us some more certainty.

Regards,
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)