Re: [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386

[Richard Henderson]

On 06/14/2016 04:58 AM, Peter Maydell wrote:
>> +    greg_t *pcreg = &uc->uc_mcontext.gregs[REG_EIP];
> 
> user-exec.c has
> #ifndef REG_EIP
> /* for glibc 2.1 */
> #define REG_EIP    EIP
> #endif
> 
> Do we still care about glibc 2.1 ? (Probably not, 2.2 was
> released fifteen years ago now...)
> 

Heh.  I would say not.  We've got other much more recent requirements.

>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +        .global safe_syscall_base
>> +        .global safe_syscall_start
>> +        .global safe_syscall_end
>> +        .type   safe_syscall_base, @function
> 
> I guess 4-space indent would match the rest of QEMU...

This is assembler not C.  My brain is tied to a 1-tab indent.

>> +        .cfi_rel_offset esi, 0
>> +       push    %edi
> 
> Odd indentation here.

Yeah, that's mixing code from your x86_64 version which uses spaces not tabs.

>> +        cmp    $0, (%eax)
>> +        mov     8+16(%esp), %eax        /* syscall number */
>> +        jnz     1f
> 
> Any particular reason for doing the jump after the mov?

No.  Indeed, recent cpus will fuse the cmp+jnz so they're better off together.

>> +        .cfi_def_cfa_offset 4
> 
> Shouldn't these all be ".cfi_adjust_cfa_offset -4" ? That's what glibc
> uses AFAICT.

Typo.  Good catch.

>> +        .cfi_remember_state
> 
> We don't need to remember state here I think.

Correct.  Cut and paste.

> Other than some trivialities like order of register push/pops
> this is virtually identical code to the version I had, so it
> must be right :-)

Heh.


r~