395
edits
User:Philosopher/Nasal FGCommand crashes: Difference between revisions
Jump to navigation
Jump to search
User:Philosopher/Nasal FGCommand crashes (view source)
Revision as of 13:08, 9 April 2014
, 9 April 2014→Current status: good idea H, that seems right!
m (→Current status) |
Philosopher (talk | contribs) (→Current status: good idea H, that seems right!) |
||
| Line 28: | Line 28: | ||
Well, it will keep calling bind() for each invocation/parse() of the binding [https://gitorious.org/fg/flightgear/source/740b3f35e98b8c0506bc887326ef831704520f89:src/Scripting/NasalSys.cxx#L1113]. And it's all in a fresh context using a new func naRef - so it may be possible, that previously bound stuff is still lingering around - but we need to dump the namespace to see what's going on there for tens of thousands of invocations. The short-term "fix" might be NOT re-parsing things over and over again, and checking if that solves the problem or not. After all, it seems unrelated to compiler optimizations, which is a good thing :) We may also want to check Andy's usage of naBindFunction() vs. naBindToContext() [https://gitorious.org/fg/simgear/source/19481983e5e67c60603acb41d42ff406751ada0c:simgear/nasal/code.c#L834]. | Well, it will keep calling bind() for each invocation/parse() of the binding [https://gitorious.org/fg/flightgear/source/740b3f35e98b8c0506bc887326ef831704520f89:src/Scripting/NasalSys.cxx#L1113]. And it's all in a fresh context using a new func naRef - so it may be possible, that previously bound stuff is still lingering around - but we need to dump the namespace to see what's going on there for tens of thousands of invocations. The short-term "fix" might be NOT re-parsing things over and over again, and checking if that solves the problem or not. After all, it seems unrelated to compiler optimizations, which is a good thing :) We may also want to check Andy's usage of naBindFunction() vs. naBindToContext() [https://gitorious.org/fg/simgear/source/19481983e5e67c60603acb41d42ff406751ada0c:simgear/nasal/code.c#L834]. | ||
Context... that's a good point. How about [https://gitorious.org/fg/flightgear/commit/0fbc448 commit 0fbc448]? That changed it so it creates and then deletes a context (instead of using global one), and is about the right timeframe - between 2.12 and present (it's dated Nov 22, 2013). It looks like we can still revert it (there's a global context still), so I'll see what that does, but it's awfully suspicious if it would cause problems. But I do agree that freeing the wrong thing could definitely be a problem - if you lose the context and free the function, it's really going to get screwed over, and possibly rewritten with anything else. And since the whole Nasal and GC path is deterministic (so far), that would support why I keep getting the same indexes over and over again - just trigger it in the wrong place and the function goes kaboom. | |||
: Actually, to keep going off on that, it looks like freeing the context immediately is quite dangerous. naNew(ctx...) calls naTempSave(ctx...), which allows ''active'' naContexts to keep the function alive (as long as they don't run code...), but obviously naFreeContext(ctx) doesn't keep it alive, which would allow for GC'ing the objects saved with naTempSave(ctx...) - which, for example, can happen in the if statement you mentioned, which [https://gitorious.org/fg/flightgear/source/740b3f35e98b8c0506bc887326ef831704520f89:src/Scripting/NasalSys.cxx#L1151 creates a new hash] if needed. (And I believe things started crashing much earlier when I deleted the namespace each time, which would support that.) In addition to the timing seeming right, this all seems to fit, so I'll revert the commit and test it ASAP. This also does raise doubts for me about threaded GC though... with creation and deletion of contexts, anything threaded might slip into the cracks. (You weren't on the list, but James sent out an email about 1-2 weeks ago proposing a threaded GC patch again, and I can obviously send that to you/put it up on gitorious.) | |||
: I also added assertions (and AFAIK they're active), which basically checks that what's going in is what's going to come out, and everything looked fine so far, so that would point to corruption instead of a bad pointer being passed to it. | |||
=== Available backtraces === | === Available backtraces === | ||