ImGui DLL export issue


#1

Hi,

I found an issue with DLL exports i.c.w. ImGui.

Somehow this only happens when we include #include <imgui/imgui_internal.h> (which is needed when making custom ImGui controls), and gives an undefined reference for GImGui (which we don’t actually reference).

The problem originates from the fact that NAP_SHARED_LIBRARY is defined for each module one compiles,
and the presence of NAP_SHARED_LIBRARY for our own module, makes ImGui think it itself is being compiled, setting dllexport on the GImGui symbol.

The fix I made locally for now is to change NAP_SHARED_LIBRARY to NAP_SHARED_LIBRARY_IMGUI for the CMake file of ImGui.

From imgui.h,

// Define attributes of all API symbols declarations, e.g. for DLL under Windows.
#ifdef _WIN32
- #ifdef NAP_SHARED_LIBRARY
+ #ifdef NAP_SHARED_LIBRARY_IMGUI
#define IMGUI_API __declspec(dllexport)	// Export the symbols
#else
#define IMGUI_API __declspec(dllimport)	// Import the symbols

napimgui/CMakeLists.txt,

- target_compile_definitions(${PROJECT_NAME} PRIVATE NAP_SHARED_LIBRARY)
+ target_compile_definitions(${PROJECT_NAME} PRIVATE NAP_SHARED_LIBRARY_IMGUI)

This fixes the linkage issue. Not sure why this was never a problem before. Maybe it only occurs for globally exposed variables, and not functions or structs and classes? Seems like any time you’d include header files from another module or library the dllexport flag would be set? Or am I missing something?

Anyway, the above fix works. Hopefully this or a similar fix can be made to Nap.

Cheers,
Marcel


#2

Sorry,

This is the correct, new line, in CMakeLists.txt,

target_compile_definitions(${PROJECT_NAME} PRIVATE NAP_SHARED_LIBRARY NAP_SHARED_LIBRARY_IMGUI)


#3

Hi Marcel,

Thanks for bringing this up and a potential fix, although I don’t think it’s the right thing to do here. But I could be mistaken. Introducing a new compile directive is something I’d like to avoid.

I assume you are using NAP 0.2.3? In 0.3 we added the support for data driven parameters, including the option to register and render parameters using ImGUI. Here we also have a module (mod_napparametergui) that depends on mod_napimgui . However, mod_napparametergui only uses functionality exposed in imgui/imgui.h.

From what I can see imgui.cpp brings in imgui_internal.h, bringing in imgui_internal.h directly into your own module would indeed result in unresolved external bindings, as the interface isn’t exposed correctly in the first place because the compile directive is missing, and therefore can’t be linked to on Windows. Did you try including imgui.h before imgui_internal.h?

The export symbol is correct, also when you link from one dll to another, as we do this all the time, since many modules depend on functionality exposed in another module.

If I want to reproduce your issue, do I include imgui_internal.h directly in an other module?


#4

Hi Coen,

You are correct, we are using 0.2.3.

The problem is not with including the imgui module. It’s only when I include imgui_internal.h, as it exposes the GImGui symbol.

I’m including imgui.h first. imgui_internal.h has a guard to ensure imgui.h is included first. So dllexport would be defined. (note: dllexport, not dllimport, since the same preprocessor define is used for all modules.)

To reproduce: On Windows, include imgui_internal.h after including imgui.h in a module other the napimgui.

I think what happens is,

  • dllexport is defined when building modules.
  • The build creates an import library for each module.
  • dllimport is missing when including other modules.
  • Which is not a problem, since the import library will be linked, which has the right symbols, and the code necessary to look for the right symbols inside the DLL files.
  • But somehow this works correctly for functions and classes only, and not global variables (?).

dllimport afaik is optional, and will generate slightly faster code and offloads some of the work to the linker. But the import library should still work.

Marcel


#5

I found a related bit of information on MSDN, https://docs.microsoft.com/en-us/cpp/build/importing-into-an-application-using-declspec-dllimport?redirectedfrom=MSDN&view=vs-2019,

Using __declspec(dllimport) is optional on function declarations, but the compiler produces more efficient code if you use this keyword. However, you must use __declspec(dllimport) for the importing executable to access the DLL’s public data symbols and objects. Note that the users of your DLL still need to link with an import library.

Seems the behavior really is different for data types vs functions & classes!


#6

Yes, that seems to be true, ImGUI should have used a function to expose the global, which is considered good / standard practice. But I’m also pretty sure they did not intend to see imgui_internal.h used outside of dll bounds,. I’ll repro the issue and look for a good fix. I was planning on upgrading ImGUI sometime soon and will repro the issue / find a fix when I do.

PS: ran into a similar issue trying to expose some variables related to laser point bounds. That’s why we only export classes and functions, not variables. Interestingly enough, assigning and exporting the definition should do the trick when the GImGui variable is defined in imgui.cpp, according to some docs online:

https://docs.microsoft.com/en-us/cpp/cpp/definitions-and-declarations-cpp?view=vs-2019


#7

I’m pretty sure they do expect it to be used by developers across dll bounds, since nearly every function in imgui_internal.h is decorated with IMGUI_API. Also afaik using the functions in it is the only way to develop custom widgets.

They just expect dllimport to be correctly set for the (single) variable that needs it. I agree a function would’ve been nicer as it would’ve made it work with without dllimport, just the dllexport when building the dll, but that’s a decision they made which is hard to roll back I guess (with all of the 3rd party widgets that possibly depend on it).


#8

(Well actually all it would take for them to do so is to add the function, update the places inside the header which use the variable to use the function instead, and put the global behind a preprocessor macro so it can disabled from being exposed, either by default or opt-in.)