linux-user riscv32 stat structure doesn't match glibc/musl

[michaeljclark]

QEMU linux-user emulation has a problem with the stat system call on riscv32. It is likely that QEMU has the wrong structure. RISC-V is using Linux asm-generic stat.

riscv-qemu is using the default stat implementation for riscv32 however it appears one of the QEMU structure members likely has a different size in the struct stat definition in riscv-qemu/linux-user/syscall.c vs the struct stat used by riscv32 glibc and musl-libc.

$ cat ~/src/riscv-gcc-bugs/stat.c 
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

int
main (void)
{
  int fd = open ("tmp.file", O_CREAT|O_RDWR, S_IRWXU);
  if (fd == -1)
    {
      perror ("open failed");
      exit (1);
    }
  struct stat buf;
  int result = fstat (fd, &buf);
  if (result == -1)
    {
      perror ("fstat failed");
      exit (1);
    }
  printf ("S_ISREG (buf.st_mode) = %d\n", S_ISREG (buf.st_mode));
  return 0;
}

Both musl and glibc have the same problem (correct result is 1):

musl:

$ riscv32-linux-musl-gcc -static stat.c
$ qemu-riscv32 ./a.out 
S_ISREG (buf.st_mode) = 0

glibc:

$ riscv32-unknown-linux-gnu-gcc -static stat.c
$ qemu-riscv32 ./a.out 
S_ISREG (buf.st_mode) = 0

control (x86_64 Linux):

$ gcc stat.c 
$ ./a.out 
S_ISREG (buf.st_mode) = 1

[michaeljclark]

@jim-wilson

Found it. It is sizeof(st_dev) and sizeof(st_ino) as they come before st_mode. musl and glibc on riscv32 expect them to be 64-bits (8-bytes) but QEMU defines them to be abi_ulong, and unsigned long on 32-bit systems is 32-bits (4 bytes)

This is from musl:

$ cat ./arch/riscv32/bits/stat.h
struct stat {
	dev_t st_dev;
	ino_t st_ino;
	mode_t st_mode;
	nlink_t st_nlink;
	uid_t st_uid;
	gid_t st_gid;
	dev_t st_rdev;
	unsigned long __pad;
	off_t st_size;
	blksize_t st_blksize;
	int __pad2;
	blkcnt_t st_blocks;
	struct timespec st_atim;
	struct timespec st_mtim;
	struct timespec st_ctim;
	unsigned __unused[2];
};

Add the following two lines to the stat.c test:

  printf ("sizeof(buf.st_dev) = %zu\n", sizeof(buf.st_dev));
  printf ("sizeof(buf.st_ino) = %zu\n", sizeof(buf.st_ino));

$ qemu-riscv32 ./a.out
sizeof(buf.st_dev) = 8
sizeof(buf.st_ino) = 8
S_ISREG (buf.st_mode) = 0

This is from QEMU (we are using asm-generic):

/* These are the asm-generic versions of the stat and stat64 structures */

struct target_stat {
    abi_ulong st_dev;
    abi_ulong st_ino;
    unsigned int st_mode;
    unsigned int st_nlink;
    unsigned int st_uid;
    unsigned int st_gid;
    abi_ulong st_rdev;
    abi_ulong __pad1;
    abi_long st_size;
    int st_blksize;
    int __pad2;
    abi_long st_blocks;
    abi_long target_st_atime;
    abi_ulong target_st_atime_nsec;
    abi_long target_st_mtime;
    abi_ulong target_st_mtime_nsec;
    abi_long target_st_ctime;
    abi_ulong target_st_ctime_nsec;
    unsigned int __unused4;
    unsigned int __unused5;
};

Note that abi_ulong on riscv32 will be 32-bits or 4-bytes.

[michaeljclark]

QEMU appears to match asm-generic in Linux which uses unsigned long (32-bits on riscv32) unless __ARCH_WANT_STAT64 is defined.

I suspect QEMU might match Linux on riscv32 and the stat test may work correctly in a Linux full-system.

We should run the test in a riscv32 Linux system. It's probably not too late to change to 64-bits (8-bytes) if Linux is currently 32-bits (4-bytes).

$ more include/uapi/asm-generic/stat.h
/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
#ifndef __ASM_GENERIC_STAT_H
#define __ASM_GENERIC_STAT_H

/*
 * Everybody gets this wrong and has to stick with it for all
 * eternity. Hopefully, this version gets used by new architectures
 * so they don't fall into the same traps.
 *
 * stat64 is copied from powerpc64, with explicit padding added.
 * stat is the same structure layout on 64-bit, without the 'long long'
 * types.
 *
 * By convention, 64 bit architectures use the stat interface, while
 * 32 bit architectures use the stat64 interface. Note that we don't
 * provide an __old_kernel_stat here, which new architecture should
 * not have to start with.
 */

#include <asm/bitsperlong.h>

#define STAT_HAVE_NSEC 1

struct stat {
	unsigned long	st_dev;		/* Device.  */
	unsigned long	st_ino;		/* File serial number.  */
	unsigned int	st_mode;	/* File mode.  */
	unsigned int	st_nlink;	/* Link count.  */
	unsigned int	st_uid;		/* User ID of the file's owner.  */
	unsigned int	st_gid;		/* Group ID of the file's group. */
	unsigned long	st_rdev;	/* Device number, if device.  */
	unsigned long	__pad1;
	long		st_size;	/* Size of file, in bytes.  */
	int		st_blksize;	/* Optimal block size for I/O.  */
	int		__pad2;
	long		st_blocks;	/* Number 512-byte blocks allocated. */
	long		st_atime;	/* Time of last access.  */
	unsigned long	st_atime_nsec;
	long		st_mtime;	/* Time of last modification.  */
	unsigned long	st_mtime_nsec;
	long		st_ctime;	/* Time of last status change.  */
	unsigned long	st_ctime_nsec;
	unsigned int	__unused4;
	unsigned int	__unused5;
};

/* This matches struct stat64 in glibc2.1. Only used for 32 bit. */
#if __BITS_PER_LONG != 64 || defined(__ARCH_WANT_STAT64)
struct stat64 {
	unsigned long long st_dev;	/* Device.  */
	unsigned long long st_ino;	/* File serial number.  */
	unsigned int	st_mode;	/* File mode.  */
	unsigned int	st_nlink;	/* Link count.  */
	unsigned int	st_uid;		/* User ID of the file's owner.  */
	unsigned int	st_gid;		/* Group ID of the file's group. */
	unsigned long long st_rdev;	/* Device number, if device.  */
	unsigned long long __pad1;
	long long	st_size;	/* Size of file, in bytes.  */
	int		st_blksize;	/* Optimal block size for I/O.  */
	int		__pad2;
	long long	st_blocks;	/* Number 512-byte blocks allocated. */
	int		st_atime;	/* Time of last access.  */
	unsigned int	st_atime_nsec;
	int		st_mtime;	/* Time of last modification.  */
	unsigned int	st_mtime_nsec;
	int		st_ctime;	/* Time of last status change.  */
	unsigned int	st_ctime_nsec;
	unsigned int	__unused4;
	unsigned int	__unused5;
};
#endif

#endif /* __ASM_GENERIC_STAT_H */

At least from my read of the linux asm-generic header is that the 64-bit stat isn't defined unless __ARCH_WANT_STAT64 is defined and I can't find that anywhere in arch/riscv

mclark@minty:~/src/sifive/riscv-linux$ grep -r __ARCH_WANT_STAT64 arch/riscv/
mclark@minty:~/src/sifive/riscv-linux$ 

The question is less about whether it's Linux, glibc or QEMU that is wrong, but is more whether we want 64-bit stat. Perhaps the riscv32 linux somehow pulls in a 64-bit definition of st_dev and st_ino but I can't find where...

Next step is to run the test in riscv32 linux full system emulator... we indeed might have to Linux and/or QEMU to match glibc/musl given 64-bit stat structure makes sense.

We'd only change musl or glibc if we decided we wanted 32-bit stat, but that makes less sense for a new port.

[michaeljclark]

I keep remembering about this bug and thought to remind @jim-wilson and @palmer-dabbelt

From my analysis, it seems we need to add a define to riscv-linux/arch/riscv/include/asm/unistd.h

#define __ARCH_WANT_STAT64

• https://github.com/riscv/riscv-linux/blob/master/arch/riscv/include/asm/unistd.h

I believe QEMU matches Linux kernel, but glibc and musl are expecting 64-bit stat.

@jim-wilson We should run your glibc test case in Linux vs qemu-user.

[jim-wilson]

I don't have a 32-bit linux I can use for testing. I can only do 32-bit qemu-user testing. Andes is in the process of submitting 32-bit glibc support upstream. It is possible that they have a fix for this in their glibc patches. We should probably wait until the glibc patch submission gets sorted out, and then check again using upstream glibc.

[michaeljclark]

On Wed, 1 Aug 2018 at 4:40 AM, Jim Wilson ***@***.***> wrote: I don't have a 32-bit linux I can use for testing. I can only do 32-bit qemu-user testing. Andes is in the process of submitting 32-bit glibc support upstream. It is possible that they have a fix for this in their glibc patches. We should probably wait until the glibc patch submission gets sorted out, and then check again using upstream glibc.

Well that was the problem we want to avoid. If Andes upstream glibc that matches the current 32-bit Linux kernel then we may be stuck with 32-bit stat. Most architectures want 64-bit stat. It would be nice to verify this part of the ABI before it is upstream, when it is then too late to change mjc@miqi:~/src/sifive/riscv-linux$ grep -r __ARCH_WANT_STAT64 arch/ arch/m68k/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/x86/include/asm/unistd.h:# define __ARCH_WANT_STAT64 arch/parisc/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/tile/include/uapi/asm/stat.h:#define __ARCH_WANT_STAT64 /* Used for compat_sys_stat64() etc. */ arch/sparc/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/xtensa/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/microblaze/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/m32r/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/alpha/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/mips/include/asm/unistd.h:# define __ARCH_WANT_STAT64 arch/powerpc/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/sh/include/asm/unistd.h:# define __ARCH_WANT_STAT64 arch/frv/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/arm/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/blackfin/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/cris/include/asm/unistd.h:#define __ARCH_WANT_STAT64 arch/mn10300/include/asm/unistd.h:#define __ARCH_WANT_STAT64

[michaeljclark]

Linux is authoritative for the ABI, given that glibc requires Linux to be upstream first. Therein lies the problem. A potential ABI mistake. I can perhaps try your test with musl libc against current linux and see whether musl matches the kernel. I think musl libc matches glibc. I think QEMU matches Linux. I'm not certain. We definitely should sort this out before glibc is upstream.

[jim-wilson]

I posted a message to the libc-alpha thread for the 32-bit RISC-V glibc submission, to let them know about the problem. I'm not sure if there is much else I can easily do.
https://sourceware.org/ml/libc-alpha/2018-07/msg01077.html

[michaeljclark]

riscv32 is an unusual port which confused me. All pre-existing Linux 32-bit ports have 32-bit stat and define __ARCH_WANT_STAT64 for additional 64-bit interfaces, however riscv32 has a 64-bit stat structure using the normal stat interfaces. @palmer-dabbelt mentioned that riscv32 Linux wanted to get rid of the legacy interfaces although I was not certain of the details e.g. how and where it has been implemented, hence my note about getting riscv32 Linux running in "full system mode" so we can run authoritative tests.

This message on the musl list gives some background on 32-bit ports. This is about time_t. Both time_t and the 64-bit default stat on rv32 are unique (vs stat and stat64):

• https://www.openwall.com/lists/musl/2018/09/28/6

Apparently there are issues that may crop up with this approach, and it may affect porting as many Linux 32-bit apps assume 32-bit for various things, and add -D_FILE_OFFSET_BITS=64 to use the *64 abis, which riscv32 appears not to have.

As I mentioned before, linux-kernel is authoritative for the ABI and I just wanted to test on rv32 linux, however that wasn't building at the time so I couldn't test. I managed to get rv32 linux building and booting over the weekend, with a couple of minor patches, and ran your test case and found that riscv32 indeed doesn't have stat/32-bit and stat64/64-bit, rather it has stat/64-bit like rv64.

In any case, now I have tested it, and have made a fix:

• 6b6f93c

Having the exact details help. I was not involved in the riscv32 Linux port, and the ABI is not described anywhere and the sizes are hidden behind several layers of typedefs. I did not know little details like the fact that the stat64 structure was being used with the stat interface. riscv32 is the first port that does this.

Apologies Jim, however at no point did I make any assumptions here, rather I wanted to test in riscv32 QEMU Full System. I can share my image build scripts after i've tidied them up so we can run glibc tests in full system vs qemu-riscv32...

[palmer-dabbelt]

We're using the generic Linux ABI, which we haven't frozen for our 32-bit port. Here's the mailing list post where we talk about the stat shakeup: https://www.spinics.net/lists/kernel/msg2897588.html