Closed Bug 9519 Opened 25 years ago Closed 22 years ago

Coding style - 64-bits CPU porting problems

Categories

(Core :: Internationalization, defect, P3)

Other
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: sh990154, Assigned: ftang)

Details

(Keywords: platform-parity, topembed)

Attachments

(3 files)

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
briano, is this a Unix platform?
Status: NEW → ASSIGNED
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.
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.
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.
> 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.
mass reassigning briano's open bugs to me while he's on sabbatical.
accept bug.
set to M13.
Target Milestone: M13 → M14
mass migration to M14
Keywords: pp
Target Milestone: M14 → M18
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
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
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.
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 ?
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.)
Summary: [PP]Coding style - porting problems → Coding style - porting problems
*** Bug 28965 has been marked as a duplicate of this bug. ***
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
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.
mass re-assign of all bugs where i was listed as the qa contact
QA Contact: cyeh → chofmann
reassign to cata and add jebetak to the cc list.
Assignee: ftang → cata
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: M20 → Future
Keywords: helpwanted
move all cata's bug to ftang
Assignee: cata → ftang
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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 :(
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 → ---
this patch look reasable. Need more careful review. target m1.0
Target Milestone: --- → mozilla1.0
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
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
Oink.

/be
Keywords: mozilla1.0mozilla1.0+
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+
test ok on windows
work fine on both linux and mac. I think we should land this.
Blocks: 104148
ask brendan to sr= it.
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+
fixed and check in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Blocks: 104060
No longer blocks: 104148
No longer blocks: 104060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: