546535 - failures on bigfile2 and bigfile3 tests on Android

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Ben Combee (:bcombee)]

bigfile2 and bigfile3 fail on Android because of a miscompilation of NSPR.  The Android build uses nsprpub/pr/include/md/_linux.h since it's a Linux platform.  However, this header uses checks on versions of GLIBC to indicate if library features are present, such as the existence of 64-bit seek calls.  With Android, these GLIBC checks fail because they provide their own C library called bionic, so we make the wrong assumptions.

We should either add Bionic checks to _linux.h or create a separate _android.h that has the correct values to fix this.

Comment 1

[Harry Li]

as seen in http://mxr.mozilla.org/nspr/source/nsprpub/pr/tests/README.TXT and 
runtests.sh, it seems bigfile2 and bigfile3 are not tested on any platform. 
Should we fix this in Android?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Even though we want these to be fixed, should this bug block fennec?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

not blocking

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this is mostly a guess, but I assume that its safer to not use these features until we're sure they exist on bionic.

Comment 5

[Wan-Teh Chang]

Comment on attachment 477396 [details] [diff] [review]
patch

_PR_POLL_AVAILABLE: I looked into this.  As long as
the <poll.h> system header exists, you should leave
_PR_POLL_AVAILABLE defined for Android.

_PR_HAVE_OFF64_T: if you undefine this and define
_PR_NO_LARGE_FILES instead, you won't be able to
handle files larger than 2GB on Android.  iPhone 4
has 16GB and 32GB models, so mobile phones are
likely to deal with large files soon, not to mention
Android can also be used for other embedded devices.

_PR_INET6: does Android not support IPv6?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #5)
> Comment on attachment 477396 [details] [diff] [review]
> patch
> 
> _PR_POLL_AVAILABLE: I looked into this.  As long as
> the <poll.h> system header exists, you should leave
> _PR_POLL_AVAILABLE defined for Android.
ok
> 
> _PR_HAVE_OFF64_T: if you undefine this and define
> _PR_NO_LARGE_FILES instead, you won't be able to
> handle files larger than 2GB on Android.  iPhone 4
> has 16GB and 32GB models, so mobile phones are
> likely to deal with large files soon, not to mention
> Android can also be used for other embedded devices.

I grep'd the android ndk for open64 and didn't find anything, and that's what this is macro is used for, right? Or are are you saying we shouldn't have _PR_HAVE_OFF64_T or _PR_NO_LARGE_FILES defined? In which case, lseek64, fstat64 and stat64 are all defined, but mmap64 isn't.

I'm not sure any of this really matters though since open64 isn't defined and thus we'd fail here (unless its on the devices and not the ndk):
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/unix.c#2774

> 
> _PR_INET6: does Android not support IPv6?
no, it does. That was mistake.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #5)
> Comment on attachment 477396 [details] [diff] [review]
> patch
> 
> _PR_POLL_AVAILABLE: I looked into this.  As long as
> the <poll.h> system header exists, you should leave
> _PR_POLL_AVAILABLE defined for Android.

just wanted to point out bug 558423, not sure if that matters

Comment 8

[Wan-Teh Chang]

We need to try our best to support large files.
Otherwise it'll come back to haunt us.  It may
take more work than simply defining _PR_HAVE_OFF64_T,
but the presence of lseek64, fstat64, and stat64
makes me hopeful that Android supports large files.
mmap64 may not be as important.

Re: failure of the poll_er test: that should not
matter to the _PR_POLL_AVAILABLE issue.  If Android
has the <poll.h> system header and the poll() system
call, _PR_POLL_AVAILABLE should be defined.

Comment 9

[Wan-Teh Chang]

Attachment: patch v2

Please review and test this patch.

(We fixed the IPv6 support issue in bug 626866.)

Please check if it's OK to use __mmap2.  I found it
in Android's implementation of mmap:
http://android.git.kernel.org/?p=platform/bionic.git;a=blob;f=libc/unistd/mmap.c

If we should not use __mmap2, I can implement mmap64
in a different way, with reduced functionality.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Once I finally found an 8GB sd card, I tested with this. Both bigfile2 and bigfile3 fail after printing "PR_Seek64 failed" to the console. Is there any other information that would be useful? or other tests to run?

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

I should note that I tested on the nexus one running froyo

Comment 12

[Wan-Teh Chang]

Please set a breakpoint in _MD_lseek64 (in unix.c) and
see what the value of errno is (stored in the local
variable syserr):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/md/unix/unix.c&rev=3.58&mark=2335,2356,2360#2335

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

errno is 27, "File too large"

Comment 14

[Wan-Teh Chang]

Attachment: patch v3 (checked in)

Pass O_LARGEFILE to open() explicitly.

Please try this patch instead.  If the PR_Seek64 calls
in bigfile2.c and bigfile3.c still fail, try changing
the offset argument to a < 32-bit value:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/tests/bigfile2.c&rev=3.8&mark=69#57

For example, change 32 to 16 and see if that allows
the test to pass.

By the way, my comments in bug 5451 showed that the
bigfile2.c and bigfile3.c tests were written for
Windows specifically, so when running on other platforms,
they don't verify the results.  But at least we'll know
if PR_Seek64 and PR_Write fail on a large file.

Comment 15

[Wan-Teh Chang]

Comment on attachment 510619 [details] [diff] [review]
patch v3 (checked in)

According to this search in the Android git repository:
http://android.git.kernel.org/?p=platform%2Fbionic.git&a=search&h=HEAD&st=grep&s=O_LARGEFILE

we should not need to pass O_LARGEFILE to open() explicitly.
So I don't know why patch v2 doesn't work.  Perhaps the
older Froyo source code is different.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #14)
> For example, change 32 to 16 and see if that allows
> the test to pass.

32 fails, but 16 and 24 pass

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #16)
> (In reply to comment #14)
> > For example, change 32 to 16 and see if that allows
> > the test to pass.
> 
> 32 fails, but 16 and 24 pass

31 passes as well, just took most of the night

Comment 18

[Wan-Teh Chang]

Comment on attachment 510619 [details] [diff] [review]
patch v3 (checked in)

Brad: thank you for testing.  You're using patch v3 now, right?

This means the large file API in Android doesn't really support
large files, unless I made a mistake.  But the large file API
still works on small files.  For example, PR_Seek64 works if
the file offset is < 32 bits.

So we should still check in this patch.  Could you please
review it?  I need your opinion on whether it is OK to use
the __mmap2 function this way.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 510619 [details] [diff] [review]
patch v3 (checked in)

the use of mmap2 looks to be fine to me.

One thing I wonder about though is what happens when offset isn't a multiple of ANDROID_PAGE_SIZE. Should we just increment the return value by the remainder? If so, would munmap handle that?

Comment 20

[Wan-Teh Chang]

Comment on attachment 510619 [details] [diff] [review]
patch v3 (checked in)

My mmap64 function is based on the mmap function in Android:
http://android.git.kernel.org/?p=platform/bionic.git;a=blob;f=libc/unistd/mmap.c

(I've wanted to send a patch to Android.)

The mmap man page says:
http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html

    The off argument is constrained to be aligned and sized
    according to the value returned by sysconf() when passed
    _SC_PAGESIZE or _SC_PAGE_SIZE.

and

    [EINVAL]
        The addr argument (if MAP_FIXED was specified) or off
        is not a multiple of the page size as returned by
        sysconf(), or is considered invalid by the implementation.

So the way we handle an 'offset' that isn't a multiple of
ANDROID_PAGE_SIZE meets the requirement of the spec.

Comment 21

[Wan-Teh Chang]

Comment on attachment 510619 [details] [diff] [review]
patch v3 (checked in)

Patch checked in on the NSPR trunk (NSPR 4.8.8).

Checking in pr/include/md/_linux.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.h,v  <--  _linux.h
new revision: 3.59; previous revision: 3.58
done
Checking in pr/src/md/unix/unix.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/unix.c,v  <--  unix.c
new revision: 3.59; previous revision: 3.58
done

Comment 22

[Wan-Teh Chang]

I'm not sure if we can mark this bug FIXED because
the bigfile2 and bigfile3 tests still fail on Android
Froyo, but I believe we're using the large file API
correctly and the failure is caused by Android itself.

Brad, can you run the tests on the latest version of
Android?

Marked the bug WONTFIX.

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

bigfile2 still fails on Android 2.3.3 (gingerbread) on my nexus one

Comment 24

[Wan-Teh Chang]

I did a lot of searches for large file support in Android.
I found no bug reports related to large file support in
the Android issue tracker.  The only thing useful I found
is a discussion thread on android-platform:
http://groups.google.com/group/android-platform/browse_thread/thread/e38ffbc35731e15f

We should write a simple test program that calls open()
and lseek64.  If it reproduces the EFBIG error, we should
submit a bug report in the Android issue tracker.

Comment 25

[Wan-Teh Chang]

Attachment: bigfile.c: standalone test program

Brad: this test program is based on NSPR's bigfile2.c
test program, but doesn't use NSPR.

Could you please run this test program on the latest
version of Android you have?  If it also fails, then
I'll submit a bug report to Android.  If it succeeds,
then the bug is in NSPR.

Thank you.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

on my nexus one running 2.3.3:

$ ./bigfile
lseek64 failed: 22

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

I also compiled it with ndk-r5 and platform level 9 (latest ndk versions) and got the same result.

Comment 28

[Wan-Teh Chang]

(In reply to comment #26)
Brad: thank you for testing it right away.

errno 22 is different from the errno 27 you reported in
comment 13, so either something changed in Android
between Froyo and 2.3.3, or my test program doesn't
simulate NSPR code faithfully.

Comment 29

[Wan-Teh Chang]

Brad: can you change the line
    offset <<= 32;
to
    offset <<= 16;
and see if it enables the test program to pass?

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

oh my i9000 running 2.2.1 (froyo) I get a different error:
$ ./bigfile
write failed: 38

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

on both 2.2.1 and 2.3.3, changing the offset to 16 allows the test to pass.

curiously it segfaults on exit though.