I have this ready to test, but I need a reproducer. Checked muellmusik/Utopia#16 but I don't see something that's easily testable.

Even without having tested it in action, though, I think this handling method is definitely preferable. It conforms to the standard way of handling exceptions in SC and incurs no loss of information since primitive failures would simply print an extra "Failed" before.

A simple one is turn off all wifi and networking and then try a NetAddr:sendMsg, e.g.

NetAddr("147.0.0.200", 51720).sendMsg(\foo) // send_to: Network is unreachable

try { NetAddr("147.0.0.200", 51720).sendMsg(\foo) } {} // this shows the catch works

Thanks! Works as advertised.

Glad it makes sense. I had wondered if there was a better place to store the info than in file local vars, but SC has a variety of practices for this from different eras, and this seemed at least consistent with that part of the code base.

Having to think through what to do in the catch-all I was reminded of this: http://yosefk.com/c++fqa/exceptions.html#fqa-17.6

I had wondered if there was a better place to store the info than in file local vars, but SC has a variety of practices for this from different eras, and this seemed at least consistent with that part of the code base.

Yeah, true.. I would prefer to see the three new static variables you added as a struct, possibly in a separate header, with member (or even nonmember) functions to abstract away a block like

lastPrimitiveExceptionClass = slotRawSymbol(&slotRawClass(&meth->ownerclass)->name)->name;
lastPrimitiveExceptionMethod = slotRawSymbol(&meth->name)->name;
lastPrimitiveException = std::current_exception();

I know that's being very finnicky, but for christ's sake, the file is already >4000 loc.

How would you feel about changing the type of str in prPrimitiveErrorString to std::string? It would make memory management much easier there—I just noticed that as you've written it, str will become garbage at line 338 when result destructs.

} else {
	std::string result = std::string("caught unknown exception in primitive in method ") + lastPrimitiveExceptionClass + ":" + lastPrimitiveExceptionMethod;
	str = result.c_str();
	lastPrimitiveExceptionClass = nullptr;
	lastPrimitiveExceptionMethod = nullptr;
}

Ah yes, nice catch!

Yes, sure that makes sense.

Yeah, true.. I would prefer to see the three new static variables you added as a struct, possibly in a separate header, with member (or even nonmember) functions to abstract away a block like

Actually I wondered if they shouldn't be in VMGlobals. That would be necessary if we ever wanted multiple VMs I think?

Actually I wondered if they shouldn't be in VMGlobals. That would be necessary if we ever wanted multiple VMs I think?

Oh, very good point actually. The idea is to have a similar API to windows GetLastError / C errno, right?

Also, perhaps it's better not to clear the values once they've been read; this would ensure that the variables always do point to the "last error". Not a big deal now since they're only being read in one place, but it could be helpful down the line if usage expands.

The idea is to have a similar API to windows GetLastError / C errno, right?

Yes, something like that. Via VMGlobals the last primitiveError and primitiveIndex are stored in the active Thread. There's an enum in PyrErrors.h. For exceptions we could check for particular issues and add additional error types (I thought about doing that for these network ones), but we'd still need errException as a catch-all. So I think we do need to store some more info if we want try/catch to work, and looking at it I think g->thread is the place to do it.

Fine with not clearing them. I can prepare another version with these changes.

I do worry slightly that the use of VMGlobals to hold state specific to a VM is not consistently respected, and that if we did want multiple VMs, there'd be a lot of cleanup to do, but that's another issue...

@brianlheim working on this, but slightly baffled and probably just doing something really dumb. Tried to change it to stash in the thread, so modified PyrThread to look like this:

struct PyrThread : public PyrObjectHdr
{
	PyrSlot state, func, stack, method, block, frame, ip, sp;
	PyrSlot numpop, receiver, numArgsPushed;
	PyrSlot parent, terminalValue;
	PyrSlot primitiveError;
	PyrSlot primitiveIndex;
	PyrSlot randData;
	PyrSlot beats, seconds, clock, nextBeat, endBeat, endValue;
	PyrSlot environment;
	PyrSlot exceptionHandler;
	PyrSlot threadPlayer;
	PyrSlot executingPath;
	PyrSlot oldExecutingPath;
	PyrSlot stackSize;
	std::exception_ptr lastPrimitiveException; // here's the exception
	PyrMethod *primitiveMeth;
};

When I get to this line in doPrimitive I get an EXC_BAD_ACCESS:

g->thread->lastPrimitiveException = std::current_exception();

Anything boneheaded and obvious?

@muellmusik hm, let me step through it. nothing immediately apparent

The actual crash seems to happen in #0 0x00007fff830e0bb9 in __cxa_decrement_exception_refcount ()

But surely it should be increasing the refcount?

A little more detail:

#0	0x00007fff830e0bb9 in __cxa_decrement_exception_refcount ()
#1	0x00007fff84c547f2 in std::exception_ptr::operator=(std::exception_ptr const&) ()
#2	0x00000001011fa6c8 in doPrimitive(VMGlobals*, PyrMethod*, int) at /code/git_SC/SuperCollider/lang/LangPrimSource/PyrPrimitive.cpp:3838
...

The PyrThread should be valid as other stuff before and after seems to access it without any problem.

Hmm... I'm not liking where this is going. I don't think PyrThread is ever constructed on its own, but always by converting from PyrSlot. Take a look here:

https://github.com/supercollider/supercollider/blob/master/lang/LangPrimSource/PyrPrimitive.cpp#L2977-L3022

At least, I stuck a breakpoint on a constructor for PyrThread and it never got triggered. So your new data member is always initially garbage, which means you can't call an assignment operator on it. I guess a workaround would be to use an actual pointer, but I'm not sure how you would manage that with object lifetimes. You couldn't do a pointer to an exception_ptr or to a std::exception, because those would run out of reference counts at the end of the local scope and destruct. Maybe it would be OK to put it in the VM global struct? I know that's not elegant, but it is at least a little better than an actually global static field. I'm hesitant about rewriting any of the code I linked to, because of potential memory leaks...

The reason it's decrementing the refcount is because it's trying to clear its own memory before being copy-assigned to a new value. Clearing its internal state would mean that there would be one less reference to the previously held exception pointer.

I don't think PyrThread is ever constructed on its own, but always by converting from PyrSlot

Ah right. Because GC, etc. I should have known it would be something like this.

Maybe it would be OK to put it in the VM global struct?

Well the only issue I guess would be if the thread changed, but I think that shouldn't happen as you'd have to yield during error handling, which seems like a bad idea anyway.

Well the only issue I guess would be if the thread changed, but I think that shouldn't happen as you'd have to yield during error handling, which seems like a bad idea anyway.

I agree that it should be maintained on a per-thread basis—that would match the behavior of GetLastError too.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms679360(v=vs.85).aspx

Just thought I would suggest it because it sounds like fixing up what's happening with PyrThread might be too much trouble.

Okay, I made a map like so:

static std::map<PyrThread*, std::exception_ptr> lastExceptions;

to store the exception by thread. Seems to work. If there's no obvious issue with that approach I'll clean it up and push for further review.

@muellmusik seems like something that should still go in VM globals (I'm assuming that's not where you put it because of static in the declaration) but either way works for me. :) thanks!

seems like something that should still go in VM globals

Yes, that makes sense. I think I'll change it to hold all the relevant info so we don't need to split it over more than one a place.

Have a look at this @brianlheim. If we're happy I can make a clean PR before merging.

Thank you @muellmusik ! Traveling today but I will try to get a chance to look at it. May have to wait until tomorrow.

No worries. Before I forget, do you have anything that will throw something besides an exception? We need to test that catch.

I have no idea.. I was planning on inverting the catch clauses in order to test. It would be fun to add some primitives to the debug build of sclang that do nothing but throw std::domain_error; or throw std::string("Catch me");.

It would be fun to add some primitives to the debug build of sclang that do nothing but throw std::domain_error; or throw std::string("Catch me");.

Yeah, I thought I might make a dummy primitive.

Okay, tried this

static int prThrowCrap(struct VMGlobals * g, int numArgsPushed)
{
	printf("throwing\n");
	throw std::string("What's this?");
	return errNone;
}

and needed one little fix.

BTW, I notice that a number of primitive invocations aren't followed by ^this.primitiveFailed. This strikes me as rather bad practice, unless either 1) We can be 100% sure that a given primitive will never throw any sort of exception, 2) We know a primitive could throw exceptions but we want to suppress them all.

Neither of these strikes me as credible, and in 1) there'd be no harm done by adding it, just in case.

If you agree I'll file a bug about it.

Sorry for the delay! I had to catch up on work. I just tested this (both the std::exception and the non-std::exception branches) and it works great. Thanks so much for this.

BTW, I notice that a number of primitive invocations aren't followed by ^this.primitiveFailed. This strikes me as rather bad practice, unless either 1) We can be 100% sure that a given primitive will never throw any sort of exception, 2) We know a primitive could throw exceptions but we want to suppress them all.

I completely agree. If you wouldn't mind filing a ticket for it, that would be great!

BTW, I notice that a number of primitive invocations aren't followed by ^this.primitiveFailed. This strikes me as rather bad practice, unless either 1) We can be 100% sure that a given primitive will never throw any sort of exception, 2) We know a primitive could throw exceptions but we want to suppress them all.

Those need to be reviewed case-by-case.

It is perhaps fair to say that all primitive invocations in the class library should be followed by an explicit return, even if it's only ^this.

It's flatly wrong to suggest that every time a primitive function in the backend returns an error code, it should always be treated as a fatal error.

@jamshark70 can you elaborate a bit? i'm having a hard time thinking of a reason why a current silent primitive failure shouldn't be an explicit silent primitive error.

@jamshark70 They should be handled one way or another. If there's fallback SC code as there is in some cases fine, but I can't think of a case for ^this. Or?

In principle, I agree that a primitive invocation should have an explicit fallback in the language, and I agree that the fallback should either return something meaningful or throw an error. An explicit ^this would be pretty dodgy unless we're sure a no-op is OK (but, never say never).

BUT, "This strikes me as rather bad practice, unless either 1) We can be 100% sure that a given primitive will never throw any sort of exception" -- we might be confusing a couple of terms here. If a primitive throws an exception, it had better be caught somewhere in C++, or the language is going down. If a C++ exception is caught, then it could move on to the next point.

BUT, if a primitive hits an error condition, what it's supposed to do is return an error code, at which point the SC interpreter falls back to the byte codes in the method. If the primitive does return errNone, the fallback does not execute. So, if all of the primitive's return points are errNone, then we can be certain that execution will never reach the fallback. The primitive will be successful, or it will crash the whole process -- e.g., Platform:systemAppSupportDir.

If there is a possibility of a primitive failure crashing the process, then ^this.primitiveFailed will solve nothing. It would be valid to add it after adding the code to catch the C++ exception -- but in the absence of that, it's purely cosmetic.

Scott had gone on to say "Neither of these strikes me as credible" -- I agree, it's in-credible to suppose that exceptions will never be thrown anywhere in SC's C++ backend. But it is totally credible that some/many fallbacks would be redundant.

Here's a complete list of primitive-invoking methods that end with ^this (either implicitly or explicitly -- there's no way through introspection to tell them apart).

(
m = List.new;
var returnSelf = Int8Array[0xF4];
Class.allClasses.do { |cl|
	cl.methods.do { |method|
		if(method.primitiveName.notNil and: { method.code == returnSelf }) {
			m.add(method);
		}
	}
};
m.do(_.postln); ""
)

ArrayedCollection:indexedSize
ArrayedCollection:size
ArrayedCollection:maxSize
Boolean:keywordWarnings
Boolean:trace
Char:ascii
Char:toUpper
Char:toLower
Char:isAlpha
Char:isAlphaNum
Char:isPrint
Char:isPunct
Char:isControl
Char:isSpace
Char:isDecDigit
Char:isUpper
Char:isLower
Class:dumpClassSubtree
Float:as32Bits
Float:high32Bits
Float:low32Bits
FunctionDef:dumpByteCodes
FunctionDef:numArgs
FunctionDef:numVars
FunctionDef:varArgs
FunctionDef:dumpContexts
Integer:nextPowerOfTwo
Integer:isPowerOfTwo
Integer:leadingZeroes
Integer:trailingZeroes
Integer:numBits
Integer:log2Ceil
Integer:grayCode
Integer:nthPrime
Integer:prevPrime
Integer:nextPrime
Integer:indexOfPrime
Integer:isPrime
Integer:exit
Meta_AppClock:prSchedNotify
Meta_Class:allClasses
Meta_Date:seed
Meta_Font:availableFonts
Meta_LanguageConfig:currentPath
Meta_LanguageConfig:includePaths
Meta_LanguageConfig:excludePaths
Meta_LanguageConfig:postInlineWarnings
Meta_Main:scVersionMajor
Meta_Main:scVersionMinor
Meta_Main:scVersionPostfix
Meta_Pen:matrix
Meta_Process:tailCallOptimize
Meta_Process:elapsedTime
Meta_Process:monotonicClockTime
Meta_QtGUI:cursorPosition
Meta_ScIDE:connected
Meta_Thread:primitiveError
Meta_Thread:primitiveErrorString
Object:dump
Object:totalFree
Object:largestFreeBlock
Object:gcDumpGrey
Object:gcDumpSet
Object:gcInfo
Object:gcSanity
Object:canCallOS
Object:prHalt
Object:dumpBackTrace
Object:getBackTrace
Object:crash
Object:stackDepth
Object:dumpStack
Object:dumpDetailedBackTrace
Object:pr_asCompileString
Platform:recompile
Platform:userHomeDir
Platform:systemAppSupportDir
Platform:userAppSupportDir
Platform:systemExtensionDir
Platform:userExtensionDir
Platform:userConfigDir
Platform:resourceDir
Platform:ideName
String:hash
String:asCompileString
String:prPostln
String:prPost
Symbol:asClass
Symbol:isMap
Thread:randData
Thread:failedPrimitiveName

BUT, if a primitive hits an error condition, what it's supposed to do is return an error code, at which point the SC interpreter falls back to the byte codes in the method. If the primitive does return errNone, the fallback does not execute. So, if all of the primitive's return points are errNone, then we can be certain that execution will never reach the fallback. The primitive will be successful, or it will crash the whole process -- e.g., Platform:systemAppSupportDir.

I am not sure if I'm reading this right, but have you considered that a C++ function invoked by a primitive might not return anything at all, and instead throw an exception, which will be caught by the interpreter, and cause the fallback to execute? It seems to me that that's precisely what this PR has been about. Yes, certain exceptions can cause the interpreter to terminate, but others can be caught and managed, as we've seen above with NetAddr.sendMsg.

But it is totally credible that some/many fallbacks would be redundant.

I don't see why redundancy is anything to avoid here. Yes, you can 'guarantee' through inspection that some functions are probably not going to throw an error, but that's only a guarantee on that bit of code, one of many possible implementations. I think this point is amplified by the fact that having no fallback leads to a silent failure, which may manifest in ways that are difficult to detect and difficult to solve.

I also think that if the concern is only that added code may be dead code under the current implementation, then what you are asking for is more work for no effective difference, and may be pitting the great against the good.

I see, I did misunderstand the issue. Sorry for that. I hadn't found where primitive exceptions were caught. Now I see, yes, you're right, it is possible for some function to fail and enter the fallback.

It sounded to me at first like we were going to throw interpreter errors for every primitive failure, which would be bad, so I thought caution would be warranted. I'm sometimes suspicious of "let's make a whole bunch of changes in lots of places where there's theoretically a problem but we aren't actually observing any problems" -- but here, the principle is right, I'm just reflexively reacting with "if it ain't broke..." (to which the rejoinder would be, it might be broke tomorrow).

If a primitive throws an exception, it had better be caught somewhere in C++, or the language is going down.

Totally confused by this, James. It will be caught in doPrimitive. No primitive can bring the language down just by throwing an exception. Yes, it may make things unstable in some way, but then better to get an error, yes?

You may want to catch exceptions within the primitive itself, and deal with them without throwing to doPrimitive's catch. Fine. But those should target specific exceptions, so that any unexpected ones are still thrown to doPrimitive. But if there's no this.primitiveFailed in the fallback code all such exceptions are silent.

So, if all of the primitive's return points are errNone, then we can be certain that execution will never reach the fallback.

Nope. If a primitive throws an exception and doesn't catch it itself, it will be caught in doPrimitive and return errException. Observe:

	try {
		err = (*def->func)(g, numArgsNeeded); // primitive happens here
	} catch (std::exception& ex) {
		g->lastExceptions[g->thread] = std::make_pair(std::current_exception(), meth);
		err = errException;
	} catch (...) {
		g->lastExceptions[g->thread] = std::make_pair(nullptr, meth);
		err = errException;
	}

So, no, just because a primitive's explicit return points are all errNone doesn't mean it can't have errException as a result.

I could see a case however for a more nuanced error handling, with say an errFallback to distinguish cases where you want the lang fallback from other kinds of failures.

Sorry, crossing messages.

What we're suggesting should result in no changed behaviour if no exceptions are thrown. If one is, then we'll actually know about it. So worse case I think we might shake loose a bug that was previously suppressed.

will this make failures like non boolean in test escspable? ( currently there's no way for overriding if or while, si that would be a good side effect )

Do you mean can you do this:

try { if(x=4, { "this is ok"}); } {}

and catch the non-Boolean error? Yes.

No, Julian is looking for an end-run around inlined control structures.

This PR will have no effect on inlining, because inlining involves no primitives or method calls.

ah yes, I see. It would be great to be able to bail out from wherever inlining happens, but that's another thing altogether.

I know it's an aside, but that "if" override keeps coming back, so... overriding inlined control structures would require "falling back" to a method call if the control opcode fails. AFAICS method calls for control structures require Function objects for the branches to be passed as arguments -- but the branches have already been compiled as inline bytecodes. So the branches would have to be compiled twice -- once inline, and again as Functions living in the containing FunctionDef. Then you would need fallback bytecodes -- that is, now we have something like (doing it by hand, so consider this pseudocode):

if(x > 0) { a } { b }

-->

Push x
Push 0
Do '>'
Jump if false to "aa"
Push a
Jump to "bb"
"aa": Push b
"bb": Block return

But an "if" override could be done by:

Push x
Push 0
Do '>'
Jump if false to "aa", on failure to "bb"
Push a
Jump to "cc"
"aa": Push b
Jump to "cc"
"bb": Leave condition on stack
Push true func
Push false func
Call method "if"
"cc": Block return

You can't call "if" as a method if the arguments don't exist, and you can't skip this for any inlinable control structure, because in a duck typed language, it's impossible for the compiler to be sure of the return type of any operation. So the entire class library and all user code would bear this redundancy.

AFAICS there is not nor ever will be a magic trick to avoid this redundancy -- just like there's no magic trick in a SynthDef to get the server to add UGens and reoptimize the graph based on control inputs. Ya just have to live with it... and it's worth it, here, for the 2-3 order of magnitude speed improvement.

thank you for this clarification, @jamshark70 . Probably the only way to go about cases that need to be "escaped" is to introduce a set of non inlined methods that can be used if needed:

+ Boolean {

	if1 { |trueFunc, falseFunc|
		^this.if(trueFunc, falseFunc)
	}

	while1 { |trueFunc, falseFunc|
		^this.while(trueFunc, falseFunc)
	}

}

+ Function {
	loop1 {
		^this.loop	
	}
}