Closed
Bug 411210
Opened 17 years ago
Closed 17 years ago
TT: fix compile errors with gcc in tamarin-tracing
Categories
(Tamarin Graveyard :: Build Config, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: zbynek.winkler, Assigned: stejohns)
References
Details
Attachments
(1 file, 5 obsolete files)
132.83 KB,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071128 Iceweasel/2.0.0.11 (Debian-2.0.0.11-1) Build Identifier: TT currently does not compile on Linux with gcc. I have a patch to fix it. Reproducible: Always
Reporter | ||
Comment 1•17 years ago
|
||
With this patch I am able to get a binary out (debug and release). However, the release build segfaults and the debug build reports assert at MMgc/GCDebugUnix.cpp:64 where I've put it because I am not sure how to implement _GCDebugBreak on linux (BTW: is it really necessary to have all these *Debug.* files that are almost the same?) Also there are some places where I was not really sure what the proper fix would be (there is this one weird place where I had to change some ints to unsigned to prevent this strange warning/error about assuming no overflow, there are places where I used __attribute__((unused)) that should be probably replaced by some proper macro magic). And there might be others. Anyway, at least it compiles ;-)
Reporter | ||
Comment 2•17 years ago
|
||
I must have been really tired yesterday. This patch fixes endless recursion in _GCDebugBreak(). But maybe there should be something like http://www.mozilla.org/unix/debugging-faq.html#assertion instead?
Assignee | ||
Comment 3•17 years ago
|
||
Cool. Looks fundamentally fine. I'll do my best to get this pushed into HG this week.
Comment 4•17 years ago
|
||
(In reply to comment #2) > But maybe there should be something like > http://www.mozilla.org/unix/debugging-faq.html#assertion > instead? Good idea, want to bugify the suggestion so we can track it?
Reporter | ||
Comment 5•17 years ago
|
||
With this patch and the patch (attachment 295975 [details] [diff] [review] bug 409021) I can build with gcc. The resulting binary runs up to Assertion failed: "((vic != __null))" ("/home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/Assembler.cpp":218)AvmAssert failed: 0 (/home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/nanojit.h:80) The backtrace at this point is: (gdb) bt #0 avmplus::_AvmDebugBreak (format=0x8121157 "AvmAssert failed: %s (%s:%d)") at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/AvmDebugUnix.cpp:66 #1 0x0808584e in nanojit::DebugBreak () at /home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/nanojit.h:80 #2 0x080b31c3 in nanojit::Assembler::registerAlloc (this=0xb6d94018, allow=268435456) at /home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/Assembler.cpp:218 #3 0x080b5789 in nanojit::Assembler::findRegFor (this=0xb6d94018, i=0xb6da0004, allow=268435456) at /home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/Assembler.cpp:549 #4 0x080b5bb7 in nanojit::Assembler::findSpecificRegFor (this=0xb6d94018, i=0xb6da0004, w=4294967292) at /home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/Assembler.cpp:534 #5 0x080c780c in nanojit::Assembler::gen (this=0xb6d94018, whichPass=nanojit::Assembler::kLoopVariantPass, reader=0xb6cdf1b0) at /home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/Assembler.cpp:1724 #6 0x080cbe84 in nanojit::Assembler::assemble (this=0xb6d94018, frag=0xb6d44630, reader=0xb6cdf1b0) at /home/zbynek/hg.mozilla.org/tamarin-tracing/nanojit/Assembler.cpp:913 #7 0x08073f06 in avmplus::Interpreter::eot (this=0xb6cc7034, state=@0xbfdb703c, lastop=19) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Interpreter.cpp:3276 #8 0x08077bcb in avmplus::Interpreter::taken (this=0xb6cc7034, state=@0xbfdb703c) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Interpreter.cpp:3118 #9 0x0807f57e in avmplus::Interpreter::inner (this=0xb6cc7034, f=0xb6cc7744, ip=0x8124007 "K{Q\025K{Q\026KH\004", sp=0xb6cc96ec, rp=0xb6cc7788) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Interpreter.cpp:2406 #10 0x08084eed in avmplus::Interpreter::interpScript (this=0xb6cc7034, global=0xb6d90038) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Interpreter.cpp:113 #11 0x08067a1e in avmplus::AvmCore::handleActionPool (this=0xb6cc7008, pool=0xb6ce2108, domainEnv=0xb6d2c378, toplevel=0xb6d48088, codeContext=0x0, p_thrower=@0xbfdb7838) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/AvmCore.cpp:351 #12 0x08067aff in avmplus::AvmCore::initTopLevel (this=0xb6cc7008, p_thrower=@0xbfdb7838) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/AvmCore.cpp:325 #13 0x08049e71 in avmshell::Shell::runShell (this=0xb6cc7008, p_thrower=@0xbfdb7838) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:307 #14 0x0804a8e9 in avmshell::Shell::main (this=0xb6cc7008, argc=1, argv=0xbfdb7dd4) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:589 #15 0x0804aaee in _main (argc=1, argv=0xbfdb7dd4) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:891 #16 0x0804ac50 in main (argc=Cannot access memory at address 0x0 ) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:1047 I don't see an easy fix for this so leave it to someone else. The register allocator cannot find its victim ;-).
Comment 6•17 years ago
|
||
Make to make sure the platform ifdefs enable AVMPLUS_IA32 and the various i386 specific parts in nanojit/*. TT works on intel mac which uses gcc, but there might be a mac ifdef somewhere that wants to be a gcc ifdef. I think the assert is triggered when there are no free registers even after something has been spilled, which could be from not intializing the register allocator correctly.
Reporter | ||
Comment 7•17 years ago
|
||
Unfortunately I do not see any obvious place. AVMPLUS_IA32 is defined by configure.py and thus in the generated Makefile.
Reporter | ||
Comment 8•17 years ago
|
||
Some fun with the debugger shows you were right. So this patch adds the definition of AVMPLUS_FASTCALL that follows the one for Mac. With this patch we get to another segmentation fault. The 'te1' variable is not NULL but it is not valid either. 1576 const byte* p1 = te1 ? te1->m_poolPos : NULL; (gdb) bt #0 0x0809d173 in avmplus::TraitsEnv::sameTraits (te1=0x28b7840f, te2=0xb6d6e9c8) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/TraitsEnv.cpp:1576 #1 0x0809bfdb in avmplus::Toplevel::toErrorString (this=0xb6d5c088, te=0xb6dc0a04) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Toplevel.cpp:173 #2 0x08079316 in avmplus::traitsenv2es (te=0xb6dc0a04, env=0xb6d689a8) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Interpreter.cpp:1635 #3 0x0808454a in avmplus::Interpreter::inner (this=0xb6cdb034, f=0xb6cdb798, ip=0x812332d "E\n\004H\005", sp=0xb6cdd71c, rp=0xb6cdb7d8) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/vm_fpu_interp.h:917 #4 0x080851cc in avmplus::Interpreter::interpScript (this=0xb6cdb034, global=0xb6da4038) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/Interpreter.cpp:113 #5 0x080679c4 in avmplus::AvmCore::handleActionPool (this=0xb6cdb008, pool=0xb6cf6108, domainEnv=0xb6d40378, toplevel=0xb6d5c088, codeContext=0x0, p_thrower=@0xbfa0a578) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/AvmCore.cpp:351 #6 0x08067aa5 in avmplus::AvmCore::initTopLevel (this=0xb6cdb008, p_thrower=@0xbfa0a578) at /home/zbynek/hg.mozilla.org/tamarin-tracing/core/AvmCore.cpp:325 #7 0x08049e7d in avmshell::Shell::runShell (this=0xb6cdb008, p_thrower=@0xbfa0a578) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:307 #8 0x0804a8ff in avmshell::Shell::main (this=0xb6cdb008, argc=1, argv=0xbfa0ab14) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:589 #9 0x0804ab04 in _main (argc=1, argv=0xbfa0ab14) at /home/zbynek/hg.mozilla.org/tamarin-tracing/shell/avmshell.cpp:891
Assignee | ||
Comment 9•17 years ago
|
||
sweet. (BTW, I'm working on integrating all your Linux patches now and getting it working, hopefully today or tomorrow)
Reporter | ||
Comment 10•17 years ago
|
||
Great! Thanks. So I just finally might get from the 'compiling' part to the 'finding out' part of how the trace compiler works which is what I wanted to do originally ;-).
Assignee | ||
Comment 11•17 years ago
|
||
FYI, I have made progress here; we had at least one latent bug that was in the "happened to work" category on Windows and Mac but broke tracing on Linux. I'm hot on the trail of a second one. Hopefully will have a fully happy Linux version within a day or two. (Interpreter mode is working on Linux but I'm not going to bother pushing the fix till tracing works as well.)
Assignee | ||
Updated•17 years ago
|
Attachment #295853 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Attachment #295971 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Attachment #295980 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Attachment #296204 -
Attachment is obsolete: true
Assignee | ||
Comment 12•17 years ago
|
||
Patch excludes machine-generated files using hg diff --exclude=core/vm_* --exclude=shell/shell_* --exclude=core/builtin_*
Assignee | ||
Comment 13•17 years ago
|
||
Consolidation of multiple patches to fix the Linux build. Linux now builds & passes sanity tests. -- fc.py didn't verify that a superword's declared in/out args matched the implementation, which could cause the flush-to-stack code to be inconsistent between Interpreter and Tracing modes. Now we verify that that the number and type of in/out args match. Also, calling functions that returned a Box type wasn't quite right in a way that made for compiler-dependent code; now we consistent use Box::RetType. Also corrected the "-superwords" option to actually work (to allow for disabling superwords). -- various makefile, configure, manifest etc issues related to Linux. -- Ensured that AVMPLUS_CDECL is not defined for Linux. It appears that we want exactly one of AVMPLUS_FASTCALL or AVMPLUS_CDECL to be declared, but the code did not enforce this; defining both causes incorrect stack popping in some situations in traced code, causing eventually crashing and/or other misbehavior. -- moved core/AvmDebugWin.cpp to platform/win32/AvmDebugWin.cpp -- rewrote nextnamens in Forth instead of using native function -- rewrote hasnext2 (Forth) using CASE to minimize redundant conditionals -- fixed incorrect superword in/out superword arg declaration for OP_setlocal -- fixed a few build errors in MMgc related to building ARM using RVCT compiler that crept into a previous merge -- corrected inconsistent verbose assembly output for i386 tracer; we sometimes used [%s] and sometimes (%s) for memory accesses. Now use (%s) consistently (makes reading/searching verbose output a little simpler)
Assignee | ||
Updated•17 years ago
|
Attachment #296903 -
Flags: review?(treilly)
Comment 15•17 years ago
|
||
Adding back #ifdef MMGC_DRC seems to undo a change Tom made to prepare for mult-allocator support. I'd prefer to see assembly traces using [] for addressing modes but if () is that much easier to grep for then I'll concede. (aren't both special in regex's?) How about we eradicate AVMPLUS_CDECL and use AVMPLUS_FASTCALL (true/false) solely to switch behavior. or better: how about we eradicate AVMPLUS_FASTCALL too, use FASTCALL exclusively, and only support fastcall on x86. All the compilers we use (or likely will use, thinking about intel cc) support fastcall. (nit) why s/sp++;/++sp;/ when it's a standalone statement anyway? should boxtype use choose? (maybe out of scope for this bug) requiring the index to GETLOCAL/SETLOCAL to be boxed seems like unnecessary overhead. is it just to appease fc.py's type checker? also, fwiw, the is a U30 parameter (unsigned). (nit) Change in Assembler.h is whitespace only and still looks odd. tabs can/should be expanded to spaces. ditto for Lir.h
Assignee | ||
Comment 16•17 years ago
|
||
re: MMGC_DRC, oops, that was probably a merge collision. I'll remove and get Tommy to recheck for other such mistakes. re: [] vs (), it's fine with me either way. I had thought () was Intel standard but if [] is easier to grep for I'll switch everything to that. I just didn't want to have mixed form. re: fastcall vs cdecl, sounds fine. I'll rework and resubmit. re: sp++ vs ++sp, there is no point, it's just one of those things that got changed along the way and not unchanged... I think it's a holdover from the early C++ days when some compilers were stupid about postincrement and I got in the habit of preincrement. re: boxtype using choose... not sure. probably for another bug. re: index to GETLOCAL/SETLOCAL being boxed, there is no such requirement. Are you talking about OP_getlocal/OP_setlocal? re: whitespace... is there a way to tell Xcode and/or HG to auto-expand everything to spaces?
Comment 17•17 years ago
|
||
(In reply to comment #16) > re: [] vs (), it's fine with me either way. I had thought () was Intel standard > but if [] is easier to grep for I'll switch everything to that. I just didn't > want to have mixed form. I dont think either is easier to grep for since [ and ( would both need escaping, however when i just find [ easier to read. msft's disassembler (at least) uses [. Code snippets in the IA32 documentation use [, although there, the syntax is a bit different. for example: mov ebx, DWORD PTR [esi+4] > re: index to GETLOCAL/SETLOCAL being boxed, there is no such requirement. Are > you talking about OP_getlocal/OP_setlocal? yes, they have ibox, I assumed it was now a requirement of GETLOCAL because of that. anyway, ideally no boxing is required. IMM returns u, and GET/SETLOCAL expect i, maybe that's the problem? if so we should make GET/SETLOCAL take u instead of i. after all we dont need/support negative local variable indexes. > re: whitespace... is there a way to tell Xcode and/or HG to auto-expand > everything to spaces? i use hg diff|expand, but its a nit so not a biggie. apparently a mozilla coding convention is spaces, no hard tabs except in files that require it (eg Makefile's). not worth a holding up this change for, but we should be biased in that direction if we're fixing whitespace.
Assignee | ||
Comment 18•17 years ago
|
||
re: [], am changing to that and will resubmit momentarily. re: GETLOCAL and SETLOCAL shouldn't require ibox, it should be able to be removed with no effect... not sure how it crept in in the first place. but I'd prefer to do that in a future changelist. re: expand, didn't know about that. I'll use that for future diffs.
Comment 19•17 years ago
|
||
MMGC_DRC is always on and I'm gonna remove it. MMgc/SPAM differences will be covered by a MMGC ifdef going forward.
Assignee | ||
Comment 20•17 years ago
|
||
Consolidation of multiple patches to fix the Linux build. Linux now builds & passes sanity tests. -- fc.py didn't verify that a superword's declared in/out args matched the implementation, which could cause the flush-to-stack code to be inconsistent between Interpreter and Tracing modes. Now we verify that that the number and type of in/out args match. Also, calling functions that returned a Box type wasn't quite right in a way that made for compiler-dependent code; now we consistent use Box::RetType. Also corrected the "-superwords" option to actually work (to allow for disabling superwords). -- various makefile, configure, manifest etc issues related to Linux. -- Removed AVMPLUS_CDECL and AVMPLUS_FASTCALL entirely; now i386 always uses FASTCALL. Getting these set incorrectly caused really hard-to-debug errors in tracing mode, and every Intel compiler we are likely to ever use will support FASTCALL mode. -- moved core/AvmDebugWin.cpp to platform/win32/AvmDebugWin.cpp -- rewrote nextnamens in Forth instead of using native function -- rewrote hasnext2 (Forth) using CASE to minimize redundant conditionals -- fixed incorrect superword in/out superword arg declaration for OP_setlocal -- fixed a few build errors in MMgc related to building ARM using RVCT compiler that crept into a previous merge -- corrected inconsistent verbose assembly output for i386 tracer; we sometimes used [%s] and sometimes (%s) for memory accesses. Now use [%s] consistently (makes reading/searching verbose output a little simpler)
Attachment #296903 -
Attachment is obsolete: true
Attachment #297006 -
Flags: review?(edwsmith)
Attachment #296903 -
Flags: review?(treilly)
Attachment #296903 -
Flags: review?(edwsmith)
Updated•17 years ago
|
Attachment #297006 -
Flags: review?(edwsmith) → review+
Assignee | ||
Comment 21•17 years ago
|
||
pushed as changeset 277 0bed8f45bb0a
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 22•15 years ago
|
||
Closing out all TT: transfer bugs that are resolved fixed.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•