Fix __fxstat which is called from fstat. Remove obsolete fstat64.

Fix __fxstat which is called from fstat. It was used linux syscall.

There is two kind of file functions and types in glibc - func and func64. There is several modes that determine how these functions come into play.

1. 32-bit system, no 64-bit features.
There is only func that works with 32-bit offsets.

2. 32-bit system with _LARGEFILE64_SOURCE.
There is both func and func64. func works with 32-bit offsets, func64 works with 64-bit offsets.

3. 32-bit system with _FILE_OFFSET_BITS=64.
There are func and func64 that are exported as/mapped to "func64".

4. 64-bit system.
There is func that works with 64-bit offsets.

5. 64-bit system with _FILE_OFFSET_BITS=64
There is func and func64 that are exported as/mapped to "func64".

In this patch I implement fstat and fstat64 that both work with 64-bit offsets (i.e. cases 3 and 5). Although we don't need fstat externally, glibc uses it internally.

Change explanations.
bits/types.h: due to long int being 32-bit with __WORDSIZE=64 I have to fix basic type defines.

typesizes.h: if __something_t and __something64_t are different, then weak mapping funcs64 to funcs produce compile errors. This is needed for 64-bit glibc only.

fxstat[64].c: I moved code from fxstat64.c adapting it to new stat structure.

xstat.c: I removed code duplication (converting stat structure) here.

nacl_stat.h: xstat and xstat64 call convertation function by two different names.

xstatconv.c: Glibc have stat convertation functions here. We have our own function and these functions produce compiler errors now so I removed them.

kernel_stat.h: I removed padding and obsolete fields in stat structures because we don't need them anymore and so need to update defines in this file.

bits/stat.h files: here goes our new stat[64] structures. One file is used with 32-bit glibc, the other is used with 64-bit glibc. My changes are identical in these files.

features.h: I force _FILE_OFFSET_BITS to be equal to 64 in user code.

sys/stat.h file: I map __fxstat64 to __fxstat if not in the libc.so. Otherwise it is done in fxstat.c.

BUG=none
TEST=http://codereview.chromium.org/6018003/

Committed: http://git.chromium.org/gitweb/?p=nacl-glibc.git;a=commit;h=08f0205

[eaeltsin, 2010-12-20 15:46:16]

http://codereview.chromium.org/6026005/diff/1/sysdeps/nacl/nacl_stat.h
File sysdeps/nacl/nacl_stat.h (right):

http://codereview.chromium.org/6026005/diff/1/sysdeps/nacl/nacl_stat.h#newcod...
sysdeps/nacl/nacl_stat.h:102: void __nacl_abi_stat_to_stat (struct nacl_abi_stat
*nacl_st,
Please remove declaration from here, make this static in the source file.

[pasko-google - do not use, 2010-12-20 15:46:35]

This needs a test in tests/glibc_syscall_wrappers, please, check that it works
for both glibc32 and glibc64.
Please, also check that stat and stat64 work in glibc32 and glibc64.
Also, think about not duplicating code across fxstat.c and fxstat64.c

[halyavin, 2010-12-21 09:32:40]

On 2010/12/20 15:46:16, Evgeny Eltsin wrote:
> http://codereview.chromium.org/6026005/diff/1/sysdeps/nacl/nacl_stat.h
> File sysdeps/nacl/nacl_stat.h (right):
> 
>
http://codereview.chromium.org/6026005/diff/1/sysdeps/nacl/nacl_stat.h#newcod...
> sysdeps/nacl/nacl_stat.h:102: void __nacl_abi_stat_to_stat (struct
nacl_abi_stat
> *nacl_st,
> Please remove declaration from here, make this static in the source file.
Done

[halyavin, 2010-12-21 11:24:07]

On 2010/12/20 15:46:35, pasko wrote:
> This needs a test in tests/glibc_syscall_wrappers, please, check that it works
> for both glibc32 and glibc64.
> Please, also check that stat and stat64 work in glibc32 and glibc64.
> Also, think about not duplicating code across fxstat.c and fxstat64.c

Test is here: http://codereview.chromium.org/6018003/

Andrey Khalyavin

[halyavin, 2010-12-23 16:04:05]

Now structures are the same in 32-bit, 64-bit and between the modes.

Andrey Khalyavin

[pasko-google - do not use, 2010-12-24 10:23:06]

General suggestions:

please, provide explanations in the code review description of what you do and
*why* you do it. For example, it would be good to provide a background of what
struct stat and struct stat64 are, how portable programs should be using them.
Then describe what is going to be exported from glibc after your change.

Specific comments:

the change in defining stat64 looks large and pretty much untested. Can we
reduce it by 'asking glibc for a wide offset'?

http://codereview.chromium.org/6026005/diff/18001/sysdeps/nacl/nacl_stat.h
File sysdeps/nacl/nacl_stat.h (right):

http://codereview.chromium.org/6026005/diff/18001/sysdeps/nacl/nacl_stat.h#ne...
sysdeps/nacl/nacl_stat.h:102: void __nacl_abi_stat_to_stat (struct nacl_abi_stat
*nacl_st,
so now you have stat == stat64?
so this copying of functionality is even more super ugly?

Why do we need stat64? I assume we can eliminate it...

[halyavin, 2010-12-24 11:37:47]

On 2010/12/24 10:23:06, pasko wrote:
> Specific comments:
> 
> the change in defining stat64 looks large and pretty much untested. Can we
> reduce it by 'asking glibc for a wide offset'?
Nope. This option works for 32-bit glibc only.
> 
> http://codereview.chromium.org/6026005/diff/18001/sysdeps/nacl/nacl_stat.h
> File sysdeps/nacl/nacl_stat.h (right):
> 
>
http://codereview.chromium.org/6026005/diff/18001/sysdeps/nacl/nacl_stat.h#ne...
> sysdeps/nacl/nacl_stat.h:102: void __nacl_abi_stat_to_stat (struct
nacl_abi_stat
> *nacl_st,
> so now you have stat == stat64?
> so this copying of functionality is even more super ugly?
Yes, we have stat == stat64. There is no copying of functionality though - the
functions are the same.
> 
> Why do we need stat64? I assume we can eliminate it...

Yes if we are 64-bit system and no if we are 32-bit system. But with being stuck
in between we need to make our own decision.

Andrey Khalyavin

[halyavin, 2010-12-24 12:29:17]

Updated description with explanation. Take another look.

Andrey Khalyavin.

[pasko-google - do not use, 2010-12-24 12:32:46]

Andrey,

thanks for a better description.

Please, run io/test-stat.c and io/test-stat2.c on both nacl32 and nacl64. List
them in in the TEST= section.

[pasko-google - do not use, 2010-12-24 13:04:49]

stat.h: As you know, I don't like massive copying. Can the definitions be
included from a new header file into both places?

[pasko-google - do not use, 2010-12-24 15:54:46]

Quote from the Description:
> 2. 32-bit system with _LARGEFILE64_SOURCE.
> There is both func and func64. func works with 32-bit offsets, func64 works
with 64-bit offsets.
> 
> 3. 32-bit system with _FILE_OFFSET_BITS=64.
> There is func that exported as/mapped to "func64". func works with 64-bit
offsets.

it is not precise to speak in terms of 32bit system 'with' _LARGEFILE_SOURCE,
this can be understood incorrectly. On the same 32bit system it is possible to
compile sources in several modes regarding large file support:

gcc -D_LARGEFILE_SOURCE
adds fseeko(), ftello()

gcc -D_FILE_OFFSET_BITS=64
makes the default 'struct stat' to have a 64bit offset

gcc -D_FILE_OFFSET_BITS=32
makes the default 'struct stat' to have a 32bit offset

A more correct approach would be to refer to largefile modes of compilation, not
'32-bit systems' or '64-bit systems'. Systems may support different modes of
compilation regarding LFS.

One may issue a command to know what the best largefile support the system has:
shell> getconf LFS_CFLAGS

on my 32bit system (Ubuntu Karmic) it returns:
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64

All the systems that NativeClient is supposed to run on will have this level of
LFS support for user modules. One can hardly find a program that has been
supported during last 10 years that would get confused with this mode. We should
have: "-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64" by default and issue a
preprocessor error when FILE_OFFSET_BITS is not equal to 64.

This means no func64.

[halyavin, 2010-12-27 09:00:52]

On 2010/12/24 13:04:49, pasko wrote:
> stat.h: As you know, I don't like massive copying. Can the definitions be
> included from a new header file into both places?
These files are already kind of copy of each other. So I don't think this is a
good idea.

Andrey Khalyavin

[halyavin, 2010-12-28 14:17:41]

Take another look.

Andrey Khalyavin

[eaeltsin, 2011-01-13 10:43:45]

http://codereview.chromium.org/6026005/diff/41001/bits/types.h
File bits/types.h (right):

http://codereview.chromium.org/6026005/diff/41001/bits/types.h#newcode105
bits/types.h:105: 
New empty line here? Please remove if so.

http://codereview.chromium.org/6026005/diff/41001/include/features.h
File include/features.h (right):

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode115
include/features.h:115: 
Please add a comment on what you are doing here.

What about USE_FILE_OFFSET64?
Also see comment in io/sys/stat.h

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode121
include/features.h:121: #  if _FILE_OFFSET_BITS != 64
elif instead endif+if?

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode122
include/features.h:122: #   error "native client requires _FILE_OFFSET_BITS
being equal to 64"
_FILE_OFFSET_BITS == 64

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode171
include/features.h:171: # undef  _LARGEFILE64_SOURCE
Please avoid spacing only changes.

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode265
include/features.h:265: unfortunately we can't make glibc itself live without
them */
Please align this comment, add punctuation and capitalize it properly.

http://codereview.chromium.org/6026005/diff/41001/io/sys/stat.h
File io/sys/stat.h (right):

http://codereview.chromium.org/6026005/diff/41001/io/sys/stat.h#newcode396
io/sys/stat.h:396: #if !defined __USE_FILE_OFFSET64 || defined __native_client__
Why do you need to check for native client here?
Probably everything should be set by features.h?

http://codereview.chromium.org/6026005/diff/41001/io/sys/stat.h#newcode432
io/sys/stat.h:432: __asm__ ("__fxstat")
Please add a comment on this.

http://codereview.chromium.org/6026005/diff/41001/sysdeps/nacl/xstatconv.c
File sysdeps/nacl/xstatconv.c (right):

http://codereview.chromium.org/6026005/diff/41001/sysdeps/nacl/xstatconv.c#ne...
sysdeps/nacl/xstatconv.c:1: /* we implement stat convertations in fxstat.c */
convertations -> conversions

Please comment why we need this file at all.

http://codereview.chromium.org/6026005/diff/41001/sysdeps/unix/sysv/linux/ker...
File sysdeps/unix/sysv/linux/kernel_stat.h (right):

http://codereview.chromium.org/6026005/diff/41001/sysdeps/unix/sysv/linux/ker...
sysdeps/unix/sysv/linux/kernel_stat.h:27: #define _HAVE_STAT___PAD1
Please keep the original order of defines while possible.

[halyavin, 2011-01-13 11:15:21]

http://codereview.chromium.org/6026005/diff/41001/bits/types.h
File bits/types.h (right):

http://codereview.chromium.org/6026005/diff/41001/bits/types.h#newcode105
bits/types.h:105: 
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> New empty line here? Please remove if so.

Done.

http://codereview.chromium.org/6026005/diff/41001/include/features.h
File include/features.h (right):

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode115
include/features.h:115: 
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> Please add a comment on what you are doing here.
> 
> What about USE_FILE_OFFSET64?
> Also see comment in io/sys/stat.h

Done.

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode121
include/features.h:121: #  if _FILE_OFFSET_BITS != 64
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> elif instead endif+if?

Done.

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode122
include/features.h:122: #   error "native client requires _FILE_OFFSET_BITS
being equal to 64"
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> _FILE_OFFSET_BITS == 64

Done.

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode171
include/features.h:171: # undef  _LARGEFILE64_SOURCE
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> Please avoid spacing only changes.

Done.

http://codereview.chromium.org/6026005/diff/41001/include/features.h#newcode265
include/features.h:265: unfortunately we can't make glibc itself live without
them */
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> Please align this comment, add punctuation and capitalize it properly.

Done.

http://codereview.chromium.org/6026005/diff/41001/io/sys/stat.h
File io/sys/stat.h (right):

http://codereview.chromium.org/6026005/diff/41001/io/sys/stat.h#newcode396
io/sys/stat.h:396: #if !defined __USE_FILE_OFFSET64 || defined __native_client__
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> Why do you need to check for native client here?
> Probably everything should be set by features.h?
Yes, we do. Because we implement __USE_FILE_OFFSET64 mode but do not need to map
__fxstat to __fxstat64 since we don't have __fxstat64 at all.

http://codereview.chromium.org/6026005/diff/41001/io/sys/stat.h#newcode432
io/sys/stat.h:432: __asm__ ("__fxstat")
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> Please add a comment on this.

We are going to have a dozen of such lines (one for every file function) and it
doesn't make sense to have an identical comment for every one of them.

This line maps __fxstat64 to __fxstat in cases:
1. Other glibc libraries.
  They actively use such functions and so we have to compile them in
__USE_LARGEFILE64 mode.
2. glibc itself in static mode.
  Glibc itself uses __fxstat64 and must be compiled in __USE_LARGEFILE64 mode.

glibc itself in shared mode maps __fxstat64 to __GI___fxstat64. So we can't map
it that way. The mapping is done in fxstat.c instead.

http://codereview.chromium.org/6026005/diff/41001/sysdeps/nacl/xstatconv.c
File sysdeps/nacl/xstatconv.c (right):

http://codereview.chromium.org/6026005/diff/41001/sysdeps/nacl/xstatconv.c#ne...
sysdeps/nacl/xstatconv.c:1: /* we implement stat convertations in fxstat.c */
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> convertations -> conversions
> 
> Please comment why we need this file at all.

Done.

http://codereview.chromium.org/6026005/diff/41001/sysdeps/unix/sysv/linux/ker...
File sysdeps/unix/sysv/linux/kernel_stat.h (right):

http://codereview.chromium.org/6026005/diff/41001/sysdeps/unix/sysv/linux/ker...
sysdeps/unix/sysv/linux/kernel_stat.h:27: #define _HAVE_STAT___PAD1
On 2011/01/13 10:43:45, Evgeny Eltsin wrote:
> Please keep the original order of defines while possible.

Done.

[khim, 2011-01-13 13:12:38]

One question, the rest LGTM...

http://codereview.chromium.org/6026005/diff/48001/sysdeps/nacl/fxstat.c
File sysdeps/nacl/fxstat.c (right):

http://codereview.chromium.org/6026005/diff/48001/sysdeps/nacl/fxstat.c#newco...
sysdeps/nacl/fxstat.c:50: extern __typeof (__nacl_abi_stat_to_stat64)
__nacl_abi_stat_to_stat64 __attribute__ ((alias ("__nacl_abi_stat_to_stat")));
Why do we need both __nacl_abi_stat_to_stat64 and __nacl_abi_stat_to_stat ?

I can see where we call __nacl_abi_stat_to_stat but what about
__nacl_abi_stat_to_stat64 ???

[Mark Seaborn, 2011-01-14 16:10:44]

This change is quite big in scope because you're changing bits/types.h and
bits/typesizes.h.  I did not look at it closely before because the subject line
does not make the scope clear.  I am still not sure what is enabled by this
change.  You really need to fill out TEST= and BUG= first.

You have said "Although we don't need fstat externally, glibc uses it
internally".  OK, where does it use it internally?  How are you testing those
uses?

Cheers,
Mark

[halyavin, 2011-01-17 08:11:44]

> You have said "Although we don't need fstat externally, glibc uses it
> internally".  OK, where does it use it internally?  How are you testing those
> uses?

In this document
https://docs.google.com/a/google.com/document/d/1CSPmXWSKrc51O0S3TwmfLzlysf9S...
you can find the list of internal uses of stat and stat64. List for fstat and
fstat64 is similar. 

I test this uses by compiling glibc and checking symbol tables. fstat and
fstat64 are completely equal now and so this internal use doesn't require
separate testing. 

Andrey Khalyavin
> 
> Cheers,
> Mark

[eaeltsin, 2011-01-17 16:25:31]

lgtm

[Mark Seaborn, 2011-01-18 16:19:46]

On 17 January 2011 08:11, <halyavin@google.com> wrote:

> You have said "Although we don't need fstat externally, glibc uses it
>> internally".  OK, where does it use it internally?  How are you testing
>> those
>> uses?
>>
>
> In this document
>
>
https://docs.google.com/a/google.com/document/d/1CSPmXWSKrc51O0S3TwmfLzlysf9S...
> you can find the list of internal uses of stat and stat64. List for fstat
> and
> fstat64 is similar.


Can you move this document to a publicly-accessible location, please?

Mark

[pasko-google - do not use, 2011-01-29 11:57:56]

http://codereview.chromium.org/6026005/diff/48001/sysdeps/unix/sysv/linux/bit...
File sysdeps/unix/sysv/linux/bits/stat.h (right):

http://codereview.chromium.org/6026005/diff/48001/sysdeps/unix/sysv/linux/bit...
sysdeps/unix/sysv/linux/bits/stat.h:36: struct stat64
this part is overly similar in
sysdeps/unix/sysv/linux/bits/stat.h and
sysdeps/unix/sysv/linux/x86_64/bits/stat.h

why did you not move it to a common header?

[halyavin, 2011-01-31 08:33:36]

On 2011/01/29 11:57:56, pasko wrote:
>
http://codereview.chromium.org/6026005/diff/48001/sysdeps/unix/sysv/linux/bit...
> File sysdeps/unix/sysv/linux/bits/stat.h (right):
> 
>
http://codereview.chromium.org/6026005/diff/48001/sysdeps/unix/sysv/linux/bit...
> sysdeps/unix/sysv/linux/bits/stat.h:36: struct stat64
> this part is overly similar in
> sysdeps/unix/sysv/linux/bits/stat.h and
> sysdeps/unix/sysv/linux/x86_64/bits/stat.h
> 
> why did you not move it to a common header?
Glibc does not use a common header here. These two files will be used anyway. I
don't know the way to override the *.h files. Even if I did that, we would have
even more duplication because we need to copy the rest of stat.h file.

Andrey Khalyavin