Closed Bug 239962 Opened 21 years ago Closed 21 years ago

Incorrect table layout caused by Sun compiler with -xO5

Categories

(Core :: Layout: Tables, defect)

Sun
Solaris
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: yuanyi21, Assigned: yuanyi21)

References

Details

Attachments

(2 files, 2 obsolete files)

If I use Sun workshop 6 update 2 (F6U2) to build mozilla and turn on the optimization to maximum -xO5, it will generate incorrect code at this line: http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableOuterFrame.cpp#1224 the value of |position->mWidth.GetPercentValue()| can not be correctly obtained. This is definitely a compiler bug and only happens when -xO5 turned on (-xO4 and lower works fine). However, I still want to work around this annoying issue with a very small patch.
Attached file testcase
With the correctly compiled code, this test case will show a text in a light-blue background just around it. With the incorrectly compiled code, the dark-blue table cell is also shown around the text.
Kyle: Can you try to make a standalone (e.g. small program which shows the problem without relying on the rest of Mozilla's codebase) testcase that we can escalate the problem and get the compiler team to fix the bug, please ?
Summary: Incorrect table layout caused by F6U2 -xO5 → Incorrect table layout caused by F6U2 -xO5
Attached patch workaround patch (obsolete) — Splinter Review
(In reply to comment #2) > Kyle: > Can you try to make a standalone (e.g. small program which shows the problem > without relying on the rest of Mozilla's codebase) testcase that we can escalate > the problem and get the compiler team to fix the bug, please ? I tried, but failed. The problem seems to be very context dependent.
Comment on attachment 145665 [details] [diff] [review] workaround patch > + float percent = position->mWidth.GetPercentValue(); > + switch (position->mWidth.GetUnit()) { Please add a small comment with this bug as reference to explain why this was moved out of the |switch()|-block. Otherwise the next one who touches the bug will clean the stuff up and undo this work. I can do the review, try roc@ocallahan.org for sr= ...
s/the next one who touches the bug /the next one who touches the file /
Attachment #145665 - Attachment is obsolete: true
Attachment #145666 - Flags: review?(roland.mainz)
Comment on attachment 145666 [details] [diff] [review] the changes should be SOLARIS only Mhhh... OK... |ifdef|'ing may be an overkill... but it's OK for me. Please change the |SOLARIS| to |__SUNPRO_CC| since the bug is Sun Workshop/Forte-specific and doesn't cover other Solaris compilers...
Attachment #145666 - Flags: review?(roland.mainz) → review-
Yes, you are right. I forgot other compilers on Solaris. |ifdef| is here to avoid the performance impact on other platform/compiler, though the impact is very tiny.
Attachment #145666 - Attachment is obsolete: true
Attachment #145668 - Flags: review?(roland.mainz)
Comment on attachment 145668 [details] [diff] [review] using __SUNPRO_CC instead of SOLARIS > // Bug 239962, this is a workaround to avoid incorrect code generation by Sun compiler. Nit: s/by Sun compiler/by the Sun Forte 6U2 compiler/ No need to file a new patch for that unless superreview wants to see one, you can fix that when you commit r=roland.mainz@nrubsig.org
Attachment #145668 - Flags: superreview?(roc)
Attachment #145668 - Flags: review?(roland.mainz)
Attachment #145668 - Flags: review+
(In reply to comment #10) > Nit: > s/by Sun compiler/by the Sun Forte 6U2 compiler/ > Thanks, Roland. But this does not only happen with Forte6U2, but also with the new compiler - Workshop 8 (or whatever people call it). That's my fault. I should change the summary.
Status: NEW → ASSIGNED
Summary: Incorrect table layout caused by F6U2 -xO5 → Incorrect table layout caused by Sun compiler with -xO5
Attachment #145668 - Flags: superreview?(roc) → superreview+
Kyle Yuan wrote: > Thanks, Roland. But this does not only happen with Forte6U2, but also with the > new compiler - Workshop 8 Official marketing name is currently "Sun ONE compiler suite" (or similar stuff). May change every two weeks, people internally still call it "Sun Workshop", some "Sun Forte". I am happy with s/by Sun compiler/by the Sun Forte compiler/ then, that's more or less the closest we can get (marketing has _NOW_ the time to complain: ... BEEP... BEEP... BEEP... BEEP... time over... =:-)
Comment on attachment 145668 [details] [diff] [review] using __SUNPRO_CC instead of SOLARIS Requesting a= for mozilla 1.7 ...
Attachment #145668 - Flags: approval1.7?
This really fixes it? A good compiler would optimize this away. Wouldn't it make more sense to simply not optimize this file?
(In reply to comment #14) > This really fixes it? A good compiler would optimize this away. > > Wouldn't it make more sense to simply not optimize this file? Yes, it really fixes it. I don't know how sun compiler optimizes, but my change can survive. Actually, if I put the |float percent = ...| inside the case statement, it will be optimized away. I think this fix is simpler than not optimize the whole file, and the performance impact is minimum.
Comment on attachment 145668 [details] [diff] [review] using __SUNPRO_CC instead of SOLARIS PLEASE make sure you have opened a bug with the compiler people to get this fixed. a=mkaply
Attachment #145668 - Flags: approval1.7? → approval1.7+
Michael Kaply (IBM) wrote: > This really fixes it? A good compiler would optimize this away Depends... the main difference between -xO4 and -xO5 is that -xO5 has some extra "rules" for rare cases where an optimisation may bring some perf. advantage (mainly for HPC applications). But it seems that one of the rules isn't correctly applied - kyle's patch works because it avoids that the matching rule is triggered.
Michael Kaply (IBM) wrote: > (From update of attachment 145668 [details] [diff] [review]) > PLEASE make sure you have opened a bug with the compiler people to get this > fixed. > a=mkaply I've already CC:'ed gonufer to have a look at the problem...
checked in. I remember I reported a bug against this bug 1 year ago, but it was closed due to some reason. Anyway, I will re-check whether there is already a bug filed to the compiler team.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Kyle Yuan wrote: > Anyway, I will re-check whether there is already a > bug filed to the compiler team. Any news here ?
*** Bug 185949 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: