Closed Bug 145267 Opened 22 years ago Closed 22 years ago

gcc 3.1 optimization problems in nsFrame::GetBidiProperty

Categories

(SeaMonkey :: Build Config, defect)

Product:
SeaMonkey
Component:
Build Config
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED WONTFIX

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: relnote)

Attachments

(1 file)

simplified testcase for optimization problem
368 bytes, text/plain
Details 
My gcc 3.1 -O2 build always draws the caret in the wrong place.  printf
debugging led to the problem being at the end of
nsTextFrame::GetPointFromOffset.  When outPoint->x gets filled in from width, it
just doesn't do anything.
Well, I can certainly say that the basic block reordering makes this function
fun to read.  Since I had a printf near where the correct value should have been
filled in, I'll start at that point.

gcc 3.0.4 does (in the |else| near the end that ought to have braces around it):

    70b1:       8b 4d 08                mov    0x8(%ebp),%ecx
    70b4:       8b 95 6c fb ff ff       mov    0xfffffb6c(%ebp),%edx
    70ba:       8b 41 0c                mov    0xc(%ecx),%eax
    70bd:       39 c2                   cmp    %eax,%edx
    70bf:       7e 07                   jle    70c8 <nsTextFrame::GetPoi
    70c1:       8b 55 18                mov    0x18(%ebp),%edx
    70c4:       89 02                   mov    %eax,(%edx)
    70c6:       eb 86                   jmp    704e <nsTextFrame::GetPoi
    70c8:       8b 4d 18                mov    0x18(%ebp),%ecx
    70cb:       89 11                   mov    %edx,(%ecx)
    70cd:       e9 7c ff ff ff          jmp    704e <nsTextFrame::GetPoi

which jumps to a list of destructor calls (presumably):

    704e:       8b 45 18                mov    0x18(%ebp),%eax
    7051:       83 ec 0c                sub    $0xc,%esp
    7054:       c7 40 04 00 00 00 00    movl   $0x0,0x4(%eax)
    705b:       8d 85 98 fb ff ff       lea    0xfffffb98(%ebp),%eax
    7061:       50                      push   %eax
    7062:       e8 fc ff ff ff          call   7063 <nsTextFrame::GetPoi
    7067:       8d 85 c8 fc ff ff       lea    0xfffffcc8(%ebp),%eax
    706d:       89 04 24                mov    %eax,(%esp,1)
    7070:       e8 fc ff ff ff          call   7071 <nsTextFrame::GetPoi
    7075:       8d 85 d8 fc ff ff       lea    0xfffffcd8(%ebp),%eax
    707b:       89 04 24                mov    %eax,(%esp,1)
    707e:       e8 fc ff ff ff          call   707f <nsTextFrame::GetPoi
    7083:       8d 85 e8 fc ff ff       lea    0xfffffce8(%ebp),%eax
    7089:       89 04 24                mov    %eax,(%esp,1)
    708c:       e8 fc ff ff ff          call   708d <nsTextFrame::GetPoi
    7091:       8d 85 88 fe ff ff       lea    0xfffffe88(%ebp),%eax
    7097:       89 04 24                mov    %eax,(%esp,1)
    709a:       e8 fc ff ff ff          call   709b <nsTextFrame::GetPoi
    709f:       8d 45 98                lea    0xffffff98(%ebp),%eax
    70a2:       89 04 24                mov    %eax,(%esp,1)
    70a5:       e8 fc ff ff ff          call   70a6 <nsTextFrame::GetPoi
    70aa:       31 c0                   xor    %eax,%eax
    70ac:       e9 eb fd ff ff          jmp    6e9c <nsTextFrame::GetPoi

which then goes to:

    6e9c:       83 c4 10                add    $0x10,%esp
    6e9f:       8d 65 f4                lea    0xfffffff4(%ebp),%esp
    6ea2:       5b                      pop    %ebx
    6ea3:       5e                      pop    %esi
    6ea4:       5f                      pop    %edi
    6ea5:       5d                      pop    %ebp
    6ea6:       c3                      ret    


gcc 3.1 does this:


    77d0:       8b 4d 08                mov    0x8(%ebp),%ecx
    77d3:       8b 95 6c fb ff ff       mov    0xfffffb6c(%ebp),%edx
    77d9:       8b 41 0c                mov    0xc(%ecx),%eax
    77dc:       39 c2                   cmp    %eax,%edx
    77de:       7e 07                   jle    77e7 <nsTextFrame::GetPoi
    77e0:       8b 55 18                mov    0x18(%ebp),%edx
    77e3:       89 02                   mov    %eax,(%edx)
    77e5:       eb 80                   jmp    7767 <nsTextFrame::GetPoi

which then brings us to:

    7767:       8b 45 18                mov    0x18(%ebp),%eax
    776a:       c7 40 04 00 00 00 00    movl   $0x0,0x4(%eax)
    7771:       8b 95 54 fb ff ff       mov    0xfffffb54(%ebp),%edx
    7777:       89 14 24                mov    %edx,(%esp,1)
    777a:       e8 fc ff ff ff          call   777b <nsTextFrame::GetPoi
    777f:       8b 8d 58 fb ff ff       mov    0xfffffb58(%ebp),%ecx
    7785:       89 0c 24                mov    %ecx,(%esp,1)
    7788:       e8 fc ff ff ff          call   7789 <nsTextFrame::GetPoi
    778d:       8d 85 d8 fc ff ff       lea    0xfffffcd8(%ebp),%eax
    7793:       89 04 24                mov    %eax,(%esp,1)
    7796:       e8 fc ff ff ff          call   7797 <nsTextFrame::GetPoi
    779b:       8b 85 5c fb ff ff       mov    0xfffffb5c(%ebp),%eax
    77a1:       89 04 24                mov    %eax,(%esp,1)
    77a4:       e8 fc ff ff ff          call   77a5 <nsTextFrame::GetPoi
    77a9:       8b 95 60 fb ff ff       mov    0xfffffb60(%ebp),%edx
    77af:       89 14 24                mov    %edx,(%esp,1)
    77b2:       e8 fc ff ff ff          call   77b3 <nsTextFrame::GetPoi
    77b7:       8b 8d 64 fb ff ff       mov    0xfffffb64(%ebp),%ecx
    77bd:       89 0c 24                mov    %ecx,(%esp,1)
    77c0:       e8 fc ff ff ff          call   77c1 <nsTextFrame::GetPoi
    77c5:       31 c0                   xor    %eax,%eax
    77c7:       e9 8f fd ff ff          jmp    755b <nsTextFrame::GetPoi

and thus:

    755b:       81 c4 cc 04 00 00       add    $0x4cc,%esp
    7561:       5b                      pop    %ebx
    7562:       5e                      pop    %esi
    7563:       5f                      pop    %edi
    7564:       5d                      pop    %ebp
    7565:       c3                      ret    

In the first of the blocks from gcc 3.1 I skipped the end:

    77e7:       8b 4d 18                mov    0x18(%ebp),%ecx
    77ea:       89 11                   mov    %edx,(%ecx)
    77ec:       e9 76 ff ff ff          jmp    7767 <nsTextFrame::GetPoi

So, after all that, I still don't see anything wrong.
OK, I should have put more printfs in sooner.  The problem seems to be that
|width| is being clobbered (set to zero) around the call to GetBidiProperty.
OK, the real problem is in nsFrame::GetBidiProperty, where the |memset| is
clobbering more than the 1 bytes that it's asked to clobber.
Summary: gcc 3.1 optimization problems in nsTextFrame::GetPointFromOffset → gcc 3.1 optimization problems in nsFrame::GetBidiProperty
This seems to be as simple as I could get it:
 * I had to use offset 3.
 * I had to cast to (void**) so the function parameter would be void**, not
void*.
 * I had to pass the 1 as a parameter.
dbaron@dbaron Linux (0) ~/gccbug/145267 > /usr/local/gcc-3.1/bin/gcc -O test.cpp
; ./a.out
BABABABA BABABABA
00BABABA BABA0000
dbaron@dbaron Linux (0) ~/gccbug/145267 > /usr/local/gcc-3.1/bin/gcc test.cpp ;
./a.out
BABABABA BABABABA
00BABABA BABABABA


Note that gcc 3.0.4 gives an internal compiler error on this particular testcase.

So, brendan, bbaetz -- is this our bug or gcc's?  I'm guessing it's gcc's, since
memset should do what it's supposed to, but could it be considered ours since
we're dealing with a variable in a void** and we only want 1 byte of what it
points to to be modified?

FWIW, a cvs gcc build from last night prints:

BABABABA BABABABA
00BABABA BABABABA

I don't have 3.1 arround, but does -fno-builtins fix it, perhaps?
-fno-builtins doesn't fix it.
What about -fno-strict-aliasing? I think the test case you have is valid, only
because going from a char to anything shouldn't break the aliasing rules, but
I'm not certain.

If you put do_memset into a separate file than main, and/or use -fno-inline,
does that fix it?
Neither -fno-inline nor -fno-strict-aliasing fixes the problem.  When I created
the testcase, I originally tried to make the memset inline, but that didn't work.
This is a better link to the gcc PR:
http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=6703

Glenn Nakamura added a comment to the PR pointing out a patch that fixes the
problem that is on the gcc trunk.
The gcc patch provided fixes the caret positioning for me.
*** Bug 145213 has been marked as a duplicate of this bug. ***
*** Bug 151857 has been marked as a duplicate of this bug. ***
*** Bug 147774 has been marked as a duplicate of this bug. ***
*** Bug 153717 has been marked as a duplicate of this bug. ***
*** Bug 156041 has been marked as a duplicate of this bug. ***
GCC 3.1.1 is out now and includes the fix.

Is the right thing to do WONTFIX this, and add something to the release notes
documenting gcc 3.1 as an unsuitable compiler?
INVALID rather than WONTFIX probably, but I don't see an issue with that.

I don't thik we need a configure check or anything like that, though.
OK, wontfix, and adding |relnote| keyword.  Is that enough to get noticed, or
should I email someone as well?

SUGGESTED RELEASE NOTE TEXT:

  gcc 3.1 should not be used to compile Mozilla on x86 platforms, since it has
  an optimization bug that prevents the caret from being drawn in the right
  place (and may cause other bugs).  See bug 145267.  Other versions of gcc,
  such as 3.0.4, 3.1.1, and 3.2, do not have this problem.
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: relnote
Resolution: --- → WONTFIX
v wontfix
Status: RESOLVED → VERIFIED
*** Bug 148723 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: