468484 - TM: merge from redux

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

This is a tracking bug for review of tracemonkey patches that are ports of patches found in tamarin-redux (either directly or via the intermediate nanojit-specific repositories).

Comment 1

[Graydon Hoare :graydon]

Attachment: Remove some of the difference between last post-merge tamarin-redux and tracemonkey states

Comment 2

[Graydon Hoare :graydon]

Attachment: This patch adds a macro that wraps operator delete in a little MMgc-specific mojo to call GC::Free directly if we are collecting

Comment 3

[Graydon Hoare :graydon]

Attachment: merge

Comment 4

[Graydon Hoare :graydon]

Attachment: make asm_output[123] varadic, and add some LIR instruction comments

Comment 5

[Graydon Hoare :graydon]

Attachment: Re-insert asm-counting code lost in previous redux-tracemonkey merge

Comment 6

[Graydon Hoare :graydon]

Attachment: redid nMarkExecute() to fix the bug. Now it always does exactly one page

Comment 7

[Andreas Gal :gal]

Comment on attachment 351944 [details] [diff] [review]
 Re-insert asm-counting code lost in previous redux-tracemonkey merge

r=me but please check with ed whether we actually want this code or whether we should do the reverse and remove it from TC.

Comment 8

[Andreas Gal :gal]

Comment on attachment 351938 [details] [diff] [review]
Remove some of the difference between last post-merge tamarin-redux and tracemonkey states

Looks trivial. I think I can + it for david.

Comment 9

[Graydon Hoare :graydon]

Attachment: fix boundary bug injected by CallInfo change

Comment 10

[Andreas Gal :gal]

Comment on attachment 351940 [details] [diff] [review]
This patch adds a macro that wraps operator delete in a little MMgc-specific mojo to call GC::Free directly if we are collecting

David, please take a look. I remember we had a ton of trouble with new/delete and valgrind.

Comment 11

[Graydon Hoare :graydon]

Attachment: Fixed bug causing too much spilling, other arm tweaks

Comment 12

[Andreas Gal :gal]

Comment on attachment 351947 [details] [diff] [review]
fix boundary bug injected by CallInfo change

Wow. How did we not run into this?

Comment 13

[Graydon Hoare :graydon]

Attachment: Fix cascading register spilling bug 462522 (r=rreitmai+)

Comment 14

[Andreas Gal :gal]

Comment on attachment 351942 [details] [diff] [review]
merge

Looks trivial too.

Comment 15

[Graydon Hoare :graydon]

Attachment: Bug 463182 - Can not build tamarin-redux on solaris with Sun compiler. stejohns: review+

Comment 16

[Andreas Gal :gal]

Comment on attachment 351949 [details] [diff] [review]
Fixed bug causing too much spilling, other arm tweaks

This is david land.

Comment 17

[Graydon Hoare :graydon]

Attachment: trivial cleanups to simplify armjit merge (r=me)

Comment 18

[Andreas Gal :gal]

Comment on attachment 351950 [details] [diff] [review]
Fix cascading register spilling bug 462522 (r=rreitmai+)

Looks good. We talked about this bug but david is the right reviewer for this.

Comment 19

[Andreas Gal :gal]

Comment on attachment 351952 [details] [diff] [review]
trivial cleanups to simplify armjit merge (r=me)

This doesn't look like trivial cleanup to me.

Comment 20

[Graydon Hoare :graydon]

(In reply to comment #19)
> (From update of attachment 351952 [details] [diff] [review])
> This doesn't look like trivial cleanup to me.

No, the comment isn't great; it obviously has some logical adjustments in it too. Ask edwin? I'm just copying the patches+comments directly from redux, not re-titling them myself.

Comment 21

[Graydon Hoare :graydon]

FYI: there are more patches coming in this series; what's listed here so far is only what applies *and works* at this point in my patch queue. They're also bottom-to-top in order of application, so they'll all be waiting on the first review.

Comment 22

[Andreas Gal :gal]

Comment on attachment 351940 [details] [diff] [review]
This patch adds a macro that wraps operator delete in a little MMgc-specific mojo to call GC::Free directly if we are collecting

I didn't realize this was coming from adobe.

Comment 23

[Graydon Hoare :graydon]

Attachment: LirBuffer has been modified to provide advance warning of out of memory (OOM) conditions.

This one is a little scary, because I had to eyeball and manually reduce a large amount of divergence in how we manage pages to get it working; the motivating patches for all the steps involved appear a bit scattered over the past.

I *think* this simplifies our page handling in the fragmento a fair bit. But review would be good. It builds here, and passes the trace tests, and does not appear to regress in terms of performance.

Comment 24

[Graydon Hoare :graydon]

Attachment: Fixed a couple more small nanojit injections

Tiny, only one fix remained in this patch that didn't get absorbed back in earlier sloshing.

Comment 25

[Graydon Hoare :graydon]

Attachment: isQuad(LIR_callh) on 32bit cpu's should be false (r=dvander+)

Curious: reviewed and made into tamarin, but not yet us? Hmm.

Comment 26

[Graydon Hoare :graydon]

Attachment: Trivial, rename avmplus::AvmConfiguration, adjust sites of use to match tamarin.

Comment 27

[Graydon Hoare :graydon]

Attachment: whitespace, typenames, and similar divergence-reducing changes

Comment 28

[Andreas Gal :gal]

Comment on attachment 352378 [details] [diff] [review]
whitespace, typenames, and similar divergence-reducing changes

>diff -r 8b5a5c74be20 js/src/nanojit/Assembler.cpp
>--- a/js/src/nanojit/Assembler.cpp	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/Assembler.cpp	Wed Dec 10 12:05:18 2008 -0800
>@@ -70,7 +70,7 @@
> 	        return ins->resv() == 0;
> 	    }
> 
>-	public:		
>+	public:
> 		DeadCodeFilter(LirFilter *in, const CallInfo *f) : LirFilter(in), functions(f) {}
> 		LInsp read() {
> 			for (;;) {
>@@ -145,7 +145,7 @@
> 	 */
>     Assembler::Assembler(Fragmento* frago)
>         : hasLoop(0)
>-		, _frago(frago)
>+        , _frago(frago)
>         , _gc(frago->core()->gc)
>         , _labels(_gc)
>         , _patches(_gc)
>@@ -257,12 +257,12 @@
> 			setError(ResvFull); 
> 			item = 1;
> 		}
>-		Reservation *r = &_resvTable[item];
>+        Reservation *r = &_resvTable[item];
> 		_resvFree = r->arIndex;
> 		r->reg = UnknownReg;
> 		r->arIndex = 0;
>-		r->used = 1;
>-		i->setresv(item);
>+        r->used = 1;
>+        i->setresv(item);
> 		return r;
> 	}
> 
>@@ -925,7 +925,7 @@
> 		NanoAssertMsgf(error() || _fpuStkDepth == 0,"_fpuStkDepth %d",_fpuStkDepth);
> 
> 		internalReset();  // clear the reservation tables and regalloc
>-		NanoAssert(!_branchStateMap || _branchStateMap->isEmpty());
>+		NanoAssert( !_branchStateMap || _branchStateMap->isEmpty());

Is this intentional?

> 		_branchStateMap = 0;
> 
> #ifdef AVMPLUS_ARM
>@@ -1319,7 +1319,6 @@
>                         NanoAssert(label->addr == 0 && label->regs.isValid());
>                         //evictRegs(~_allocator.free);
>                         intersectRegisterState(label->regs);
>-                        //asm_align_code();
>                         label->addr = _nIns;
>                     }
> 					verbose_only( if (_verbose) { outputAddr=true; asm_output("[%s]", _thisfrag->lirbuf->names->formatRef(ins)); } )
>diff -r 8b5a5c74be20 js/src/nanojit/Assembler.h
>--- a/js/src/nanojit/Assembler.h	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/Assembler.h	Wed Dec 10 12:05:19 2008 -0800
>@@ -331,7 +331,6 @@
> 			};
> 			
> 			// platform specific implementation (see NativeXXX.cpp file)
>-			void		nInit(uint32_t flags);
> 			void		nInit(AvmCore *);
> 			Register	nRegisterAllocFromSet(int32_t set);
> 			void		nRegisterResetAll(RegAlloc& a);
>diff -r 8b5a5c74be20 js/src/nanojit/LIR.cpp
>--- a/js/src/nanojit/LIR.cpp	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/LIR.cpp	Wed Dec 10 12:05:19 2008 -0800
>@@ -95,7 +95,7 @@
> 		if (start)
> 			_unused = &start->lir[0];
> 		//buffer_count++;
>-		//fprintf(stderr, "LirBuffer %x start %x\n", (int)this, (int)_start);
>+		//fprintf(stderr, "LirBuffer %x unused %x\n", (int)this, (int)_unused);
> 	}
> 
> 	LirBuffer::~LirBuffer()
>@@ -126,6 +126,7 @@
> 		// doesn't include embedded constants nor LIR_skip payload
> 		return _stats.lir;
> 	}
>+
> 	int32_t LirBuffer::byteCount() 
> 	{
> 		return ((_pages.size() ? _pages.size()-1 : 0) * sizeof(Page)) +
>@@ -138,9 +139,7 @@
> 		if (page)
> 			_pages.add(page);
> 		else
>-		{
> 			_noMem = 1;
>-		}
> 		return page;
> 	}
> 	
>@@ -1093,8 +1092,8 @@
> 			op = LIR_callh;
> 		LInsp args2[MAXARGS*2]; // arm could require 2 args per double
> 		int32_t j = 0;
>-		int32_t i = 0;
>-		while (j < argc) {
>+        int32_t i = 0;
>+        while (j < argc) {
> 			argt >>= 2;
> 			ArgSize a = ArgSize(argt&3);
> 			if (a == ARGSIZE_F) {
>@@ -1249,7 +1248,7 @@
> #ifdef MEMORY_INFO
> //		m_list.set_meminfo_name("LInsHashSet.list");
> #endif
>-        LInsp *list = (LInsp*) gc->Alloc(sizeof(LInsp)*m_cap);
>+        LInsp *list = (LInsp*) gc->Alloc(sizeof(LInsp)*m_cap, GC::kZero);
>         WB(gc, this, &m_list, list);
> 	}
> 
>@@ -1343,7 +1342,7 @@
> 	void FASTCALL LInsHashSet::grow()
> 	{
> 		const uint32_t newcap = m_cap << 1;
>-        LInsp *newlist = (LInsp*) m_gc->Alloc(newcap * sizeof(LInsp));
>+        LInsp *newlist = (LInsp*) m_gc->Alloc(newcap * sizeof(LInsp), GC::kZero);
>         LInsp *list = m_list;
> #ifdef MEMORY_INFO
> //		newlist.set_meminfo_name("LInsHashSet.list");
>@@ -1550,7 +1549,7 @@
>         }
> 		void add(LInsp i, LInsp use) {
>             if (!i->isconst() && !i->isconstq() && !live.containsKey(i)) {
>-                NanoAssert(unsigned(i->opcode()) < sizeof(lirNames) / sizeof(lirNames[0]));
>+                NanoAssert(size_t(i->opcode()) < sizeof(lirNames) / sizeof(lirNames[0]));
>                 live.put(i,use);
>             }
> 		}
>@@ -1583,7 +1582,7 @@
>         LirReader br(lirbuf);
> 		StackFilter sf(&br, gc, lirbuf, lirbuf->sp);
> 		StackFilter r(&sf, gc, lirbuf, lirbuf->rp);
>-		int total = 0;
>+        int total = 0;
>         if (lirbuf->state)
>             live.add(lirbuf->state, r.pos());
> 		for (LInsp i = r.read(); i != 0; i = r.read())
>@@ -1602,7 +1601,7 @@
> 			if (live.contains(i))
> 			{
> 				live.retire(i,gc);
>-                NanoAssert(unsigned(i->opcode()) < sizeof(operandCount) / sizeof(operandCount[0]));
>+                NanoAssert(size_t(i->opcode()) < sizeof(operandCount) / sizeof(operandCount[0]));
> 				if (i->isStore()) {
> 					live.add(i->oprnd2(),i); // base
> 					live.add(i->oprnd1(),i); // val
>@@ -1742,7 +1741,7 @@
> 				}
> #endif
> 			} else {
>-                NanoAssert(unsigned(ref->opcode()) < sizeof(lirNames) / sizeof(lirNames[0]));
>+                NanoAssert(size_t(ref->opcode()) < sizeof(lirNames) / sizeof(lirNames[0]));
> 				copyName(ref, lirNames[ref->opcode()], lircounts.add(ref->opcode()));
> 			}
> 			StringNullTerminatedUTF8 cname(gc, names.get(ref)->name);
>@@ -2165,11 +2164,11 @@
>         return out->insStorei(v, b, d);
>     }
> 
>-    LInsp LoadFilter::insCall(const CallInfo *call, LInsp args[])
>+    LInsp LoadFilter::insCall(const CallInfo *ci, LInsp args[])
>     {
>-        if (!call->_cse)
>+        if (!ci->_cse)
>             exprs.clear();
>-        return out->insCall(call, args);
>+        return out->insCall(ci, args);
>     }
> 
>     LInsp LoadFilter::ins0(LOpcode op)
>diff -r 8b5a5c74be20 js/src/nanojit/LIR.h
>--- a/js/src/nanojit/LIR.h	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/LIR.h	Wed Dec 10 12:05:19 2008 -0800
>@@ -235,7 +235,7 @@
> 		 *      
> 		 *    For pointing to instructions further than this range LIR_tramp is used.
> 		 */
>-		union 
>+		union
> 		{
> 			u_type u;
> 			c_type c;
>@@ -246,12 +246,7 @@
> 		};
> 
> 		enum {
>-			callInfoWords =
>-#ifdef NANOJIT_64BIT
>-			    2
>-#else
>-			    1
>-#endif
>+			callInfoWords = sizeof(LIns*)/sizeof(u_type)
> 		};
> 
> 		uint32_t reference(LIns*) const;
>@@ -500,7 +495,7 @@
> 	class LirNameMap MMGC_SUBCLASS_DECL
> 	{
> 		template <class Key>
>-		class CountMap : public avmplus::SortedMap<Key, int, avmplus::LIST_NonGCObjects> {
>+		class CountMap: public avmplus::SortedMap<Key, int, avmplus::LIST_NonGCObjects> {
> 		public:
> 			CountMap(avmplus::GC*gc) : avmplus::SortedMap<Key, int, avmplus::LIST_NonGCObjects>(gc) {}
> 			int add(Key k) {
>@@ -553,12 +548,12 @@
> 		DWB(LirNameMap*) names;
>     public:
> 		VerboseWriter(avmplus::GC *gc, LirWriter *out, LirNameMap* names) 
>-			: LirWriter(out), code(gc), names(names) 
>+			: LirWriter(out), code(gc), names(names)
> 		{}
> 
> 		LInsp add(LInsp i) {
>-			if (i)
>-				code.add(i);
>+            if (i)
>+                code.add(i);

Indentation is all over the place in this patch. Its almost worse than before. But it was crazy to begin with so I don't mind matching TT for merging purposes.

> 			return i;
> 		}
> 
>@@ -588,7 +583,6 @@
> 			return add_flush(out->insBranch(v, condition, to));
> 		}
> 
>-
> 		LIns* ins0(LOpcode v) {
>             if (v == LIR_label || v == LIR_start) {
>                 flush();
>@@ -597,7 +591,7 @@
> 		}
> 
> 		LIns* ins1(LOpcode v, LInsp a) {
>-			return isRet(v) ? add_flush(out->ins1(v, a)) : add(out->ins1(v, a));
>+            return isRet(v) ? add_flush(out->ins1(v, a)) : add(out->ins1(v, a));
> 		}
> 		LIns* ins2(LOpcode v, LInsp a, LInsp b) {
> 			return v == LIR_2 ? out->ins2(v,a,b) : add(out->ins2(v, a, b));
>@@ -714,7 +708,6 @@
> 			struct 
> 			{
> 				uint32_t lir;	// # instructions
>-				uint32_t pages;	// pages consumed
> 			}
> 			_stats;
> 
>@@ -760,7 +753,7 @@
> 		    LInsp	insCall(const CallInfo *call, LInsp args[]);
> 			LInsp	insGuard(LOpcode op, LInsp cond, LIns *x);
> 			LInsp	insBranch(LOpcode v, LInsp condition, LInsp to);
>-			LInsp   insAlloc(int32_t size);
>+            LInsp   insAlloc(int32_t size);
> 
> 			// buffer mgmt
> 			LInsp	skip(size_t);
>@@ -783,7 +776,7 @@
> 	public:
> 		LirFilter *in;
> 		LirFilter(LirFilter *in) : in(in) {}
>-		virtual ~LirFilter() {}
>+        virtual ~LirFilter(){}
> 
> 		virtual LInsp read() {
> 			return in->read();
>diff -r 8b5a5c74be20 js/src/nanojit/Native.h
>--- a/js/src/nanojit/Native.h	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/Native.h	Wed Dec 10 12:05:19 2008 -0800
>@@ -100,7 +100,7 @@
> 			if (verbose_enabled()) {\
> 				outline[0]='\0';\
> 				if (outputAddr) sprintf(outline, "  %10p  ",_nIns);\
>-				else sprintf(outline, "            ");\
>+				else sprintf(outline, "              ");\
> 				sprintf(&outline[14], ##__VA_ARGS__);\
> 				Assembler::outputAlign(outline, 45);\
> 				RegAlloc::formatRegisters(_allocator, outline, _thisfrag);\
>diff -r 8b5a5c74be20 js/src/nanojit/NativeARM.cpp
>--- a/js/src/nanojit/NativeARM.cpp	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/NativeARM.cpp	Wed Dec 10 12:05:19 2008 -0800
>@@ -1108,11 +1108,11 @@
> }
> 
> NIns*
>-Assembler::asm_branch(bool branchOnFalse, LInsp cond, NIns* targ, bool /*far*/)
>+Assembler::asm_branch(bool branchOnFalse, LInsp cond, NIns* targ, bool far)
> {
>     // ignore far -- we figure this out on our own.
>     // XXX noone actually uses the far param in nj anyway... (always false)
>-    
>+    (void)far;
> 
>     NIns* at = 0;
>     LOpcode condop = cond->opcode();
>diff -r 8b5a5c74be20 js/src/nanojit/Nativei386.cpp
>--- a/js/src/nanojit/Nativei386.cpp	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/Nativei386.cpp	Wed Dec 10 12:05:19 2008 -0800
>@@ -93,6 +93,7 @@
> 
> 	void Assembler::nInit(AvmCore* core)
> 	{
>+		(void) core;
>         OSDep::getDate();
> 	}
> 
>diff -r 8b5a5c74be20 js/src/nanojit/RegAlloc.cpp
>--- a/js/src/nanojit/RegAlloc.cpp	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/RegAlloc.cpp	Wed Dec 10 12:05:19 2008 -0800
>@@ -121,7 +121,6 @@
>                 }
> 			}
> 		}
>-
>         NanoAssert(a != 0);
>         return a;
> 	}
>diff -r 8b5a5c74be20 js/src/nanojit/avmplus.h
>--- a/js/src/nanojit/avmplus.h	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/avmplus.h	Wed Dec 10 12:05:19 2008 -0800
>@@ -253,10 +253,24 @@
>         static GCHeap heap;
>         
>     public:
>+		/**
>+		* flags to be passed as second argument to alloc
>+		*/
>+		enum AllocFlags
>+		{
>+			kZero=1,
>+			kContainsPointers=2,
>+			kFinalize=4,
>+			kRCObject=8
>+		};
>+
>         static inline void*
>-        Alloc(uint32_t bytes)
>+        Alloc(uint32_t bytes, int flags=kZero)
>         {
>+          if (flags & kZero)
>             return calloc(1, bytes);
>+          else
>+            return malloc(bytes);
>         }
>     
>         static inline void
>diff -r 8b5a5c74be20 js/src/nanojit/nanojit.h
>--- a/js/src/nanojit/nanojit.h	Tue Dec 09 21:07:17 2008 -0800
>+++ b/js/src/nanojit/nanojit.h	Wed Dec 10 12:05:19 2008 -0800
>@@ -172,12 +172,12 @@
> 	#define verbose_output						if (verbose_enabled()) Assembler::output
> 	#define verbose_outputf						if (verbose_enabled()) Assembler::outputf
> 	#define verbose_enabled()					(_verbose)
>-	#define verbose_only(x)						x
>+	#define verbose_only(...)					__VA_ARGS__
> #else
> 	#define verbose_output
> 	#define verbose_outputf
> 	#define verbose_enabled()
>-	#define verbose_only(x)
>+	#define verbose_only(...)
> #endif /*NJ_VERBOSE*/
> 
> #ifdef _DEBUG

Comment 29

[Graydon Hoare :graydon]

Backed out a portion of the "other arm tweaks" patch above in revision cd8eefe8ad96, will try again once I'm somewhat more confident in my arm testing.

Comment 30

[Graydon Hoare :graydon]

Further backout of part of the 'whitespace and typenames' patch, in revision c0c5a3618692, due to arm-wince breakage (use of the symbol 'far' makes cl angry, right right).

Comment 31

[Robert Sayre]

This bug is getting difficult to track. Can remaining work be split in to smaller chunks in separate bugs?

Comment 32

[Graydon Hoare :graydon]

Yeah. I'm not really sure how to manage it long-term: it's an endless task (Adobe will keep generating new code) and a bug with thousands of attachments seems uncool.

Maybe a keyword for "this is a merge so consider it at least 50% already-reviewed by a tracemonkey author at a different company"?

(I also think, given that it's an endless task, that it makes little sense for it to be a blocker)

Comment 33

[Robert Sayre]

(In reply to comment #32)
> Yeah. I'm not really sure how to manage it long-term: it's an endless task
> (Adobe will keep generating new code) and a bug with thousands of attachments
> seems uncool.

Let's make specific bugs at some reasonable level of granularity, so that bug fixes can be approved for 1.9.1 etc, without much confusion. We can make a meta bug or whiteboard notation to keep track--your choice.

Comment 34

[Nicholas Nethercote [inactive]]

Graydon, can this bug be closed now?  It looks like we're using the model of "one bug report per merge", possible with a single meta-bug for tracking, at least for some periods.

Comment 35

[Graydon Hoare :graydon]

Oh, goodness yes, these have all rotted beyond any use anyway.