Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SetAllocatorFunctions() accross DLL #3836

Closed
n00bmind opened this issue Feb 23, 2021 · 6 comments
Closed

SetAllocatorFunctions() accross DLL #3836

n00bmind opened this issue Feb 23, 2021 · 6 comments
Labels

Comments

@n00bmind
Copy link

Version/Branch of Dear ImGui:

Version: 1.77
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: custom
Compiler: MSVC 19.16.27035
Operating System: Windows 10

My Issue/Question:

In my setup I have an exe that loads a dll containing game code. The game code contains most of the ImGui code, however the per-frame functions (NewFrame, Render..) happen in main(), which is in the exe of course.
I've been using ImGui like this for some years now, however after a recent upgrade I started getting a crash. The crash was always in a debug CRT heap, with the message

Debug Assertion Failed! Expression: __acrt_first_block == header

I decided to track it down, because it was very reproducible, and it turns out it's caused by the new(ish) GcCompactTransientWindowBuffers (called from NewFrame(), if I'm not mistaken). I got a clue from a stack overflow answer that this can usually happen due to allocations across a DLL boundary, which is exactly what's happening here.

I am using the default allocators provided (i.e: malloc & free), meaning both GImAllocatorAllocFunc & GImAllocatorFreeFunc in imgui.cpp are different for the exe & dll and hence use separate heaps. My assumption is this has not been a problem so far because ImGui wasnt freeing much until that GC process was introduced? (and ofc I'm not even calling DestroyContext during shutdown).

This problem is easily fixed if I make a call during the setup of my DLL to SetAllocatorFunctions() next to my current SetContext() call, passing in the global allocators used by the exe (I had to hack an accessor to those, since the variables are static in imgui.cpp).

I understand this is not a bug in the library per se.. however I feel the situation for this kind of use case could be improved a bit. There is mention of the need to call SetAllocatorFunctions when hot-reloading, however please note this is a problem caused just by the bridge EXE - DLL itself, and it happens even if the dll is just loaded once on startup. I'm kind of surprised this hasn't been noted before, to be honest.

I feel the comments should clarify the need to do something similar to what I did whenever ImGui code is used across dynamically linked libraries, and also provide a GetAllocatorFunctions() or similar. I don't know.. just wanted to raise this as I thought maybe this wasn't too visible.

(Sorry I didn't provide a code example, but this requires quite a specific setup and not just code).

@PathogenDavid
Copy link
Contributor

The core issue is you essentially have two copies of Dear ImGui in your program: One in the executable and one in the library. This means you have two copies of GImGui, GImAllocatorAllocFunc, GImAllocatorFreeFunc, and GImAllocatorUserData. If they don't match between the two copies you run into the issues you've seen.

In my opinion, the proper solution to this is either:

  • Only statically link ImGui in your game logic DLL and access it from the host exe through that
  • Build ImGui as a DLL and reference it from the executable and the DLL

Otherwise I think you've gone into weird setup territory and it's not a huge ask for you to call SetAllocatorFunctions with your own allocator.

I feel the comments should clarify the need to do something similar to what I did whenever ImGui code is used across dynamically linked libraries

Maybe the wording could be improved, but the hot reloading comments you're referring to does:

Important: globals are not shared across DLL boundaries! If you use DLLs or any form of hot-reloading

(Extra emphasis on "or")

The comments focus on hot reloading because that's just the most common reason people have multiple copies of ImGui within the same app.

@n00bmind
Copy link
Author

Thanks for the prompt answer.
I'm pretty sure my setup is not that uncommon around here :)
And indeed, I'm not saying it's a huge ask for me to call SetAllocatorFunctions and I intend to do so from now on.

Let me reiterate, as maybe my intention was lost among that wall of text I left:

  • I think it is not that clear by reading the comments that in a situation where there's an exe-dll boundary (pretty typical) you're now going to be required to call SetAllocatorFunctions in addition to the usual SetContext call if you want to use ImGui functions in both contexts. That comment you refer to ("globals are not shared across...") continues right after with "you will need to call SetCurrentContext()..." but does not refer to the allocator functions. A few lines below the comments and code do talk about the allocators, but the comments only refer to hot-reloading situations. I think this is a first point for improvement.
  • Second, and more importantly I think, there's no current way for users to access the global allocator functions used in the exe address space so they can be set up appropriately in the dll when setting up the ImGui context. I did have to add a makeshift GetAllocatorFunctions in imgui.h/cpp in order to do this. This is I think, required, for people that don't want to install their own memory allocators but still want their setup to just work.

Hope I was a bit clearer this time.

@ocornut ocornut changed the title Crash during GcCompactTransientWindowBuffers due to different global default allocators in DLL code SetAllocatorFunctions() accross DLL Feb 24, 2021
@ocornut ocornut added the doc label Feb 24, 2021
@ocornut
Copy link
Owner

ocornut commented Feb 24, 2021

Hello,

I'll add those comments then.

This is I think, required, for people that don't want to install their own memory allocators

If you don't install allocators then things should just work without any calls?
I'm not sure I understand why you would need a GetAllocatorFunctions(). Would the DLL space versions of those global initialize in the same way as the EXE address space one?

@n00bmind
Copy link
Author

n00bmind commented Feb 24, 2021

Hi!
The problem is, as I said in the first post, with this setup one needs to make sure both the EXE and the DLL use the same heaps, which they won't by default. (as I keep mulling over this, I just realized this is probably only a problem because I'm linking the static version of the CRT in both?)

Otherwise, when the DLL calls ImGui functions causing allocations, it will use it's own heap, then when the EXE calls BeginFrame(), which will trigger the GC process, memory will be freed up using a different allocator, with its own separate heap, which will most likely crash.

Now the only way I found to make sure the DLL uses the same allocators as the EXE (please correct me if there's another way), is to get the addresses of GImAllocatorAllocFunc and GImAllocatorFreeFunc in the EXE address space and pass those to the DLL even though I'm just passing the exe's default allocators, but there is no current way to current way to do so, because those symbols are only declared as static in the .cpp.

@ocornut
Copy link
Owner

ocornut commented Feb 24, 2021

See 6f4b9c6

@n00bmind
Copy link
Author

Seems great! Thanks for the quick resolution!

ocornut added a commit that referenced this issue Mar 4, 2021
…her functions. + Made the ImGuiMemAllocFunc / ImGuiMemFreeFunc consistent with our other typedefs (#3836)
duddel added a commit to duddel/imgui that referenced this issue Mar 4, 2021
commit ee643b2
Author: ocornut <[email protected]>
Date:   Thu Mar 4 19:59:59 2021 +0100

    IsItemHovered(): fixed return value false positive when used after EndChild(), EndGroup() or widgets using either... (ocornut#3851, ocornut#1370)

    ...when the hovered location is located within a child window, e.g. InputTextMultiline().
    This is intended to have no side effects, but brace yourself for the possible comeback..
    This essentially makes IsItemHovered() not accept hover from child windows, but EndChild/EndGroup are forwarded.
    More or less should fix/revert c76f014 which was a revert of 344d48b

commit b53b8f5
Author: Rokas Kupstys <[email protected]>
Date:   Thu Mar 4 16:27:43 2021 +0200

    Demo: Use correct string formats on non-windows platforms.

    (amended)

commit 3e6dfd3
Author: ocornut <[email protected]>
Date:   Thu Mar 4 13:37:14 2021 +0100

    ImDrawList: AddImageRounded() compare texid from cmdheader as with other functions. + Made the ImGuiMemAllocFunc / ImGuiMemFreeFunc consistent with our other typedefs (ocornut#3836)

commit 8dd692c
Author: ocornut <[email protected]>
Date:   Thu Mar 4 11:03:40 2021 +0100

    Android: Amend backend and examples with minor consistency tweaks. (ocornut#3446)

commit fb85c03
Author: duddel <[email protected]>
Date:   Thu Mar 4 10:35:44 2021 +0100

    Add Android backend and example (ocornut#3446)

commit d8c88bd
Author: ocornut <[email protected]>
Date:   Thu Mar 4 09:52:00 2021 +0100

    Tables: Fixed unaligned accesses when using TableSetBgColor(ImGuiTableBgTarget_CellBg). (ocornut#3872)

    ImSpanAllocator: Support for alignment.

commit 1ddaff8
Author: ocornut <[email protected]>
Date:   Wed Mar 3 18:45:52 2021 +0100

    Demo: Tweak inputs display.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants