Can't expand chrome/*.jar files on ARM

RESOLVED FIXED in mozilla1.0

Status

()

P3
normal
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: kazhik, Assigned: jeroen.dobbelaere)

Tracking

Trunk
mozilla1.0
Other
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
If you build Mozilla for ARM and run it on ARM, nsZipArchive::BuildFileList()
fails on expanding chrome/*.jar files.

Size of ZipEnd in zipstruct.h becomes 24. But it should be 22.

Comment 1

17 years ago
This is not a build issue but I can't seem to find a libjar bugzilla component. :-/

What version of zip do you have installed on the system you used to compile mozilla?
Component: Build Config → XP Utilities

Updated

17 years ago
Priority: -- → P3
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 2

17 years ago
Version of zip is 2.3.

Comment 3

17 years ago
Here's a silly question:  How do you know that the size of ZipEnd is being
treated as 24 instead of 22 ?
(Reporter)

Comment 4

17 years ago
You can insert printf() in nsZipArchive::BuildFileList() and see sizeof(ZipEnd).


(Reporter)

Comment 5

17 years ago
On x86, sizeof(ZipEnd) is 22.


Comment 6

17 years ago
Just on a hunch, can you do the following:

rm -rf dist/bin/chrome

edit config/autoconf.mk and add -k to the end of the ZIP_PROG variable
rebuild and see if you still have the error
(Reporter)

Comment 7

17 years ago
The original reporter(in Bugzilla-jp, bug_id = 1091) tested the steps
Christopher Seawood suggested on 29 Jun. Nothing new happened.

Comment 8

17 years ago
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
(Assignee)

Comment 9

17 years ago
The reason for this behavior of gcc has to do with the ARM SDK :(

From gcc/gcc/config/arm/arm.h :

/* Setting STRUCTURE_SIZE_BOUNDARY to 32 produces more efficient code, but the
   value set in previous versions of this toolchain was 8, which produces more
   compact structures.  The command line option -mstructure_size_boundary=<n>
   can be used to change this value.  For compatability with the ARM SDK
   however the value should be left at 32.  ARM SDT Reference Manual (ARM DUI
   0020D) page 2-20 says "Structures are aligned on word boundaries".  */
#define STRUCTURE_SIZE_BOUNDARY arm_structure_size_boundary
extern int arm_structure_size_boundary;

/* This is the value used to initialise arm_structure_size_boundary.  If a
   particular arm target wants to change the default value it should change
   the definition of this macro, not STRUCTRUE_SIZE_BOUNDARY.  See netbsd.h
   for an example of this.  */
#ifndef DEFAULT_STRUCTURE_SIZE_BOUNDARY
#define DEFAULT_STRUCTURE_SIZE_BOUNDARY 32
#endif



And as far as I know (I did a rough check of the c++ draft), this
is allowed by c++, which means that the code depending on 
'sizeof(struct XXX) == an expected value' should be adapted :(

That also means we can't just read stuff off disk into a structure, at least not
stuff that we didn't write out ourselves like a zip archive.

Taking...  What's ARM?
Assignee: cls → dveditz
Component: XP Utilities → XP Miscellany
(Assignee)

Comment 11

17 years ago
Correct.

This also means that when you've received some data in bytes (possible from an
other architecture) (through file or http or ...),
that you cannot just use a struct of structs and expect the layout to match :(

example :

struct f1 {
  char a0;
  char b;
  char c;
};

struct f2 {
  char a1;
  char b;
  char d;
};

union u1 {
  struct f1 f1;
  struct f2 f2;
};

struct format {
  char a;
  char b;
  union u1 u;
  char end;
};

On ARM, the union u will start at a 4 byte boundary, because
of the alignment restrictions for struct.
Also, the size of the union is 4 bytes (not 3). Because of this,
'end' will also start on a 4 byte boundary.
The total size of 'format' is 12 bytes.
On an i386 system, the total size would be 6 bytes (!)


BTW. ARM is a company that has created a cpu risc architecture (known as ARM)
(See <http://www.arm.com/>) which is used in a lot of embedded systems.

(Assignee)

Comment 12

17 years ago
Created attachment 71657 [details] [diff] [review]
proposed patch : decode zipfile independent of sizeof(ZipEnd) etc.

The patch defines sizes of the zip-headers in the .h file, and uses those
to traverse the buffers, in stead of depending on 'sizeof(ZipEnd)' (etc...)
being a constant on all platforms.

Another solution on an ARM platform with gcc, would be to use the
'-mstructure-size-boundary=8' option. (for a better description, see bug 9519)
Patch is against mozilla-0.9.8
If the problem is that you're assuming that there is no padding, can't you just
use __attribute__((packed)) on the structure, for all platforms? Or does that
not handle trailing padding? (Well, use that on gcc, and the equivalent for
msvc/mac/etc) 'info gcc' has the docs on that attribute.

Any platform is allowed to use padding (6.7.2.1/15 in the C99 spec), so this is
technically a bug on all platforms, and if the structure is packed already, then
that attribtue shouldn't make a difference.

The patch here assumes that there is no padding between each unsigned char
element, and while thats probably a reasonable assumption (since sizeof(unsigned
char)==1, padding is sort of pointless), an implementation can add padding
anywhere it wants after any structure member.
(Assignee)

Comment 14

17 years ago
In this case, the problem is not the padding between members. This padding is
strictly specified in C. The problem is the trailing padding, making the
size (and alignment) of structs bigger.
'packed' would also help, but only for gcc. The proposed solution should
work with all compilers.
 
Sure, but your solution fails if the compiler puts padding between elements.

I'm resonably certain that msvc has a packed option.

My point is that this code is non-portable anyway. If you add an assertion that
sizeof(foo)==whatever somewhere, then at least this can be easily tracked, and
the slower member by member copy used for those cases, if they ever occur.

Will packed cause a SIGBUS for unaligned access on some platforms? With unsigned
char, I suspect not, and any platform which did require padding would have
failed for the same reason ARM is failing, right?
(Assignee)

Comment 16

17 years ago
I don't say other compilers don't have a 'packed' option, but it probably
has to be specified in a different way.

The structures in zipstruct.h all contain 'unsigned characters'.
As they are POD (plain old data), this means that each member
is guaranteed to be aligned to the smallest alignment (= alignment of 'char').

The only thing ARM adds, is the restriction that a struct has a size of n words,
and is word-aligned. This should/could only give problems (in case of unaligned
structs) when the struct as a whole is accessed (ex. duplication).
Access to a character member should not insert a SIGBUS.

The best (most portable) solution should be to never take
a bytestream and place a struct above it !
Right, thats the most portable, but its slower, so unless someone actually needs
it....

why not just use packed on ARM, and add an assertion so that others can find
this problem if they run into it?

In any event, please add a comment.
(Assignee)

Comment 18

17 years ago
Ok. I'll prepare a patch which will use the 'packed' attribute when
compiling with gcc, and an extra check to ensure that the structs have the
correct size.
 
Note that this is just my opinion; see what others think.
(Assignee)

Comment 20

17 years ago
Created attachment 72568 [details] [diff] [review]
alternative patch using __attribute__ ((packed))

This is an alternative patch using the __attribute__ ((packed)) alternative.

pro : - only one file to patch
con : - __attribute__ ((packed)) only works with gcc
      - size check can break builds with other compilers

IMHO, attachment 71657 [details] [diff] [review] is for this case the better solution to provide
a portable always working libjar.
(based on content of the structs and how they are used in the code).

Comment 21

17 years ago
I vote for the portable solution - the other one is just a hack which won't work
for non-gcc compilers...
(Assignee)

Comment 22

17 years ago
Created attachment 73687 [details] [diff] [review]
Updated proposed patch : decode zipfile independent of sizeof(ZipEnd) etc.

new version of attachment 71657 [details] [diff] [review] : update to current cvs
(Assignee)

Comment 23

17 years ago
Would it be possible to get this reviewed and fixed for 1.0 ?
This is the only remaining problem that needs to be
fixed in order to get a working build for arm-linux.

(Other problems were :
 bug 9519 and 
 bug 106864 (patch is applied but bug is not marked yet as resolved)
)
I'll add it to my review queue, should be 24hrs or so.
Sorry for the delay, it's been a crazy week.

I'd like comments in the .h file where you define the struct sizes explaining
why you're doing what looks like an ugly, crazy thing instead of a simple
sizeof, so some helpful soul doesn't "fix" it in the future.

Something along the lines...

// sizeof <struct> includes padding on Linux ARM and others; see bug 87965

r=dveditz
Status: NEW → ASSIGNED
(Assignee)

Comment 26

17 years ago
Created attachment 76550 [details] [diff] [review]
Final proposal - replace sizeof(struct) with #define

This is an update of attachment 73687 [details] [diff] [review], with some more comments explaining
the need of a #define in stead of a sizeof(...).

mozilla-seamonkey (cvs version) has been compiled with it (for arm) and
poses no problems.
Attachment #71657 - Attachment is obsolete: true
Attachment #72568 - Attachment is obsolete: true
Attachment #73687 - Attachment is obsolete: true
Comment on attachment 76550 [details] [diff] [review]
Final proposal - replace sizeof(struct) with #define

r=dveditz
Attachment #76550 - Flags: review+
Comment on attachment 76550 [details] [diff] [review]
Final proposal - replace sizeof(struct) with #define

sr=brendan@mozilla.org -- file format defines should not depend on struct
sizeof (period, bbaetz! :-).

Let's get this in for 1.0.

/be
Attachment #76550 - Flags: superreview+
(Assignee)

Comment 29

17 years ago
Mozilla-1.0rc1 + attachment 76550 [details] [diff] [review] works for me on our xscale platform.
(using gcc-3.0.4+float reload patch)

Comment 30

17 years ago
we should get branch approval for this
Assignee: dveditz → jeroen.dobbelaere
Status: ASSIGNED → NEW
Keywords: mozilla1.0
Target Milestone: mozilla1.0.1 → mozilla1.0

Comment 31

17 years ago
fixed on trunk
i'll commit the same thing to branch if/when given an a=
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 32

17 years ago
According to brad TBox (see
http://tinderbox.mozilla.org/SeaMonkey/warn1020448680.25229.html) this commit
added two warnings:

3-4. modules/libjar/nsZipArchive.cpp:1094 (See 1st of 2 warnings in build log)
Comparison between signed and unsigned

1092
1093     //-- make sure we've read enough
1094     if ( (PRUint32)bufsize < pos + ZIPCENTRAL_SIZE )
1095     {
1096       status = ZIP_ERR_CORRUPT;

Comment 33

17 years ago
this is caused by:
 878 dveditz  1.1    PRInt32  pos = 0L;
we can't simply make it unsigned because of:
 882                 pos = PR_Seek(mFd, 0, PR_SEEK_END);
 883               #ifndef STANDALONE
 884                 if (pos <= 0)
 885               #else
 886                 if (pos || ((pos = ftell(mFd)) <= 0))

Jeroen Dobbelaere: do you want to remove the cast to unsigned?
1094     if ( (PRUint32)bufsize < pos + ZIPCENTRAL_SIZE )

Aleksey Nogin: I can't figure out what the second warning is. My guess would be 
that pos and ZIPCENTRAL_SIZE aren't of the same type, but ...
(Assignee)

Comment 34

17 years ago
Created attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

This patch removes the two warnings which were introduced by attachment 76550 [details] [diff] [review]
(sorry for that :( )
Note : attachment 76550 [details] [diff] [review] must be applied first.

More specifically :
This patch removes the cast to '(PRUint32)' on line 1094
And it adds a cast to '(int)' of variable status in line 1104 :

1101 #if defined(DEBUG)
1102   if (status != ZIP_OK) {
1103	 const char* msgs[] = { "ZIP_OK", "ZIP_ERR_GENERAL", "ZIP_ERR_MEMORY",
"ZIP_ERR_DISK", "ZIP_ERR_CORRUPT", "ZIP_ERR_PARAM", "ZIP_ERR_FNF",
"ZIP_ERR_UNSUPPORTED", "ZIP_ERR_SMALLBUF", "UNKNOWN" };
1104	 printf("nsZipArchive::BuildFileList  status = %d '%s'\n", (int)status,
msgs[(status <= 0 && status >= -8) ? -status : 9]);
1105   }
1106 #endif

Comment 35

17 years ago
i'll use PRInt32 instead of int, but sure that looks fine w/ me.

i wonder if brendan has a policy on trivial warning fixes.
(Assignee)

Comment 36

17 years ago
Using PRInt32 probably won't work : the second warning is there 
because the string format '%d' is matched against the parameter status
(of type PRInt32). Depending on the header files and the target architecture,
this can be converted into 'long', 'int' or even 'short' (on 64bit architectures)
To be sure that gcc doesn't give a warning, it should be converted explicitely
to 'int' in order to match %d), or to long (together with changing %d into %ld).

Comment 37

17 years ago
Comment on attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

ok
Attachment #82378 - Flags: review+

Comment 38

17 years ago
Comment on attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #82378 - Flags: approval+
Comment on attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

sr=jst
Attachment #82378 - Flags: superreview+
*** Bug 152953 has been marked as a duplicate of this bug. ***

Updated

10 years ago
Component: XP Miscellany → General
QA Contact: granrosebugs → general

Updated

10 years ago
Component: General → Networking: JAR
QA Contact: general → networking.jar
You need to log in before you can comment on or make changes to this bug.