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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: yuanyi21, Assigned: yuanyi21)
References
Details
Attachments
(2 files, 2 obsolete files)
519 bytes,
text/html
|
Details | |
1.36 KB,
patch
|
roland.mainz
:
review+
roc
:
superreview+
mkaply
:
approval1.7+
|
Details | Diff | Splinter Review |
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.
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.
Comment 2•21 years ago
|
||
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
(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 5•21 years ago
|
||
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= ...
Comment 6•21 years ago
|
||
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 8•21 years ago
|
||
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 10•21 years ago
|
||
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+
Assignee | ||
Comment 11•21 years ago
|
||
(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+
Comment 12•21 years ago
|
||
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 13•21 years ago
|
||
Comment on attachment 145668 [details] [diff] [review]
using __SUNPRO_CC instead of SOLARIS
Requesting a= for mozilla 1.7 ...
Attachment #145668 -
Flags: approval1.7?
Comment 14•21 years ago
|
||
This really fixes it? A good compiler would optimize this away.
Wouldn't it make more sense to simply not optimize this file?
Assignee | ||
Comment 15•21 years ago
|
||
(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 16•21 years ago
|
||
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+
Comment 17•21 years ago
|
||
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.
Comment 18•21 years ago
|
||
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...
Assignee | ||
Comment 19•21 years ago
|
||
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
Comment 20•21 years ago
|
||
Kyle Yuan wrote:
> Anyway, I will re-check whether there is already a
> bug filed to the compiler team.
Any news here ?
Assignee | ||
Comment 21•20 years ago
|
||
*** 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.
Description
•