On 21/01/20 17:15, Alex Bennée wrote:
>
> Richard Henderson <richard.hender...@linaro.org> writes:
>
>> On 1/21/20 12:13 AM, Alex Bennée wrote:
>>>
>>> Richard Henderson <richard.hender...@linaro.org> writes:
>>>
>>>> On 1/20/20 1:48 AM, Alex Bennée wrote:
>>>>>> + default:
>>>>>> + sigsegv:
>>>>>
>>>>> this label looks a little extraneous.
>>>>>
>>>>> Otherwise:
>>>>>
>>>>> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
>>>>>
>>>>
>>>> Look a little further down:
>>>>
>>>>> + default:
>>>>> + sigsegv:
>>>>> + /* Like force_sig(SIGSEGV). */
>>>>> + gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> + * Validate the return address.
>>>>> + * Note that the kernel treats this the same as an invalid entry
>>>>> point.
>>>>> + */
>>>>> + if (get_user_u64(caller, env->regs[R_ESP])) {
>>>>> + goto sigsegv;
>>>>> + }
>>>
>>> Wouldn't this read better:
>>>
>>> /*
>>> * Validate the entry point. We have already validated the page
>>> * during translation, now verify the offset.
>>> */
>>> switch (env->eip & ~TARGET_PAGE_MASK) {
>>> case 0x000:
>>> syscall = TARGET_NR_gettimeofday;
>>> break;
>>> case 0x400:
>>> syscall = TARGET_NR_time;
>>> break;
>>> case 0x800:
>>> syscall = TARGET_NR_getcpu;
>>> break;
>>> default:
>>> syscall = -1;
>>> break;
>>> }
>>>
>>> /*
>>> * If we have an invalid entry point or an invalid return address we
>>> * generate a SIGSEG.
>>> */
>>> if (syscall < 0 || get_user_u64(caller, env->regs[R_ESP])) {
>>> gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>> return;
>>> }
>>>
>>
>> Only if you have a violent goto allergy.
>
> gotos have their place but jumping backwards is confusing to eye. If the
> compiler want to mess with layout after then it is free to do so.
I agree, if anything I'd place the sigsegv label at the end of the
function but Alex's version is just fine too.
Paolo
