-
Notifications
You must be signed in to change notification settings - Fork 544
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
Fix missing template instantiation in RelWithDebInfo builds #731
base: master
Are you sure you want to change the base?
Conversation
What is the problem exactly? |
On Linux? I tried many variations and they fail linking. For example:
the result is: [100%] Linking CXX executable mangosd If DDEBUG=1, this doesn't happen. It's only with RelWithDebInfo. But adding that include fixes it. |
Why are you using tbb? Also why are you setting DEBUG to 0?
compiles and links just fine |
I could not get cmangos to build at all in my Garuda Linux environment w/ i7-13700K CPU without using tbb. Is there a reason I shouldn't use tbb? I am using DDEBUG=0 because it runs much faster but I still get address sanitize and stack trace with RelWithDebInfo. And can you explain why #include "Grids/CellImpl.h" should not be added? If it’s required for proper linkage in some configurations, shouldn’t it be included explicitly? |
Well we don't officially support tbb and we've had several bug reports involving CMaNGOS compiled against tbb in the discord. I can't speak to how valid they are, however. |
Ok thanks, that makes sense about tbb, but like I said I need it to be able to build. This is only the case on my 13700K machine, it builds fine without tbb on my vanilla Arch Linux Ryzen 5800X... not sure why this is the case, but it was a solution for my Intel machine. The function Cell::VisitAllObjects is declared in Cell.h but only defined in CellImpl.h. If CellImpl.h is not explicitly included, the function template isn’t instantiated correctly, leading to an undefined reference error at link time. Seems like the issue may not appear for everyone because PCH can mask missing includes by precompiling headers from other parts of the codebase... but fixing it anyway would be nice so there aren't any configurations that won't link. |
There seem to be two different things at play here. First, the use of tbb. Using tbb is not entirely a user choice any more these days. GCC uses it for the parallel algorithm these days (because it was a contribution by Intel). See for example the documentation on https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017, and in particular note 3 there:
As far as I'm aware, we're not using tbb nor the parallel algorithms directly, so there's no need for tbb from our side. But I vaguely remember someone mentioning an issue where something included Then there's the actual undefined reference you get. A missing include leading to a missing template instantiation makes sense. I can't immediately see why 'release vs debug' makes a difference (I'll check because I'm genuinely curious now), but I do agree that necessary and missing includes should be added. |
Yes, I'm using -D_GLIBCXX_USE_TBB_PAR_BACKEND=1 -ltbb What you are saying about GCC sounds right. I think it was boost-related linking problems, but it was months ago that I figured out I needed to adjust my cmake options to use tbb in order to get it to link on this machine... and haven't had a problem since. But it was never a problem on my Ryzen machine, it links everything fine with or without tbb, with the same GCC and libraries. It's strange. |
🍰 Pullrequest
Fixes a linking issue on Linux for
VisitAllObjects
inRelWithDebInfo
(-O2 -g
) and ASan (-fsanitize=address
) builds by ensuringCellImpl.h
is included inTargetedMovementGenerator.cpp
.Proof
RelWithDebInfo
,Debug
,Release
).Issues
How2Test
-DCMAKE_BUILD_TYPE=RelWithDebInfo -fsanitize=address
on Linux.VisitAllObjects
.Todo / Checklist
VisitAllObjects
is correctly instantiated.