failures on bigfile2 and bigfile3 tests on Android

RESOLVED FIXED in 4.8.8

Status

P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: bcombee, Assigned: wtc)

Tracking

other
4.8.8
ARM
Android

Firefox Tracking Flags

(fennec-)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Summary: failures on bigfile2 and bigfile3 tests → failures on bigfile2 and bigfile3 tests on Android
tracking-fennec: --- → 2.0+

Comment 1

8 years ago
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?
Even though we want these to be fixed, should this bug block fennec?
not blocking
tracking-fennec: 2.0+ → 2.0-
Created attachment 477396 [details] [diff] [review]
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.
Attachment #477396 - Flags: review?
Attachment #477396 - Flags: review? → review?(wtc)
(Assignee)

Comment 5

8 years ago
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?
(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.
(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
(Assignee)

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
Created attachment 508045 [details] [diff] [review]
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.
Attachment #477396 - Attachment is obsolete: true
Attachment #508045 - Flags: review?(blassey.bugs)
Attachment #477396 - Flags: review?(wtc)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 4.8.8
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?
I should note that I tested on the nexus one running froyo
(Assignee)

Comment 12

8 years ago
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
errno is 27, "File too large"
(Assignee)

Comment 14

8 years ago
Created attachment 510619 [details] [diff] [review]
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.
Attachment #508045 - Attachment is obsolete: true
Attachment #508045 - Flags: review?(blassey.bugs)
(Assignee)

Comment 15

8 years ago
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.
(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
(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
(Assignee)

Comment 18

8 years ago
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.
Attachment #510619 - Flags: review?(blassey.bugs)
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?
(Assignee)

Comment 20

8 years ago
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.
Attachment #510619 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 21

8 years ago
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
Attachment #510619 - Attachment description: patch v3 → patch v3 (checked in)
(Assignee)

Comment 22

8 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
bigfile2 still fails on Android 2.3.3 (gingerbread) on my nexus one
Resolution: WONTFIX → FIXED
(Assignee)

Comment 24

8 years ago
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.
(Assignee)

Comment 25

8 years ago
Created attachment 519556 [details]
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.
on my nexus one running 2.3.3:

$ ./bigfile
lseek64 failed: 22
I also compiled it with ndk-r5 and platform level 9 (latest ndk versions) and got the same result.
(Assignee)

Comment 28

8 years ago
(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.
(Assignee)

Comment 29

8 years ago
Brad: can you change the line
    offset <<= 32;
to
    offset <<= 16;
and see if it enables the test program to pass?
OS: Android → All
(Assignee)

Updated

8 years ago
OS: All → Android
oh my i9000 running 2.2.1 (froyo) I get a different error:
$ ./bigfile
write failed: 38
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.
You need to log in before you can comment on or make changes to this bug.