Closed Bug 468484 Opened 13 years ago Closed 12 years ago

TM: merge from redux

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: graydon, Assigned: graydon)

References

Details

Attachments

(16 files)

4.47 KB, patch
gal
: review+
Details | Diff | Splinter Review
7.49 KB, patch
gal
: review+
Details | Diff | Splinter Review
10.47 KB, patch
gal
: review+
Details | Diff | Splinter Review
108.88 KB, patch
gal
: review+
Details | Diff | Splinter Review
25.96 KB, patch
gal
: review+
Details | Diff | Splinter Review
6.82 KB, patch
gal
: review+
Details | Diff | Splinter Review
769 bytes, patch
gal
: review+
Details | Diff | Splinter Review
9.48 KB, patch
dvander
: review+
Details | Diff | Splinter Review
1.11 KB, patch
dvander
: review+
Details | Diff | Splinter Review
571 bytes, patch
gal
: review+
Details | Diff | Splinter Review
2.30 KB, patch
dvander
: review+
Details | Diff | Splinter Review
43.77 KB, patch
dvander
: review+
Details | Diff | Splinter Review
641 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
842 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
6.75 KB, patch
gal
: review+
Details | Diff | Splinter Review
10.95 KB, patch
gal
: review+
Details | Diff | Splinter Review
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).
Summary: merge from redux → TM: merge from redux
Assignee: general → graydon
Attached patch mergeSplinter Review
Attachment #351942 - Flags: review?(danderson)
Attachment #351943 - Flags: review?(gal) → review+
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.
Attachment #351944 - Flags: review?(gal) → review+
Attachment #351945 - Flags: review?(gal) → review+
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.
Attachment #351938 - Flags: review?(danderson) → review+
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 on attachment 351947 [details] [diff] [review]
fix boundary bug injected by CallInfo change

Wow. How did we not run into this?
Attachment #351947 - Flags: review?(gal) → review+
Comment on attachment 351942 [details] [diff] [review]
merge

Looks trivial too.
Attachment #351942 - Flags: review?(danderson) → review+
Comment on attachment 351949 [details] [diff] [review]
Fixed bug causing too much spilling, other arm tweaks

This is david land.
Attachment #351949 - Flags: review?(gal) → review?(danderson)
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.
Attachment #351950 - Flags: review?(gal) → review?(danderson)
Attachment #351951 - Flags: review?(gal) → review+
Attachment #351952 - Flags: review?(gal) → review?(danderson)
Comment on attachment 351952 [details] [diff] [review]
trivial cleanups to simplify armjit merge (r=me)

This doesn't look like trivial cleanup to me.
Attachment #351949 - Flags: review?(danderson) → review+
Attachment #351950 - Flags: review?(danderson) → review+
Attachment #351952 - Flags: review?(danderson) → review+
(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.
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 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.
Attachment #351940 - Flags: review?(danderson) → review+
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.
Attachment #352224 - Flags: review?(danderson)
Tiny, only one fix remained in this patch that didn't get absorbed back in earlier sloshing.
Attachment #352228 - Flags: review?(danderson)
Curious: reviewed and made into tamarin, but not yet us? Hmm.
Attachment #352229 - Flags: review?(danderson)
Attachment #352224 - Flags: review?(danderson) → review+
Attachment #352228 - Flags: review?(danderson) → review+
Attachment #352229 - Flags: review?(danderson) → review+
Attachment #352260 - Flags: review?(gal) → review+
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
Attachment #352378 - Flags: review?(gal) → review+
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.
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).
Flags: blocking1.9.1+
This bug is getting difficult to track. Can remaining work be split in to smaller chunks in separate bugs?
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)
(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.
Flags: blocking1.9.1+
Depends on: 480822
Depends on: 497455
No longer depends on: 480822
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.
Oh, goodness yes, these have all rotted beyond any use anyway.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.