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
missing comdats for static initializers in template and guard variables #23846
Comments
The comdat isn't missing, the guard variable is just associated with the global variable being guarded. This is supposed to do the right thing, and is consistent with what we do on Linux. BFD ld might be doing the wrong thing, but MSVC's linker also agrees that something is wrong: $ clang++ l1.cpp --target=x86_64-windows-gnu -o l1.obj -c I'm going to try reordering the sections by hacking the assembly, and we'll see what it says. |
Yeah, after rearranging things in the .s files to bypass that link.exe error, we have this problem: The __main error is probably just a missing mingw64 library, but the other issue is real. |
This code is heavily reduced boost/math/special_functions/lanczos.hpp. Thanks for looking into it! |
The issue seems to resolve around ItaniumCXXABI::EmitGuardedInit
when the first branch is taken, we have multiple definition error with ld and LNK1243 error with link.exe. If we force the second branch (obviously this is not the correct conditional),
both ld and link.exe link the resulting objects without errors. |
Fundamentally, this is a failure to understand the limitations of comdats on our part. I'd really like to understand the difference in semantics between ELF and COFF here before we close this. In the meantime, we can disable that condition for COFF. |
Rui, are we doing something wrong for associative comdats in here? |
Sounds like we should disable the optimization and fix the boost build? |
Looks like clang-cl++ makes As to LLD, we don't add symbols in associative sections to the symbol table, so such symbols will never conflict even if they are external. We do that because I think that's the right thing to do. But this seems to be different from what MSVC linker does -- it looks like they just insert all symbols to the symbol table as long as they are external. That describes why LLD does not report an error for this test case. I think we should modify LLD to make it compatible with MSVC. |
Not adding the guard variable to the symbol table in this case would give the wrong result, so I think we need to remove this optimization. It seems pretty clear to me now that the MSVC linking algorithm resolves all external symbols up front without considering comdat associativity, and then brings some extra data along for the ride if your associated comdat was chosen. We can't put two external symbols in the same group in this model. It's not clear to me if it's working properly for ELF either, we might just be getting lucky. If the linker changes to be more strict, our object files might become unlinkable. |
As of r245812 this works. |
I don't see any change that would have fixed this, though. Are we sure everything works correctly? |
You are right, I tested on a local patched copy that disables on COFF as dicussed earlier:
Thanks for noticing! |
Will this patch by any chance make it into Clang 3.7? |
Do you think this patch is the right way to go? |
Sure. I'm concerned that this optimization doesn't work on ELF either, though, and that maybe we shouldn't be doing it in the first place. |
Putting multiple global symbols in a comdat? That is fine on ELF. You just have to be sure that the same symbols are added in every TU. |
GCC doesn't put the guard variable into the comdat of the global variable. Isn't that a problem? |
The itanium c++ abi says Some objects with static storage duration have associated guard variables used to ensure that they are initialized only once (see 3.3.2). If the object is emitted using a COMDAT group, the guard variable must be too. It is suggested that it be emitted in the same COMDAT group as the associated data object, but it may be emitted in its own COMDAT group, identified by its name. In either case, it must be weak. I remember discussing with someone (Richard maybe?) why the suggestion was not ABI breaking, but I can't remember right now :-( |
I've convinced myself it works for ELF because compilers have always defined both the global and the guard variable in the same TU. You only end up in the bad situation where you get two copies of a comdat member when one TU provides some of the external symbols of a comdat group and not the rest. With that in mind, I think the right fix is to make this whole optimization conditional on isObjFormatELF() instead of !COFF and !MachO. :) |
r246803 |
Extended Description
compiling the attached sources on Windows x64 using
clang++ l1.cpp l2.cpp -target i686-pc-windows-gnu
results in
multiple definition of `guard variable for lanczos_initializer::initializer'
where as g++ is ok:
g++ l1.cpp l2.cpp
the object files shows comdats with g++ but not with clang:
The text was updated successfully, but these errors were encountered: