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

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 <crosthwaite.peter@gmail.com>
> ---
> 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:


 /**

Regards,
Andreas

On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> 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 <crosthwaite.peter@gmail.com>
>> ---
>> 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...

-- PMM

Am 24.06.2015 um 21:11 schrieb Peter Maydell:
> On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> 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 <crosthwaite.peter@gmail.com>
>>> ---
>>> 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

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.

thanks
-- PMM