TT: fix compile errors with gcc in tamarin-tracing

VERIFIED FIXED

Status

Tamarin
Build Config
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Zbynek Winkler, Assigned: Steven Johnson)

Tracking

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
Created attachment 295853 [details] [diff] [review]
Fix compile errors with gcc on linux.

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

11 years ago
Created attachment 295971 [details] [diff] [review]
Fix GCDebugUnix.cpp.

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

11 years ago
Cool. Looks fundamentally fine. I'll do my best to get this pushed into HG this week.

Comment 4

11 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

11 years ago
Created attachment 295980 [details] [diff] [review]
Fix AvmDebugUnix.cpp.

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

11 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

11 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

11 years ago
Created attachment 296204 [details] [diff] [review]
Define AVMPLUS_FASTCALL on AVMPLUS_LINUX.

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

11 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

11 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

11 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

11 years ago
Attachment #295853 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #295971 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #295980 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #296204 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Created attachment 296903 [details] [diff] [review]
Patch

Patch excludes machine-generated files using 

hg diff --exclude=core/vm_* --exclude=shell/shell_* --exclude=core/builtin_*
Assignee: nobody → stejohns
Status: NEW → ASSIGNED
Attachment #296903 - Flags: review?(edwsmith)
(Assignee)

Comment 13

11 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

11 years ago
Attachment #296903 - Flags: review?(treilly)
(Assignee)

Updated

11 years ago
Duplicate of this bug: 409024

Comment 15

11 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

11 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

11 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

11 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

11 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

11 years ago
Created attachment 297006 [details] [diff] [review]
Patch

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

11 years ago
Attachment #297006 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 21

11 years ago
pushed as changeset 277	0bed8f45bb0a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 22

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