wxSimplebook causes link conflict

[marekr]

This is a linker bug introduced in 3.2.0, wxSimplebook was derived from wxNavigationEnabled<wxBookCtrlBase> instead of wxBookCtrlBase.

But per the comment above it

// NB: This class doesn't use DLL export declaration as it's fully inline.

The problem is other classes like wxAuiNotebook use wxNavigationEnabled

class WXDLLIMPEXP_AUI wxAuiNotebook : public wxNavigationEnabled<wxBookCtrlBase>

thus the wx DLLs end up with their own templated version of wxNavigationEnabled<wxBookCtrlBase> exported.

But because wxSimplebook is an inline class, your application will also generate its own wxNavigationEnabled<wxBookCtrlBase> expansion.

The result is a conflict if you use wxSimplebook and one of the other wx book controls.

E:\kicad\kicad\build\x64-Release\kicad\wxmsw32u_aui.lib(wxmsw32u_aui_vc.dll) : error LNK2005: "public: __cdecl wxNavigationEnabled<class wxBookCtrlBase>::wxNavigationEnabled<class wxBookCtrlBase>(void)" (??0?$wxNavigationEnabled@VwxBookCtrlBase@@@@QEAA@XZ) already defined in panel_display_options_base.cpp.obj
E:\kicad\kicad\build\x64-Release\kicad\wxmsw32u_aui.lib(wxmsw32u_aui_vc.dll) : error LNK2005: "public: virtual __cdecl wxNavigationEnabled<class wxBookCtrlBase>::~wxNavigationEnabled<class wxBookCtrlBase>(void)" (??1?$wxNavigationEnabled@VwxBookCtrlBase@@@@UEAA@XZ) already defined in panel_display_options_base.cpp.obj
 

[AliKet]

Does exporting wxSimplebook solves the problem for you ?

[vadz]

Thanks for reporting this, it's clearly a problem, but I'm not so sure it's wxSimplebook's fault -- wouldn't the same thing happen if you derived your own class from e.g. wxNavigationEnabled<wxWindow>?

This class is pretty weird in general because it exports its individual members instead of (in spite of?) not being exported itself. Unfortunately 8738110 doesn't provide much explanation, but perhaps we should use WXDLLIMPEXP_INLINE_CORE for the ctor and dtor too (Edit: this is not going to work, this macro is empty when not using visibility anyhow)? Or maybe we should make the entire class DLL-exported and explicitly instantiate it for all the classes it's used with inside wx?

In any case we clearly need some good tests for this as this isn't the first time this gets broken, judging from the history of the code.

[marekr]

wouldn't the same thing happen if you derived your own class from e.g. wxNavigationEnabled<wxWindow>?

Yes, the same would happen. However, the hack here is if you #include say, "splitter.h" which already uses wxNavigationEnabled<wxWindow> as the parent for wxSplitterWindow being exported. Since the child class is exported, that's enough hinting that to the compiler that an implementation exists elsewhere so it'll behave.

wxSimplebook can't be fixed because it's an inline class so the compiler doesn't care about exported classes using the same template. I'm not familiar with what decisions drive that in MSVC/C++.

I actually had a similar issue with wxScrolledCanvas in kicad 2 years ago because it's only a typedef to a templated class. I had to add "wx/grid.h" includes to give the hint that's its implemented in wxWidgets to avoid the link conflict.

I'm not sure what the cleanest fix would be but it would require declaring template specialization in a header somewhere as extern sort of like

extern template class WXDLLIMPEXP_INLINE_CORE wxNavigationEnabled<wxWindow>
extern template class WXDLLIMPEXP_INLINE_CORE wxScrolled<wxPanel>;

(Note I haven't had a chance to play with it, I hotglued/reverted problem in our CI for now)

[vadz]

I've tried reproducing this bug but couldn't: after adding

    #pragma comment(lib, "wxmsw33u_aui")
    wxSimplebook sb;
    wxAuiNotebook ab;

to the minimal sample (and the corresponding includes at the top), I can still link without problems using

nmake -f makefile.vc SHARED=1 BUILD=release

What am I missing here?

[marekr]

I'm not 100% sure what the link behavior breakage is at the moment, it might have to do with kicad building multiple object libraries under cmake which get linked into DLLs and that's where it blows up.
I was short on time and I just patched wx by removing the template for wxSimplebook heh.
I will try and poke it soon.

[vadz]

I'd like to fix this in 3.2.2 if possible, but it would be great to have a way to reproduce this in order to do it.

[marekr]

Yea It's been killing me to get around to this.

I think the issue comes up when you use wxSimplebook in a static module and that gets pulled in by linker again into an executable or shared library that also used wxSimplebook directly within themselves. The result I believe is both link objects contain wxSimplebook that isn't tied to a singular extern and so have separate template expanded copies. So I would need to set that up as a test

[vadz]

I think the solution might be as simple as creating some wxNavigationEnabledBookCtrlBase simply inheriting from wxNavigationEnabled<wxBookCtrlBase> and doing nothing else, but having a non-inline ctor and/or dtor and being DLL-exported and then derive wxSimplebook and all the other classes from that class instead.

AFAICS this should force instantiating this class only once inside the DLL and so solve the problem. Unfortunately I am not sure if this is really going to be ABI-compatible (in fact I'm almost sure it's not), but we could at least fix this in master like this.

But, again, I can't even test this myself, so it'd be great if you could please do it (I could make the PR for you to test if this would help).

[marekr]

Well, I sort of have a repro ....with some other things broken I'm trying to sort out so that it's not a false repro.

The big problem for me is we don't usually use msbuild at all, we roll cmake with vcpkg...which makes a few things easier for me. So this is taking some time for me to relearn msbuild :D

[vadz]

Sorry, not sure why do you need MSBuild? If you can just give/show me the minimum setup needed to see it, I can build myself. Or, as I wrote above, if can at least test my proposed fix, it could also be enough.

TIA!

[marekr]

Well, kicad does not make a good repro case unless you have say....a Ryzen 5900X or higher in processor, if you want the build time to be less than 12 hours (all our dependencies together are absolute monsters) :D

Here, I finally wrestled it to work under a standard msbuild project within the wxwidgets source, dump the wxsimplebooklink project to the samples folder.

Use the DLL Debug, x64 config.

wxsimplebook link error repro.zip

[vadz]

Thanks, I can confirm that my proposed fix fixes your test case (I still get some warnings for PANEL_DUMMY_BASE ctor and dtor but I don't think it's due to any problems in wx), so I've created #23185 with these changes.

Please let me know if you can test this with KiCad and if you still have this problem with it. TIA!

[marekr]

Please let me know if you can test this with KiCad and if you still have this problem with it. TIA!

Unfortunately because of the way we build kicad usually with vcpkg, it's a little difficult for small things to patch.

I have confidence your change fixes it, since this purely has to do with the child template expansion of wxNavigationEnabled being generated outside of wx since wxSimplebook lacked any export declaration as a hint for MSVC to instead look for it during linkage. Now that you just threw it into a dummy base class that is exported, it'll be fine even if wxSimplebook itself isn't exported.

[vadz]

OK, I'll merge this into master soon. I'm afraid I don't see how can we fix it without breaking the ABI in 3.2, so we'll have to leave it there.

[marekr]

OK, I'll merge this into master soon. I'm afraid I don't see how can we fix it without breaking the ABI in 3.2, so we'll have to leave it there.

Well, a dumb idea is throw a #include wxListbook in the simplebook header. This way MSVC should see the exported wxListbook which also includes the same wxNavigationEnabled as a hint that the template expansion is being exported

[vadz]

Well, a dumb idea is throw a #include wxListbook in the simplebook header. This way MSVC should see the exported wxListbook which also includes the same wxNavigationEnabled as a hint that the template expansion is being exported

If this actually fixes the problem, we should indeed do this, thanks for the idea!