Merge pull request #4192 from supercollider/topic/GC-fixes-post-3.10

Topic/gc fixes post 3.10

HelpSource/Guides/WritingPrimitives.schelp

@@ -35,9 +35,9 @@ enum { // primitive errors
 	errOutOfMemory,
 	errCantCallOS,
 	errException,
-	
+
 	errPropertyNotFound = 6000,
-	
+
 	errLastError
 };
 ::
@@ -160,23 +160,35 @@ The following two examples are both safe:
 definitionlist::
 ##Make the newly created object reachable:
 ||teletype::
-PyrSlot* arg = g->sp;
+PyrSlot *arg = g->sp;
 PyrObject *array1 = newPyrArray(g->gc, 2, 0, true); // runGC = true
 SetObject(arg, array1); // make the array reachable on the stack
 PyrObject *array2 = newPyrArray(g->gc, 2, 0, true);
 ...
 ::
 ##Set teletype::runGC:: to false:
 ||teletype::
-PyrSlot* arg = g->sp;
+PyrSlot *arg = g->sp;
 // runGC = true
 PyrObject *array1 = newPyrArray(g->gc, 2, 0, true);
 // runGC = false so GC is not triggered, and array1 can't be freed
 PyrObject *array2 = newPyrArray(g->gc, 2, 0, false);
 ...
 ::
 ::
-Care must be taken when writing utility functions which themselves create new objects, since this may happen somewhat opaquely and the calling context may not be known. Functions which may call themselves recursively also need special attention. In such cases setting teletype::runGC:: to false may be the safest option, or including a teletype::runGC:: arg so that GC behaviour is explicit. teletype::MsgToInt8Array:: is one example of such a function.
+One caveat: When making a new object reachable by storing it on the stack, you must ensure that you are not overwriting objects that will be needed later in the primitive. If this is done the receiver may be collected on any future allocations. One solution is to strong::push:: the object onto the stack, and then pop it when finished, e.g.:
+teletype::
+PyrSlot *receiver = g->sp; // get the receiver
+PyrObject *result = newPyrArray(g->gc, 2, 0, true); // create the result
+++g->sp;
+SetObject(g->sp, result); // push the result array on the stack, so both it and rec are reachable
+... // further allocations which make use of the receiver to populate result
+--g->sp; // pop the stack back to the receiver slot since we stored result there above
+SetObject(receiver, result); // now set the receiver
+::
+teletype::prArrayMultiChanExpand:: in lang/LangPrimSource/PyrListPrim.cpp gives an example of this approach. Setting teletype::runGC:: to false is another possible solution.
+
+Similarly, care must be taken when writing utility functions which themselves create new objects, since this may happen somewhat opaquely and the calling context may not be known. Functions which may call themselves recursively also need special attention. In such cases setting teletype::runGC:: to false may be the safest option, or including a teletype::runGC:: arg so that GC behaviour is explicit. teletype::MsgToInt8Array:: is one example of such a function.
 
 teletype::
 static PyrInt8Array* MsgToInt8Array ( sc_msg_iter& msg, bool runGC )
@@ -201,7 +213,7 @@ The following two examples are both safe:
 definitionlist::
 ##Run GCWrite as parent may not be white:
 ||teletype::
-PyrSlot* arg = g->sp;
+PyrSlot *arg = g->sp;
 PyrObject *array = newPyrArray(g->gc, 2, 0, true); // runGC = true
 SetObject(arg, array); // make the array reachable on the stack
 PyrObject *str = newPyrString(g->gc, "Hello", 0, true); // runGC = true
@@ -227,7 +239,7 @@ If placing an object inside another has modified its size (e.g. adding an object
 definitionlist::
 ##This is safe:
 ||teletype::
-PyrSlot* arg = g->sp;
+PyrSlot *arg = g->sp;
 int size = 10;
 PyrObject *array = newPyrArray(g->gc, size, 0, true); // runGC = true
 SetObject(arg, array);
@@ -244,7 +256,7 @@ for(i=0; i<numLists; ++i) {
 ::
 ##This is emphasis::not:: safe:
 ||teletype::
-PyrSlot* arg = g->sp;
+PyrSlot *arg = g->sp;
 int size = 10;
 PyrObject *array = newPyrArray(g->gc, size, 0, true); // runGC = true
 // setting size to final value here means
@@ -267,7 +279,7 @@ To summarize, before calling any function that might allocate (like teletype::ne
 numberedlist::
 ## All objects previously created must be reachable, which means they must exist
     list::
-    ## on the teletype::g->sp:: stack
+    ## on the teletype::g->sp:: stack (taking care not to overwrite any objects which will be needed later)
     ## or, in a lang-side variable or class variable.
     ## or, in a slot of another object that fulfils these criteria.
     ::
@@ -283,7 +295,7 @@ Here's an example of how a complete primitive might look:
 teletype::
 int prMyPrimitive(struct VMGlobals* g, int numArgsPushed)
 {
-    PyrSlot* arg = g->sp;
+    PyrSlot *arg = g->sp;
     float number;
     int err;
 

lang/LangPrimSource/PyrListPrim.cpp

@@ -80,7 +80,10 @@ int prArrayMultiChanExpand(struct VMGlobals *g, int numArgsPushed)
 	}
 
 	obj2 = newPyrArray(g->gc, maxlen, 0, true);
-	SetObject(a, obj2);
+	// push obj2 onto the stack so it's not collected
+	// we can't just set in a here as we still need obj1's slots below, so we need to ensure it isn't collected
+	++g->sp; // advance the stack to avoid overwriting receiver
+	SetObject(g->sp, obj2);
 	slots2 = obj2->slots;
 	for (i=0; i<maxlen; ++i) {
 		obj3 = newPyrArray(g->gc, size, 0, true);
@@ -110,6 +113,8 @@ int prArrayMultiChanExpand(struct VMGlobals *g, int numArgsPushed)
 		}
 	}
 
+	--g->sp; // pop the stack back to the receiver slot since we stored ojb2 there above
+	SetObject(a, obj2); // now we can set the result in a
 	return errNone;
 }
 

lang/LangPrimSource/PyrPrimitive.cpp

@@ -3533,7 +3533,7 @@ static int prLanguageConfig_getLibraryPaths(struct VMGlobals * g, int numArgsPus
 
 	size_t numberOfPaths = dirVector.size();
 	PyrObject * resultArray = newPyrArray(g->gc, numberOfPaths, 0, true);
-	SetObject(result, resultArray);
+	SetObject(result, resultArray); // this is okay here as we don't use the receiver
 
 	for (size_t i = 0; i != numberOfPaths; ++i) {
 		const std::string& utf8_path = SC_Codecvt::path_to_utf8_str(dirVector[i]);

lang/LangPrimSource/PyrStringPrim.cpp

@@ -410,7 +410,8 @@ static int prString_FindRegexp(struct VMGlobals *g, int numArgsPushed)
 	int match_count = matches.size();
 
 	PyrObject *result_array = newPyrArray(g->gc, match_count, 0, true);
-	SetObject(a, result_array);
+	++g->sp; // advance the stack to avoid overwriting receiver
+	SetObject(g->sp, result_array); // push result to make reachable
 
 	if( !match_count ) return errNone;
 
@@ -430,8 +431,11 @@ static int prString_FindRegexp(struct VMGlobals *g, int numArgsPushed)
 		array->size = 2;
 		SetInt(array->slots, pos + offset);
 		SetObject(array->slots+1, matched_string);
-		g->gc->GCWrite(array, matched_string); // we know matched_string is white so we can use GCWriteNew
+		g->gc->GCWriteNew(array, matched_string); // we know matched_string is white so we can use GCWriteNew
 	};
+
+	--g->sp; // pop the stack back to the receiver slot since we stored result_array there above
+	SetObject(a, result_array); // now we can set the result in a
 
 	return errNone;
 }
@@ -483,7 +487,8 @@ static int prString_FindRegexpAt(struct VMGlobals *g, int numArgsPushed)
 	}
 
 	PyrObject *array = newPyrArray(g->gc, 2, 0, true);
-	SetObject(a, array);
+	++g->sp;
+	SetObject(g->sp, array); // push on stack to make reachable
 
 	PyrString *matched_string = newPyrStringN(g->gc, matched_len, 0, true);
 	memcpy(matched_string->s, stringBegin, (size_t) matched_len);
@@ -492,6 +497,8 @@ static int prString_FindRegexpAt(struct VMGlobals *g, int numArgsPushed)
 	SetInt(array->slots+1, matched_len);
 	SetObject(array->slots, matched_string);
 	g->gc->GCWriteNew(array, matched_string); // we know matched_string is white so we can use GCWriteNew
+	--g->sp; // pop the stack back to the receiver slot since we stored array there above
+	SetObject(a, array); // now we can set the result in a
 
 	return errNone;
 }
@@ -568,7 +575,7 @@ int prString_PathMatch(struct VMGlobals *g, int numArgsPushed)
 
 	// create array with appropriate reserved size
 	PyrObject* array = newPyrArray(g->gc, paths.size(), 0, true);
-	SetObject(a, array);
+	SetObject(a, array); // this is okay here as we don't use the receiver below
 
 	// convert paths and copy into sclang array.
 	for (int i = 0; i < paths.size(); ++i) {

lang/LangPrimSource/SC_CoreAudioPrim.cpp

@@ -96,7 +96,7 @@ int listDevices(struct VMGlobals *g, int type)
 	else num = numDevices;
 
     PyrObject* devArray = newPyrArray(g->gc, num * sizeof(PyrObject), 0, true);
-	SetObject(a, devArray);
+	SetObject(a, devArray); // this is okay here as we don't use the receiver below
 
 	int j = 0;
 	for (i=0; i<numDevices; i++)

lang/LangPrimSource/SC_CoreMIDI.cpp

@@ -431,7 +431,7 @@ int prListMIDIEndpoints(struct VMGlobals *g, int numArgsPushed)
 	int numDst = MIDIGetNumberOfDestinations();
 
 	PyrObject* idarray = newPyrArray(g->gc, 6 * sizeof(PyrObject), 0 , true);
-		SetObject(a, idarray);
+		SetObject(a, idarray); // this is okay here as we don't use the receiver
 
 	PyrObject* idarraySo = newPyrArray(g->gc, numSrc * sizeof(SInt32), 0 , true);
 		SetObject(idarray->slots+idarray->size++, idarraySo);

lang/LangPrimSource/SC_HID_api.cpp

@@ -492,7 +492,7 @@ int prHID_API_BuildDeviceList(VMGlobals* g, int numArgsPushed){
 	int result = SC_HID_APIManager::instance().build_devicelist();
 	if ( result > 0 ){
 		PyrObject* allDevsArray = newPyrArray(g->gc, result * sizeof(PyrObject), 0 , true);
-		SetObject( self, allDevsArray );
+		SetObject( self, allDevsArray ); // this is okay here as we don't use the receiver
 
 		struct hid_device_info *cur_dev = SC_HID_APIManager::instance().devinfos;
 		while( cur_dev ){

lang/LangSource/SC_TerminalClient.cpp

@@ -713,7 +713,7 @@ int SC_TerminalClient::prArgv(struct VMGlobals* g, int)
 	PyrSlot* argvSlot = g->sp;
 
 	PyrObject* argvObj = newPyrArray(g->gc, argc * sizeof(PyrObject), 0, true);
-	SetObject(argvSlot, argvObj);
+	SetObject(argvSlot, argvObj); // this is okay here as we don't use the receiver
 
 	for (int i=0; i < argc; i++) {
 		PyrString* str = newPyrString(g->gc, argv[i], 0, true);

testsuite/classlibrary/TestArray.sc

@@ -162,6 +162,30 @@ TestArray : UnitTest {
 
 	} // End test_arraystats
 
+	// FIXME/TODO Consider whether this test should be moved into a separate file of GC tests (or superceded by one)
+	test_flop_noGC_regression {
+		var a, b, arraySize;
+		arraySize = 32; // this causes flop to fail
+
+		1000.do { |i|
+			var indices;
+			indices = (0..arraySize);
+
+			// run identical processing on idx:
+			a = [indices].flop.collect {|lm| lm[0] }; // << .lm called in the loop
+			b = [indices].flop.collect {|lm| lm[0] }; // << .lm called in the loop
+
+			// are results equal? >> NO
+			if(a!=b, {
+				this.assertEquals(a, b, "flop: identical flops should match", onFailure: {
+					format("\nWARNING: mismatch on iter: %", i).postln;
+					a.join(" ").postln;
+					b.join(" ").postln;
+				});
+			});
+		}
+	}
+
 } // End class
 
 TestArrayLace : UnitTest {
@@ -333,4 +357,4 @@ TestArrayLace : UnitTest {
 	}
 
 
-} // End class
+} // End class