Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

darwin syscall implementation is wrong #51

Closed
axw opened this issue Sep 25, 2013 · 9 comments
Closed

darwin syscall implementation is wrong #51

axw opened this issue Sep 25, 2013 · 9 comments

Comments

@axw
Copy link
Member

axw commented Sep 25, 2013

See comments of #47

@quarnster
Copy link
Contributor

I'm looking at this now.

In the asm_linux_amd64.ll version, we have this %4 = call {i64, i64} asm sideeffect "syscall\0A", "={ax},={dx},{ax},{di},{si},{dx},{r10},{r8},{r9}"(i64 %0, i64 %1, i64 %2, i64 %3, i64 0, i64 0, i64 0) nounwind, but isn't this sideeffect list incomplete? According to http://wiki.osdev.org/Calling_Conventions the System V X86_64 call convention scratch registers are rax, rdi, rsi, rdx, rcx, r8, r9, r10, r11. Are cx and r11 intentionally left out?

@quarnster
Copy link
Contributor

Nevermind, that appears to be the register parameter mapping and sideeffect automatically knows that side effects happen. I spot the comment in Go's asm_linux_amd64.s about the parameter registers differing, I'll add a comment in LLGO's version too just to make this clear that the argument registers used are intentional.

@axw
Copy link
Member Author

axw commented Sep 26, 2013

Nevermind, that appears to be the register parameter mapping and sideeffect automatically knows that side effects happen.

Really? I though "sideeffect" is just saying that the code may not be moved/optimised (think "volatile" in C). According to the ABI (PDF), we should have a clobber for rcx and r11.

I spot the comment in Go's asm_linux_amd64.s about the parameter registers differing, I'll add a comment in LLGO's version too just to make this clear that the argument registers used are intentional.

Thanks.

@quarnster
Copy link
Contributor

Nevermind, that appears to be the register parameter mapping and sideeffect automatically knows that side effects happen.

Really? I though "sideeffect" is just saying that the code may not be moved/optimised (think "volatile" in C). According to the ABI (PDF), we should have a clobber for rcx and r11.

Volatile in C means to me that the variable defined as such can be modified by another process/thread/hardware/etc. I've only ever used it with pointers meaning that the compiler can't just read the value into a register and keep using the register as the underlying value stored in memory could have been changed.

In the same way the registers are volatile in the sense that after the call they might have changed and we can't trust that the value we put in them before the call are the same values after the call.

http://llvm.org/docs/LangRef.html#inline-assembler-expressions says:

This value represents the inline assembler as a string (containing the instructions to emit), a list of operand constraints (stored as a string),
...
Inline asms with side effects not visible in the constraint list must be marked as having side effects. This is done through the use of the ‘sideeffect‘ keyword

So the list after the instruction I read as the operand constraints list, especially due to the weird ordering of r10 which is called out as intentional in Go's asm_linux_amd64.s. And then the use of the sideeffect keyword I read as "volatile barrier" if I'm allowed to invent a new term ;)

Do you know the motivation behind the -4095 constant? If I understand the code correctly this will make return values from -1 to -4094 fail and the rest pass. The man page just states that "The return value is defined by the system call being invoked. In general, a 0 return value indicates success. A -1 return value indicates an error, and an error code is stored in errno."

Negative values from -1 to -4094 (or their uint64 equivalent) are unlikely to mean anything other than an error, but it would be good to know if there was any specific reason for choosing -4095 as the cut-off point of sane values specifically.

@axw
Copy link
Member Author

axw commented Sep 26, 2013

I suggest taking a look at that PDF I linked, specifically section A.2.1. I think it'll answer your questions authoritatively.

So the list after the instruction I read as the operand constraints list, especially due to the weird ordering of r10 which is called out as intentional in Go's asm_linux_amd64.s. And then the use of the sideeffect keyword I read as "volatile barrier" if I'm allowed to invent a new term ;)

Okay. The comment is regarding Linux AMD64 syscall vs. "standard" SysV AMD64 ABI. If you make a call to a C function, then rcx is used; if making a syscall, r10 in its place.

Do you know the motivation behind the -4095 constant? If I understand the code correctly this will make return values from -1 to -4094 fail and the rest pass. The man page just states that "The return value is defined by the system call being invoked. In general, a 0 return value indicates success. A -1 return value indicates an error, and an error code is stored in errno."

The code is saying: if the result is less than the unsigned representation of -4095, then everything's fine. Thus, if the result is between -1 and -4095, it's an error. This is described in the ABI document. If an error, the result == -errno, hence the sub instruction.

The man page is referring to the C wrapper, which will be doing essentially the same as this function under the hood.

@quarnster
Copy link
Contributor

I see, I'll amend the pull request with a link to the pdf and will correct the code comments.

@quarnster
Copy link
Contributor

Btw, do you read the -4095 as being inclusive in the range that's an error? The doc states:

A value in the range between -4095 and -1 indicates an error

And currently we treat -1 as an error, but not -4095 due to the "less than" comparison.

quarnster added a commit to quarnster/llgo that referenced this issue Sep 26, 2013
This change includes more links to reference material and now makes
use of the carry flag to detect errors.
@axw axw closed this as completed in 1de7c36 Sep 26, 2013
axw added a commit that referenced this issue Sep 26, 2013
pkg/syscall: Fix darwin_amd64 syscalls. Fixes #51.
@axw
Copy link
Member Author

axw commented Sep 26, 2013

And currently we treat -1 as an error, but not -4095 due to the "less than" comparison.

It's a bit subtle, but that -4095 is inclusive as it's an unsigned comparison. -4095 unsigned is 0xfffff001. The range -4095..-1 is 0xfffff001..0xffffffff.

@quarnster
Copy link
Contributor

Ah, right you are :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants