Segmentation fault on object destruction

[quou]

Hello,

I have got Jinx up and running, now I'm trying to abstract it into a single class, so that I can compile and run a script a little easier.

I made the library, runtime, script and bytecode pointers into members, like so:

class ScriptWrapper {
private:
    Jinx::LibraryPtr m_library;
    Jinx::RuntimePtr m_runtime;
    Jinx::ScriptPtr m_script;
    Jinx::BufferPtr m_bytecode;

Then, I use a method compile the script, like so:

m_runtime = Jinx::CreateRuntime();

std::string scriptText;
// ... load the script from a file

m_library = m_runtime->GetLibrary("");

m_bytecode = m_runtime->Compile(scriptText.c_str());

if (!m_bytecode)
    return;

m_script = m_runtime->CreateScript(m_bytecode);

Then, I have another method that simply calls m_script->Execute();. This works perfectly, until the object containing all this is destroyed. It prints two messages to the console, both saying: "Could not free block at shutdown. Memory still in use." and then segfaults from Jinx::Impl::BlockHeap::FreeBlock, at line 5,321 of Jinx.hpp. Is this a bug, or am I doing something wrong?

[JamesBoer]

I was able to reproduce this by calling Jinx::Shutdown() before all the objects were destroyed (in Tests/Features/Main.cpp, I commented out the braces at lines 73 and 82). Is your class perhaps a static or global object? This seems like it could be a static destruction order issue, in which the allocators are getting destroyed before the Jinx objects.

[JamesBoer]

I've checked what I believe should fix for the segfault you're seeing in the Development branch. You can grab the code from there to see if it works for you.

However, the leak will still occur. As I indicated in the last post, I'm fairly certain this is cause by the static heap object being destroyed before the Jinx objects are being destroyed. At some point, I'll try to figure out a more robust solution to this problem, but for now, there are a few options to fix this.

The first option it to ensure you destroy all Jinx objects explicitly at shutdown. This means either adding a Clear() function that destroys all the Jinx objects (just set them to nullptr), or destroying the wrapper object itself explicitly at program termination.

The second option is to just disable the pool allocator completely, bypassing the problem. If you find the line containing #define JINX_DISABLE_POOL_ALLOCATOR (currently commented out), you can disable the pool allocator completely. Or, since you're using the amalgamated head, you can just #define it before you include Jinx.hpp. This won't compile in the current main branch, because of some changes made in how I suppress unused variables warnings. The latest development branch also fixes this issue.

Let me know how things go.

[quou]

Using the development branch and uncommentating #define JINX_DISABLE_POOL_ALLOCATOR worked like a charm. I'm really enjoying Jinx so far, the language itself takes a little getting used to, its not at all similar to anything else I've used before, but the API is very nice to use. I was using ChaiScript for this project before, but I had issues with speed, and it also relies too much on try/catch blocks.

[JamesBoer]

Glad that worked for you. Let me know if you run into any issues or have any other questions.

[jerryjeremiah]

Disabling the pool allocator works but it exists for a reason, so what are we giving up by not using it?

[JamesBoer]

In theory, it was to help improve performance. In practice, it no longer seems to provide any significant performance benefit. Either compilers or their default runtime allocators are improving since I tested performance four or five years ago, or other optimizations I've made since then have negated the benefits. I just now re-tested on three platforms, and there appears to be zero performance gain over plain malloc/free.

So, I'm actually considering removing the pool allocator completely, as that's a lot of complex machinery that provides no benefit, and has some drawbacks, like this issue.

Naturally, custom allocators will still be supported, which is probably a better way to get allocator-based perf benefits anyhow. And I may rework the memory tracking so it supports any allocator, including std malloc/realloc/free. It adds a tiny bit of memory and runtime overhead, but I think it's negligible enough to keep that memory stats API in place.

Let me know if you have any opinions on this.