clang-cl /EHa with MSVC std::variant emits linker error LNK2019: unresolved external symbol
#93251
Comments
EugeneZelenko
added
clang-cl
|
Has anyone had any luck working around this? I have this exact same problem in a larger codebase. We're converting to clang to move towards Linux, so we'll have to wean ourselves off of SEH eventually, but it'd be real nice to keep it on Windows for the transition. |
Endilll
added
clang
|
I did an experiment, with in dumped IR. There are In which, the declare of For is called once by in IR built with Similarly, in is called twice by in I think this explains the difference between |
AaronBallman
added
the
clang:codegen
|
@llvm/issue-subscribers-clang-codegen Author: Stephan T. Lavavej (StephanTLavavej)
Repros with Clang 17.0.3 and VS 2022 17.11 Preview 1. MSVC accepts, but Clang rejects.
#include <variant>
using namespace std;
struct UDT {
UDT() {}
~UDT() {}
};
int main() {
using V = variant<int, double, UDT>;
[[maybe_unused]] V a{1729};
[[maybe_unused]] V b{3.14};
[[maybe_unused]] V c{UDT{}};
}Reduced from the original user report DevCom-10647850 "Linking bug of |
Thank you for the investigation!
CC @rjmccall @efriedma-quic @rnk for awareness (or to see if you have any bandwidth for addressing this one, as it seems pretty important) |
|
At first glance, this doesn't really make sense to me. The only way I can see that we wouldn't emit the definition of a destructor is if the destructor isn't odr-used. I have no idea how you could declare a variable in a way that would make CodeGen try to call a destructor that isn't odr-used. (If the destructor is in fact odr-used, some core infrastructure would have to be very fundamentally broken to avoid triggering deferred emission.) If someone could reduce a testcase that doesn't require including all of std::variant, that would be helpful. |
shafik
added
the
needs-reduction
|
CC @Endilll for help with reducing, in case he's got bandwidth |
|
I am in need for a fix. Is anyone actively working on this right now? If not, I can maybe have a look at it to see if I can fix it. |
Chances are no one does, so go ahead and submit a PR! |
|
I don't have time to look at this, but I'm happy to answer questions. |
|
I started digging into the issue. First of all, the destructor exists in the source:
The fact that it's missing is therefore not related to that. I took a look at the IR code and it looks a bit weird:
The destructor call is needed for the SEH scope, that is emitted due to /EHa. However, the scope is empty. So, at least to my understanding, the destructor would never be called, as there is nothing within that scope that can ever throw. The scope is generated for this constructor call:
The scope begin is emitted, then the constructor body (which is an empty compount statement representing the red circle), and then the scope ends. I think, ideally, the seh scope should not be generated, if the body of the scope is empty. However, I don't think it's that easy to do. I also checked the AST. It seems that no body is attached to the destructor declaration. However, I'm not very familiar with the clang frontend, so it might take a while for me to understand what's going on there. As the destructor exists in source, I assume it also generates a body for it. However it probably knows the destructor is not needed and thus discards the body at some point. I have not been able to confirm that yet, though. In the end, threre are multiple ways to fix this:
Option 2 technically sounds like the cleanest, however I'm not sure if I have enough knowledge to implement that. I guess I'll further look into the AST generation to try to understand what's the deal with the destructor here. |
|
I'd prefer not to create the problem at the beginning. I noticed the function body has |
That's a good idea. I will try to implement that tomorrow |
That would be because it's not considered "used", and therefore isn't instantiated.
Sema doesn't reason about that sort of thing. My best guess is that that MarkBaseAndMemberDestructorsReferenced calls MarkFunctionReferenced, but somehow it's not considered an odr-use. |
That seems like a complicated optimization. And I'm not sure it really fixes the underlying issue. |
Yes, that's exactly what's happening. I adjusted
However, that does not feel like a proper fix to me and I'm certain this causes way more instantiations than necessary. But I lack the knowledge to confirm that.
I'm not sure if it's that complicated. I think I almost got it. There's only an assertion throwing somewhere and I need to figure out why. I'll try to finish this fix and then both of you can judge wether it's good or not 😂 I think, to really be able to judge wether this is a proper fix or not, it would be good to have a reduced sample that triggers this issue without all the bloat std::variant brings. However, I have not managed to reduce the sample yet. |
|
I created a PR that stops generating SEH scopes for noexcept functions. It fixes the sample @StephanTLavavej provided. template <class...> struct VS {};
template <class _First, class... _Rest> struct VS<_First, _Rest...> {
union {
VS<_Rest...> _Tail;
};
~VS() { /* empty */ }
VS(long) {};
VS(short) : _Tail(long{}) {}
};
int main() { VS<int, int>(short{}); }Ontop of that, I'm not sure if, in general, noexcept functions should not have SEH scopes, as I'm sure noexcept does not account for potential memory corruptions that one might want to catch using EHa. It seems that not even the |
I think the concern here makes sense. According to LangRef, the safe way should be to check if there's a C++ object in the scope and the object must have a non-trivial destructor. |
I think it's already done this way, isn't it? We have an object in scope and the object has a non-trivial destructor. Namely the one that has no definition. From what I can see, SEH scopes are always generated. Wether necessary or not. As eli mentioned, Sema does not reason about that, which explains why they are generated. What's interesting is that a lot of SEH scopes are usesless in the initial sample. Not only the one causing problems. Even with optimizations enabled, these scopes do not get removed:
Would anyone be opposed to me implementing cleanup logic somewhere (maybe at the end of the optimization or in the backend) to just get rid of those unnecessary scopes? I think this might also be a fix/workaround for this issue. Unless there is a way the destructor is not instantiated in a case where SEH scopes are needed. However, It's hard for me to judge if this can occur. |
|
Ok, I have a sample that triggers this bug, even without /EHa, so while cleaning up SEH scopes might be good, it is not a fix for this bug. void somefunc();
template <class...> struct VS {};
template <class _First, class... _Rest> struct VS<_First, _Rest...> {
union {
VS<_Rest...> _Tail;
};
~VS() { /* empty */ }
VS(long) {};
VS(short) : _Tail(long{}) { somefunc(); }
};
int main() { VS<int, int>(short{}); }somefunc may raise a regular exception and thus creates the need for the destructor instantiation, even without EHa.
I guess this makes this purely a frontend issue. I'm not sure if I have the expertise to fix this. |
|
Oh, I mistook non-trivial destructor with empty defination. So, I agree in the /EHa case, we should remove empty scopes given no exception can be triggered by empty. But I want to make sure if it is empty at the time of generating the scope intrinsics or optimizated later. If it is the former, we should not generate it at the beginning, if it is the latter, we should disable optimizaitons within the scope, because there might be some memory operations moved out/eliminated, which is not expected with /EHa. |
Good point. I didn't check that. However, I have a potential fix. I'm not sure if it is valid. I'm essentially marking union members as referenced:
|
|
It seems that my PR breaks tests. The destructor is instantiated in too many cases. Seems like checking wether the constructor may throw (or if EHa is enabled) is required. GCC does this correctly and only instantiates the destructor when the constructor actually throws. I can try to have a look at how that could be done tomorrw, but I'm afraid I lack the frontend knowledge to achieve that on my own. I've seen @zygoloid's name pop up at a few places that were related to unions and destructor instantiation. Maybe you can provide more input :D |
|
Maybe something is forgetting to push an ExpressionEvaluationContext? With the right context, isOdrUseContext should return OdrUseContext::Used, so MarkFunctionReferenced should do the right thing, I think. |
In the reduced sample, MarkFunctionReferenced is not called for the destructor in question. |
|
Okay, I see what's happening. CodeGenFunction::EmitInitializerForField emits, for each initialized field, a cleanup for that field. Sema::SetCtorInitializers, on the other hand, just calls MarkBaseAndMemberDestructorsReferenced, which iterates over the bases/fields of the class. Which is the same in most cases, but not for an anonymous struct/union: MarkBaseAndMemberDestructorsReferenced can't look inside to find the actual field that's getting initialized. Sema::SetCtorInitializers needs to iterate over the baseOrMemberInitializers instead of the CXXRecordDecl. |
|
Cleaned up testcase; should produce an error, but no error with clang: |
Thanks for the hint. I did that and this seems to be almost it. A few tests are still failing. And I think the reason is because the destructors must be instantiated only if the constructor body may throw, or if EHa is enabled. I don't know how to determine if the body may throw. Is that even possible at this point? Has the body already been generated when Or is that even necessary? The sample you provided, @efriedma-quic, does not throw and still needs destructor instantiation. So maybe the code is right and the tests need to be adjusted? |
|
I think I figured it out. The existing tests are all passing, while all the samples from here are fixed. I will cleanup my PR and add a test case, then you can destroy me 😂 |
Initializing fields, that are part of an anonymous union, in a constructor, requires their destructors to be instantiated. In general, initialized members within non-delegating constructors, need their destructor instantiated. This fixes #93251
Initializing fields, that are part of an anonymous union, in a constructor, requires their destructors to be instantiated. In general, initialized members within non-delegating constructors, need their destructor instantiated. This fixes llvm#93251
Repros with Clang 17.0.3 and VS 2022 17.11 Preview 1. MSVC accepts, but Clang rejects.
Reduced from the original user report DevCom-10647850 "Linking bug of
std::unordered_mapwithstd::variantValue Type with latest MS Visual Studio Community 2022".The text was updated successfully, but these errors were encountered: