Closed
Bug 9519
Opened 25 years ago
Closed 22 years ago
Coding style - 64-bits CPU porting problems
Categories
(Core :: Internationalization, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla1.0
People
(Reporter: sh990154, Assigned: ftang)
Details
(Keywords: platform-parity, topembed)
Attachments
(3 files)
4.91 KB,
patch
|
Details | Diff | Splinter Review | |
7.91 KB,
patch
|
ftang
:
review+
|
Details | Diff | Splinter Review |
5.01 KB,
patch
|
brendan
:
review+
brendan
:
superreview+
roc
:
approval+
|
Details | Diff | Splinter Review |
I've encountered some problems while porting mozilla to the ARM platform. The problem is well known to us and is about structure alignment. In general, structure alignment should not be a problem, if the structure elements are all adressed by their names. On our platform structures are generally aligned to 32bit - there should be no problem with that - the C/C++ standard says that structures have to be *at least* aligned to a boundary that corresponds with the smallest element in this structure. In intl/uconv a problem is encountered because structures that themselves include structures that are a multiple of 16 bits (not 32) are initialized from an array of PRInt32 which will result in data corruption (because that included structures are aligned to a multiple of 32 bits). C/C++ allows this - it would good if this problem could be taken care of in future releases.
Assignee: don → briano
QA Contact: leger → cyeh
Summary: Coding style - porting problems → [PP]Coding style - porting problems
Updated•25 years ago
|
Status: NEW → ASSIGNED
Comment 2•25 years ago
|
||
Yes, Linux/ARM is a Unix platform. So is NetBSD/ARM. I have a Corel Netwinder running Linux/ARM, and a DEC SHARK running NetBSD. Both are running Tinderbox builds on the SeaMonkey-Ports page. I am cc'ing the right people in NSPR and L10N to help look at this.
adding cata since he is responsible for the unicode converter classes now. cata - this is an unsupported platform but it would be great if we could address this problem with portability.
Comment 4•25 years ago
|
||
I don't know what I'd need to do to fix this. Is it strictly NSPR, or does it have deeper roots than that? CC'ing brendan and scc for any input they might be able to provide.
Comment 5•25 years ago
|
||
Can you give the names of the files that have the structure alignment problems you described? Without this specific info, the person assigned to fix this bug can't do anything about it. This is a real porting problem that may be overlooked by many developers. It is a good idea to post an article to some mozilla newsgroup to inform people of this problem. However, a bug report needs to have more specifc info to be useful.
my understanding of the problem is that because intl/uconv does some memory manipulations in order to compact the size of the unicode conversion tables, it causes portability problems for platforms like ARM which require stricter alignment rules than what most of our ports run on. i didn't think NSPR was part of this at all - the problems are with the unicode table manipulations.
Assignee | ||
Comment 7•25 years ago
|
||
> In intl/uconv a problem is encountered because structures that themselves
> include
> structures that are a multiple of 16 bits (not 32) are initialized from an
> array of PRInt32 which will result in data corruption
Hum.... Which one ? Which structure. I am willing to help to solve the issue if
people could give me specific information.
Comment 8•25 years ago
|
||
mass reassigning briano's open bugs to me while he's on sabbatical.
Comment 9•25 years ago
|
||
accept bug.
Comment 10•25 years ago
|
||
set to M13.
Updated•25 years ago
|
Target Milestone: M13 → M14
Comment 11•25 years ago
|
||
mass migration to M14
Updated•25 years ago
|
Target Milestone: M14 → M18
Comment 12•25 years ago
|
||
reassigning this to ftang. The issues he has are inside of mostly uconv. It certainly doesn't belong with granrose.
Assignee: granrose → ftang
Status: ASSIGNED → NEW
Assignee | ||
Comment 13•25 years ago
|
||
I will close this bug in Feb 12, 2000 if no one give me more details by that. I have ask the specific information since Aug 24, 1999 but no one bother to put in details. Without these details, this bug is meaningless. In specific, I need to know the name of the struct and file.
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•25 years ago
|
||
Subject: Re: [Bug 9519] Changed - [PP]Coding style - porting problems Date: Wed, 9 Feb 2000 22:23:54 +0100 (MET) From: "Hanske;Stefan" <sh990154@mail.uni-greifswald.de> To: ftang@netscape.com On Wed, 9 Feb 2000 bugzilla-daemon@mozilla.org wrote: > + ------- Additional Comments From ftang@netscape.com 2000-02-09 11:02 ------- > + I will close this bug in Feb 12, 2000 if no one give me more details by that. I > + have ask the specific information since Aug 24, 1999 but no one bother to put in > + details. Without these details, this bug is meaningless. > + In specific, I need to know the name of the struct and file. > + Hey, stop. I think I've made this problem clearer some time ago. The problem hits in three files: intl/uconv/src/umap.c " uscan.c " ugen.c. And it is about the following: Structure alignment on ARM system C/C++-compilers is 32 bits. Description of the problem: In this .c files the normal declaration for conversion data is included. These consist of a structure that does itself include structures (not pointers to structures but the structs itself), that are *not* 32-bit aligned *by declaration*. They do have an alignment of 8 or 16 bits. Example: struct foo { int32 dummy1; struct { int8 dummy2; int8 dummy3; } dummy_struct; int32 dummy4; } results in the final layout in the following offsets: dummy1: 0 dummy_struct.[dummy2, dummy3]: [4, 5] ***** AND HERE ARE 2 ALIGNMENT BYTES ***** dummy4: 8 So when accessing anything behind dummy_struct care has to be taken because the data has been realigned after dummy_struct. The cause of the problem: The unicode (and other converters) setup the structure in the following way (I've only watched a few): The number of 16-bit values needed is calculated and the tables filled in in the source code (the translation tables are hard coded) as an array of 16-bit integers. This will lead to reading wrong data and beyond the structure boundaries, since the two layouts (translation tables and arrays of int16) differ, because the compiler performed an additional alignment of the included structs in the declaration of the translation tables. Macroskopic (visible) effects: When some HTML/CSS/* code is read my mozilla, all is going through the converters, which will call the conversion routines in the above mentioned .c-files. This routines just output sh*t which confuses the HTML/CSS/*-code that much that segfaults occur (at startup already). Possible solutions: 1.) rewrite the .c files to use the same layout as the translation tables that are defined in the converters .cpp-files. This might not work if different converters use different initialisation strategies. 2.) rewrite the .cpp files of every converter to layout the translation tables as defined in the header files. This will work in every case but will certainly increase memory requirements on systems like ours. 3.) **NASTY** ARM gcc contains a machine dependend flag -mstructure-size-boundary=<value_in_bits> which solves the problems when applied to the OS_CFLAGS in intl/uconv/src/Makefile. This is nasty and only works by chance because actually every code (all shared libraries that are used and simply everything that shares structures) should be compiled with this flag. It only works because the functions in these .c files are isolated from every I/O functionality. It is neither sure that this ARM specific flag will remain in future versions of gcc nor that the code will not be affected by updates which kill the chance in "by chance". Other notes: The behaviour of structure alignment has often been discussed simply because the compilers which behave "our" way are seldom. However, we're not doing wrong!!! ANSI C/C++ allows generous alignments as long as it is not *smaller* than required. Code developers have thereby to live with the fear that the structure alignment may differ (this means grow only) for different ANSI (or GNU, too) compilers. ARM CPU's and memory management hardware does not in every case support halfword load instructions and words can only be fetched from a word boundary (32-bit alignment). We'd have do do 4 single byte load instructions in the worst case (or two word fetches with correct shifting and masking/combining) to get a word from a non word aligned address. This is that much a performance impact that it's not liable. That's why ARM compilers behave the way they do. I hope this is enough info. If further information is required, let me know.
Assignee | ||
Comment 15•25 years ago
|
||
Hanske- Thanks for your info. However, you still have not in particular tell me "the name of the struct and file." . I appreciate if you can point out exactly which structure you mean. Are you talking about uFormat0, uFormat1, uFormat2 , uTable in umap.h ? Any other struct ?
Reporter | ||
Comment 16•25 years ago
|
||
Yes, for instance. Another example is in intl/uconv/public/uconvutil.h, lines 67-84, declaration of struct uShiftCell. Here struct uShiftIn is two bytes long, it will be aligned to be 4 bytes long (and maybe to start at a word boundary, but I'm not sure 'bout that). This will shift the member uShiftOut at least 2 (or 4) bytes backwards. Otherwise, in intl/uconv/ucvcn/nsGB2312ToUnicode.cpp, lines 24-29, the following is written: 24 //Global functions and data [declaration] 25 26 static PRInt16 g_ASCIIShiftTable[] = { 27 0, u1ByteCharset, 28 ShiftCell(0,0,0,0,0,0,0,0) 29 }; The macro ShiftCell produces 4 16-bit values wich follow exactly one after another. So the declaration declares something that is different from the structure gcc layouts by the means of the header file. This is tracable through all the code in intl/uconv. I've forgotten already how the .ut and .uf files work. But there may be problems too. There may be a more simpler solution than the ones mentioned in my last mail. There are rumours that __attribute__((__packed__)) and __attribute__((unaligned__)) are likely to solve those problems in a more elegant way if added to structure definitions. But this will make the code much slower on ARM systems than the other, more reliable and cleaner solutions 1.) and 2.)
Updated•25 years ago
|
Summary: [PP]Coding style - porting problems → Coding style - porting problems
Assignee | ||
Comment 17•24 years ago
|
||
*** Bug 28965 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 18•24 years ago
|
||
Need help. Don't know how to make it 64-bits clean. This is a aligment issue. Unfortunately, 64-bits machine is not part of the reference platform and I have access to one. I won't have time to fix this problem. It will be nice if someone can take over this bug.
Summary: Coding style - porting problems → Coding style - 64-bits CPU porting problems
Whiteboard: NEED HELP
Target Milestone: M18 → M20
Assignee | ||
Comment 19•24 years ago
|
||
add jdunn to the cc. jdunn, these code are more-or-less straight port from 4.x code base. How we handle the DEC alpha port in the 4.x tree ? Please help us.
The 4.x port used the -taso option, but is this really a 64-bit clean problem? It sounds like it's ``just'' a structure alignment issue, on a 32-bit platform.
Comment 21•24 years ago
|
||
mass re-assign of all bugs where i was listed as the qa contact
QA Contact: cyeh → chofmann
Assignee | ||
Comment 22•24 years ago
|
||
reassign to cata and add jebetak to the cc list.
Assignee: ftang → cata
Status: ASSIGNED → NEW
Assignee | ||
Updated•24 years ago
|
Target Milestone: M20 → Future
Updated•24 years ago
|
Keywords: helpwanted
Assignee | ||
Comment 23•24 years ago
|
||
move all cata's bug to ftang
Assignee: cata → ftang
Status: ASSIGNED → NEW
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 24•22 years ago
|
||
This patch contains a proposed solution to make the code '32-bit clean', so that it should work on 'arm' without the need of -mstructure-size-boundary=8. Platforms that need structures to be 64-bit aligned will have a more difficult time :(
Comment 25•22 years ago
|
||
Missed the alignment of 'uMapCell' in patch 71455. This patch obsoletes attachment 71455 [details] [diff] [review]
Component: Browser-General → Internationalization
Whiteboard: NEED HELP
Target Milestone: Future → ---
Assignee | ||
Comment 26•22 years ago
|
||
this patch look reasable. Need more careful review. target m1.0
Target Milestone: --- → mozilla1.0
Comment 27•22 years ago
|
||
The patch looks good to me, and is the last step to get ARM platforms running Mozilla far and wide. I don't think the code review bottleneck will be narrow -- Frank, can you r= or recommend someone who will? Testing this sort of patch on many platforms is as important as reviewing it well. Who all is going to try the patch? I will on RH7.1 Linux/x86. /be
Comment 28•22 years ago
|
||
Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+, topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword. Please send any questions or feedback about this to adt@netscape.com. You can search for "Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla1.0 → mozilla1.2
Target Milestone: mozilla1.2 → mozilla1.0
Assignee | ||
Comment 30•22 years ago
|
||
Comment on attachment 71640 [details] [diff] [review] update proposal patch r=ftang looks right, But I need to try the patch on Windows and Mac
Attachment #71640 -
Flags: review+
Assignee | ||
Comment 31•22 years ago
|
||
test ok on windows
Assignee | ||
Comment 32•22 years ago
|
||
Assignee | ||
Comment 33•22 years ago
|
||
work fine on both linux and mac. I think we should land this.
Assignee | ||
Comment 34•22 years ago
|
||
ask brendan to sr= it.
Comment 35•22 years ago
|
||
Comment on attachment 72968 [details] [diff] [review] same patch in "cvs diff -u" form so my maccvs patch command can accept it. Carrying r= over to this diff -u, adding sr=brendan@mozilla.org. /be
Attachment #72968 -
Flags: superreview+
Attachment #72968 -
Flags: review+
Comment on attachment 72968 [details] [diff] [review] same patch in "cvs diff -u" form so my maccvs patch command can accept it. a=roc+moz for trunk
Attachment #72968 -
Flags: approval+
Assignee | ||
Comment 37•22 years ago
|
||
fixed and check in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•22 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•