| Message ID | 20220521000400.454525-10-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | semihosting cleanup | expand |
On Sat, 21 May 2022 at 01:04, Richard Henderson <richard.henderson@linaro.org> wrote: > > The err parameter is non-zero if and only if an error occured. > Use this instead of ret == -1 for determining if we need to > update the saved errno. The gdb protocol isn't 100% clear on this, but what it says is: https://sourceware.org/gdb/onlinedocs/gdb/The-F-Reply-Packet.html#The-F-Reply-Packet # retcode is the return code of the system call as hexadecimal value. # errno is the errno set by the call, in protocol-specific representation. # This parameter can be omitted if the call was successful. (and from the implementation in gdb it is basically just returning the return value from a syscall plus errno). That implies that our current code is right, in that the way to check for "did the call fail" is to look at the retcode, not the errno (in the same way that if you make a native syscall or library call you look first at its return value, not at errno). There's nothing in the protocol text that makes a guarantee that the errno value is non-0 if and only if the call failed. (I think gdb's implementation happens to only set errno to non-0 on failures, and lldb doesn't implement the file-io extension at all as far as I can see, so there might be scope for getting the protocol definition tightened up I guess.) > This fixes the errno setting of SYS_ISTTY, which returns 0 on > error, not -1. The gdb implementation of the isatty call returns 0 or 1 on success, and -1 on failure (though the only failure mode it has is "you messed up the protocol packet format"): https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/remote-fileio.c;h=fe191fb6069a53a3844656a81e77069afa781946;hb=HEAD#l1039 thanks -- PMM
On 5/23/22 05:13, Peter Maydell wrote: > On Sat, 21 May 2022 at 01:04, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> The err parameter is non-zero if and only if an error occured. >> Use this instead of ret == -1 for determining if we need to >> update the saved errno. > > The gdb protocol isn't 100% clear on this, but what it says is: > https://sourceware.org/gdb/onlinedocs/gdb/The-F-Reply-Packet.html#The-F-Reply-Packet > > # retcode is the return code of the system call as hexadecimal value. > # errno is the errno set by the call, in protocol-specific representation. > # This parameter can be omitted if the call was successful. > > (and from the implementation in gdb it is basically just returning > the return value from a syscall plus errno). > > That implies that our current code is right, in that the > way to check for "did the call fail" is to look at the > retcode, not the errno (in the same way that if you make a > native syscall or library call you look first at its return > value, not at errno). There's nothing in the protocol text > that makes a guarantee that the errno value is non-0 if and > only if the call failed. I admit that I didn't check the gdb code. I looked at our side and saw that when the second result is missing that we'd supply 0, and interpreted "can be omitted" as "will be omitted" on success. > The gdb implementation of the isatty call returns 0 or 1 on > success, and -1 on failure (though the only failure mode it has > is "you messed up the protocol packet format"): > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/remote-fileio.c;h=fe191fb6069a53a3844656a81e77069afa781946;hb=HEAD#l1039 Technically, isatty = 0 is failure not success and should also set ENOTTY. r~
On Mon, 23 May 2022 at 16:35, Richard Henderson <richard.henderson@linaro.org> wrote: > On 5/23/22 05:13, Peter Maydell wrote: > > The gdb implementation of the isatty call returns 0 or 1 on > > success, and -1 on failure (though the only failure mode it has > > is "you messed up the protocol packet format"): > > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/remote-fileio.c;h=fe191fb6069a53a3844656a81e77069afa781946;hb=HEAD#l1039 > > Technically, isatty = 0 is failure not success and should also set ENOTTY. There are multiple different APIs here with similar names but not necessarily always the exact same behaviour in all cases: Arm semihosting SYS_ISTTY: https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst#sys-istty-0x09 returns 1 if the handle identifies an interactive device. 0 if the handle identifies a file. A value other than 1 or 0 if an error occurs (and implicitly sets errno, I assume) GDB File-I/O isatty: https://sourceware.org/gdb/onlinedocs/gdb/isatty.html#isatty Returns 1 if fd refers to the GDB console, 0 otherwise Documentation doesn't say how it reports errors. Actual implementation returns -1 and sets errno. (We should probably report some of these spec issues as gdb bugs...) isatty() POSIX function: returns 1 for a terminal 0 with errno ENOTTY for not-a-terminal 0 with some other errno for error cases It looks like our 'host' implementation of the semihosting SYS_ISTTY doesn't correctly do the matching up between the semihosting spec and the isatty() function, so it will return the wrong value for the error case. thanks -- PMM
On 5/23/22 08:35, Richard Henderson wrote: >> That implies that our current code is right, in that the >> way to check for "did the call fail" is to look at the >> retcode, not the errno (in the same way that if you make a >> native syscall or library call you look first at its return >> value, not at errno). There's nothing in the protocol text >> that makes a guarantee that the errno value is non-0 if and >> only if the call failed. > > I admit that I didn't check the gdb code. I looked at our side and saw that when the > second result is missing that we'd supply 0, and interpreted "can be omitted" as "will be > omitted" on success. Checking, can->will is actually correct, and gdb will omit the errno parameter on success. https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=gdb/remote-fileio.c;h=fe191fb6069a53a3844656a81e77069afa781946;hb=HEAD#l328 So I think checking err != 0 is a good change. r~
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c index b00ed2c6d1..88e1c286ba 100644 --- a/semihosting/arm-compat-semi.c +++ b/semihosting/arm-compat-semi.c @@ -290,7 +290,7 @@ static target_ulong common_semi_syscall_len; static void common_semi_cb(CPUState *cs, target_ulong ret, target_ulong err) { - if (ret == (target_ulong)-1) { + if (err) { #ifdef CONFIG_USER_ONLY TaskState *ts = cs->opaque; ts->swi_errno = err;
The err parameter is non-zero if and only if an error occured. Use this instead of ret == -1 for determining if we need to update the saved errno. This fixes the errno setting of SYS_ISTTY, which returns 0 on error, not -1. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- semihosting/arm-compat-semi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)