239962 - Incorrect table layout caused by Sun compiler with -xO5

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Kyle Yuan]

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.

Comment 1

[Kyle Yuan]

Attachment: 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.

Comment 2

[Roland Mainz]

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 ?

Comment 3

[Kyle Yuan]

Attachment: workaround patch

Comment 4

[Kyle Yuan]

(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

[Roland Mainz]

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 [e-mail] for sr= ...

Comment 6

[Roland Mainz]

s/the next one who touches the bug /the next one who touches the file /

Comment 7

[Kyle Yuan]

Attachment: the changes should be SOLARIS only

Comment 8

[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...

Comment 9

[Kyle Yuan]

Attachment: using __SUNPRO_CC instead of SOLARIS

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.

Comment 10

[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=[e-mail]

Comment 11

[Kyle Yuan]

(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.

Comment 12

[Roland Mainz]

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

[Roland Mainz]

Comment on attachment 145668 [details] [diff] [review]
using __SUNPRO_CC instead of SOLARIS

Requesting a= for mozilla 1.7 ...

Comment 14

[Mike Kaply [:mkaply]]

This really fixes it? A good compiler would optimize this away.

Wouldn't it make more sense to simply not optimize this file?

Comment 15

[Kyle Yuan]

(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

[Mike Kaply [:mkaply]]

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

Comment 17

[Roland Mainz]

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

[Roland Mainz]

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...

Comment 19

[Kyle Yuan]

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.

Comment 20

[Roland Mainz]

Kyle Yuan wrote:
> Anyway, I will re-check whether there is already a
> bug filed to the compiler team.

Any news here ?

Comment 21

[Kyle Yuan]

*** Bug 185949 has been marked as a duplicate of this bug. ***