Re: [PATCH v5 00/14] port C-SKY to glibc

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 23 Nov 2018, Mao Han wrote:

> Current statx_cp.c will check __NR_fstat64, __NR_fstatat64, __WORDSIZE 
> == 32 to see if it is a 32 bit platforms without fstat64; __NR_fstat and 
> __NR_newfstatat check is for mips64-*-n32 and x86_64-*-x32 they always
> use 64 bit system call but got __WORDSIZE == 32.

That might give the right conditions for current glibc platforms, but I 
don't think it's logically correct - putting conditions on __NR_fstat and 
__NR_newfstatat, when none of the files using statx also use those 
syscalls, is hardly a clean approach.

Since the way in which those platforms avoid using the files in question 
is through having their own sysdeps override files that either contain 
just comments or reimplement the functions in question, my suggestion 
would be to have statx_cp.c files in sysdeps/unix/sysv/linux/wordsize-64 
and sysdeps/unix/sysv/linux/mips/mips64, containing just a comment.  Then 
the conditional in the main statx_cp.c file would be

#if !defined(__NR_fstat64) || !defined(__NR_fstatat64)

(note this is || not &&, because if either macro is undefined, the 
relevant source file will use statx).

Note that the ChangeLog entry needs updating to reflect the actual set of 
files modified (it still refers to wordsize-32 for two files being 
modified outside of the wordsize-32 directory).

> +#include "statx_cp.h"

I think these includes should be of <statx_cp.h> not "statx_cp.h" - in 
general, #include <> should be preferred to avoid any problems overriding 
a file in another sysdeps directory if necessary.

You're missing an include of statx_cp.h from statx_cp.c.  It's important 
to include the header from the file defining the corresponding functions, 
so that the compiler checks the prototypes in the two places are 
consistent.

> +__cp_stat_statx (struct stat *to, struct statx *from)
> +{
> +  memset(to, 0, sizeof(struct stat));

Missing space before '(', twice on this line.

> +  memset(to, 0, sizeof(struct stat64));

Likewise.

I also note that when the generic/wordsize-32 files use the existing 
syscalls, they use stat_overflow in the case of success to ensure that if 
any of the fields overflow the 32-bit versions in struct stat, an error is 
properly returned with errno set to EOVERFLOW.  This patch seems to be 
missing anything like that in the case where statx is used - indeed, 
__cp_stat_statx returns void, so preventing it from indicating an error to 
the caller.

It's probably a bad idea to duplicate the logic for such overflow checks - 
rather, the existing stat_overflow code should be used.  Maybe eliminate 
__cp_stat_statx, make the callers cast to (struct stat64 *) and call 
__cp_stat64_statx and then use stat_overflow in the callers?  (As all the 
cases needing this check are in generic/wordsize-32, we know that the stat 
and stat64 layouts are the same, just differing in whether or not high 
parts are counted as padding.  Because statx_cp.c is in a separate 
translation unit and glibc uses C with ABI boundaries, doing the cast in a 
caller like that is safe, whereas defining __cp_stat_statx to do such a 
cast, call and then check with stat_overflow, in the same translation unit 
as the __cp_stat64_statx definition, could run into aliasing issues.)

-- 
Joseph S. Myers
joseph@codesourcery.com