* [PATCH] MIPS big endian and *stat syscalls return value
@ 2015-04-07 9:25 Eugene
2015-04-07 11:58 ` Szabolcs Nagy
0 siblings, 1 reply; 6+ messages in thread
From: Eugene @ 2015-04-07 9:25 UTC (permalink / raw)
To: musl
[-- Attachment #1.1: Type: text/plain, Size: 1993 bytes --]
Hello,
Thank for a wonderful system library!
Revision f61be1f875a2758509d6e9e2cf6f1d9603b28b65
<http://git.musl-libc.org/cgit/musl/commit/arch/mips/syscall_arch.h?id=f61be1f875a2758509d6e9e2cf6f1d9603b28b65>
has led to regression with system calls of family *stat on MIPS with big
endian byte order.
Upon successful completion, the system call returns the number from stat
structure instead of 0.
_Information:
_Binutils 2.23.2/2.24
GCC 4.8.3/4.9.2
musl 1.1.5/1.1.8/HEAD
_Code to reproduce problem:_
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
int main(int argc, char *argv[])
{
int fd, res = EXIT_FAILURE, ret;
struct stat st;
fd = open("/dev/urandom", O_RDONLY);
if (fd < 0) {
perror("open");
goto out;
}
ret = fstat(fd, &st);
if (ret < 0) {
perror("fstat");
goto out_close;
} else {
printf("ret = %d\n", ret);
}
res = EXIT_SUCCESS;
out_close:
close(fd);
out:
return res;
}
_Output:_
# test_fstat
ret = 265
# strace -s 1024 test_fstat
execve("/bin/test_fstat", ["test_fstat"], [/* 7 vars */]) = 0
set_thread_area(0x2ae00764) = 0
set_tid_address(0x2adf96b4) = 150
open("/dev/urandom", O_RDONLY|O_LARGEFILE) = 3
fstat64(3, {st_mode=S_IFCHR|0600, st_rdev=makedev(1, 9), ...}) = 0
ioctl(1, TIOCNXCL, {B9600 opost isig icanon echo ...}) = 0
writev(1, [{"ret = 265", 9}, {"\n", 1}], 2ret = 265
) = 10
close(3) = 0
exit_group(0) = ?
+++ exited with 0 +++
The reason of the problem is that the function __stat_fix rewrites
register $v0, which is not stored in the parent function.
Disassembled code of fstat64 is attached.
Patchis attachedalso.
Sorry for my english.
[-- Attachment #1.2: Type: text/html, Size: 4558 bytes --]
[-- Attachment #2: dis.asm --]
[-- Type: application/octet-stream, Size: 1927 bytes --]
[-- Attachment #3: musl.patch --]
[-- Type: text/x-patch, Size: 2949 bytes --]
diff --git a/arch/mips/syscall_arch.h b/arch/mips/syscall_arch.h
index 0f89a1c..ac2487b 100644
--- a/arch/mips/syscall_arch.h
+++ b/arch/mips/syscall_arch.h
@@ -25,11 +25,13 @@ static inline long __syscall0(long n)
{
register long r7 __asm__("$7");
register long r2 __asm__("$2");
+
__asm__ __volatile__ (
"addu $2,$0,%2 ; syscall"
: "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7)
: "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
"$14", "$15", "$24", "$25", "hi", "lo", "memory");
+
return r7 ? -r2 : r2;
}
@@ -38,67 +40,84 @@ static inline long __syscall1(long n, long a)
register long r4 __asm__("$4") = a;
register long r7 __asm__("$7");
register long r2 __asm__("$2");
+
__asm__ __volatile__ (
"addu $2,$0,%2 ; syscall"
: "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
"r"(r4)
: "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
"$14", "$15", "$24", "$25", "hi", "lo", "memory");
+
return r7 ? -r2 : r2;
}
static inline long __syscall2(long n, long a, long b)
{
+ long t;
register long r4 __asm__("$4") = a;
register long r5 __asm__("$5") = b;
register long r7 __asm__("$7");
register long r2 __asm__("$2");
+
__asm__ __volatile__ (
"addu $2,$0,%2 ; syscall"
: "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
"r"(r4), "r"(r5)
: "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
"$14", "$15", "$24", "$25", "hi", "lo", "memory");
+
if (r7) return -r2;
+ t = r2;
if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
- return r2;
+
+ return t;
}
static inline long __syscall3(long n, long a, long b, long c)
{
+ long t;
register long r4 __asm__("$4") = a;
register long r5 __asm__("$5") = b;
register long r6 __asm__("$6") = c;
register long r7 __asm__("$7");
register long r2 __asm__("$2");
+
__asm__ __volatile__ (
"addu $2,$0,%2 ; syscall"
: "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
"r"(r4), "r"(r5), "r"(r6)
: "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
"$14", "$15", "$24", "$25", "hi", "lo", "memory");
+
if (r7) return -r2;
+ t = r2;
if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
- return r2;
+
+ return t;
}
static inline long __syscall4(long n, long a, long b, long c, long d)
{
+ long t;
register long r4 __asm__("$4") = a;
register long r5 __asm__("$5") = b;
register long r6 __asm__("$6") = c;
register long r7 __asm__("$7") = d;
register long r2 __asm__("$2");
+
__asm__ __volatile__ (
"addu $2,$0,%2 ; syscall"
: "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
"r"(r4), "r"(r5), "r"(r6)
: "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
"$14", "$15", "$24", "$25", "hi", "lo", "memory");
+
if (r7) return -r2;
+ t = r2;
if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
if (n == SYS_fstatat) __stat_fix(c);
- return r2;
+
+ return t;
}
#else
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS big endian and *stat syscalls return value
2015-04-07 9:25 [PATCH] MIPS big endian and *stat syscalls return value Eugene
@ 2015-04-07 11:58 ` Szabolcs Nagy
2015-04-07 14:34 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: Szabolcs Nagy @ 2015-04-07 11:58 UTC (permalink / raw)
To: musl
* Eugene <e.yudin@ndmsystems.com> [2015-04-07 12:25:45 +0300]:
>
> The reason of the problem is that the function __stat_fix rewrites register
> $v0, which is not stored in the parent function.
>
ouch
> static inline long __syscall2(long n, long a, long b)
> {
> + long t;
> register long r4 __asm__("$4") = a;
> register long r5 __asm__("$5") = b;
> register long r7 __asm__("$7");
> register long r2 __asm__("$2");
> +
> __asm__ __volatile__ (
> "addu $2,$0,%2 ; syscall"
> : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
> "r"(r4), "r"(r5)
> : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> +
> if (r7) return -r2;
> + t = r2;
> if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
> - return r2;
> +
> + return t;
> }
>
looks ok to me
(other than the newline changes)
i wonder if __stat_fix could be inlined in a way that the
compiler knows it shouldnt clobber r2.
same for __syscall3 and __syscall4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS big endian and *stat syscalls return value
2015-04-07 11:58 ` Szabolcs Nagy
@ 2015-04-07 14:34 ` Rich Felker
2015-04-07 18:18 ` Eugene Yudin
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2015-04-07 14:34 UTC (permalink / raw)
To: musl
On Tue, Apr 07, 2015 at 01:58:21PM +0200, Szabolcs Nagy wrote:
> * Eugene <e.yudin@ndmsystems.com> [2015-04-07 12:25:45 +0300]:
> >
> > The reason of the problem is that the function __stat_fix rewrites register
> > $v0, which is not stored in the parent function.
> >
>
> ouch
Yes ouch, but I've never seen this trigger in practice. With the 'new'
interpretation of __asm__("regname") that clang uses, where it only
affects constraints for __asm__ blocks, such an issue should not be
possible. With the old interpretation old (and maybe current?) gcc
uses, where the variable 'permanently lives' in the register, it
certainly could break, but I've never observed such breakage. I'm
guessing you need -O0 to trigger it, no?
> > static inline long __syscall2(long n, long a, long b)
> > {
> > + long t;
> > register long r4 __asm__("$4") = a;
> > register long r5 __asm__("$5") = b;
> > register long r7 __asm__("$7");
> > register long r2 __asm__("$2");
> > +
> > __asm__ __volatile__ (
> > "addu $2,$0,%2 ; syscall"
> > : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
> > "r"(r4), "r"(r5)
> > : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> > "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> > +
> > if (r7) return -r2;
> > + t = r2;
> > if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
> > - return r2;
> > +
> > + return t;
> > }
> >
>
> looks ok to me
> (other than the newline changes)
Yeah. I would also slightly prefer a name like 'ret' to 't' but these
are purely cosmetic issues. The functional content of the patch looks
fully valid.
> i wonder if __stat_fix could be inlined in a way that the
> compiler knows it shouldnt clobber r2.
In practice it always is, at least for me. But relying on that for
semantic purposes is not valid.
> same for __syscall3 and __syscall4
Yes.
Rich
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS big endian and *stat syscalls return value
2015-04-07 14:34 ` Rich Felker
@ 2015-04-07 18:18 ` Eugene Yudin
2015-04-07 18:24 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: Eugene Yudin @ 2015-04-07 18:18 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 620 bytes --]
Thanks for a quick response!
On Tue, Apr 7, 2015 at 5:34 PM, Rich Felker <dalias@libc.org> wrote:
> I'm
> guessing you need -O0 to trigger it, no?
I'm experience problem with optimization for size (-Os).
It's look like behaviour was changed in later GCC (at least from 4.8.3).
I will test another optimization levels.
> > i wonder if __stat_fix could be inlined in a way that the
> > compiler knows it shouldnt clobber r2.
>
> In practice it always is, at least for me. But relying on that for
> semantic purposes is not valid.
>
I read that in linux kernel are used attribute "always_inline" for critical
parts.
[-- Attachment #2: Type: text/html, Size: 1297 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS big endian and *stat syscalls return value
2015-04-07 18:18 ` Eugene Yudin
@ 2015-04-07 18:24 ` Rich Felker
2015-04-08 7:37 ` Eugene
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2015-04-07 18:24 UTC (permalink / raw)
To: musl
On Tue, Apr 07, 2015 at 09:18:33PM +0300, Eugene Yudin wrote:
> Thanks for a quick response!
>
> On Tue, Apr 7, 2015 at 5:34 PM, Rich Felker <dalias@libc.org> wrote:
>
> > I'm
> > guessing you need -O0 to trigger it, no?
>
>
> I'm experience problem with optimization for size (-Os).
> It's look like behaviour was changed in later GCC (at least from 4.8.3).
> I will test another optimization levels.
OK. In any case I committed the fix. Thanks for catching this!
> > > i wonder if __stat_fix could be inlined in a way that the
> > > compiler knows it shouldnt clobber r2.
> >
> > In practice it always is, at least for me. But relying on that for
> > semantic purposes is not valid.
>
> I read that in linux kernel are used attribute "always_inline" for critical
> parts.
Yes, and that's an ugly/broken hack. If the code is semantically wrong
(i.e. if the old code doesn't formally preclude clobbering) then it
needs to be fixed, not painted over with forced inlining.
Rich
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MIPS big endian and *stat syscalls return value
2015-04-07 18:24 ` Rich Felker
@ 2015-04-08 7:37 ` Eugene
0 siblings, 0 replies; 6+ messages in thread
From: Eugene @ 2015-04-08 7:37 UTC (permalink / raw)
To: musl
On 07.04.2015 21:24, Rich Felker wrote:
> OK. In any case I committed the fix. Thanks for catching this!
Thanks a lot!
I have received the following results for GCC 4.9.2.
Level Static inline works?
O0 no
O1 yes
O2 yes
O3 yes
Os no
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-04-08 7:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-07 9:25 [PATCH] MIPS big endian and *stat syscalls return value Eugene
2015-04-07 11:58 ` Szabolcs Nagy
2015-04-07 14:34 ` Rich Felker
2015-04-07 18:18 ` Eugene Yudin
2015-04-07 18:24 ` Rich Felker
2015-04-08 7:37 ` Eugene
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).