Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Potential ODR violation linking C++14 with C++17+ TUs that use container emplace functions #4962

Open
CaseyCarter opened this issue Sep 17, 2024 · 3 comments · May be fixed by #4963
Open

Potential ODR violation linking C++14 with C++17+ TUs that use container emplace functions #4962

CaseyCarter opened this issue Sep 17, 2024 · 3 comments · May be fixed by #4963
Labels
bug Something isn't working

Comments

@CaseyCarter
Copy link
Member

The emplace_front and emplace_back members of our containers and emplace members of our container adaptors use a "tricky" technique to implement the C++17 change that had these functions return a reference to the newly emplaced element. We changed the declare return types from void to decltype(auto), and ended each function with e.g.:

#if _HAS_CXX17
    return back();
#endif

This approach has the nice property that the functions continue to return void in C++14 mode as they did before the change.

However, the mangled name of the functions is the same in both language modes (credit to STL):

C:\Temp>type meow.cpp

#include <vector>

decltype(auto) meow(std::vector<int>& v) {
    return v.emplace_back(1729);
}

C:\Temp>cl /EHsc /nologo /W4 /std:c++14 /c meow.cpp
meow.cpp

C:\Temp>dumpbin /symbols meow.obj | rg emplace_back
0AC 00000000 SECT19 notype ()    External     | ??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAA?A_T$$QEAH@Z (public: decltype(auto) __cdecl std::vector<int,class std::allocator<int> >::emplace_back<int>(int &&))
196 00000000 SECT73 notype       Static       | $unwind$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAA?A_T$$QEAH@Z
199 00000000 SECT74 notype       Static       | $pdata$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAA?A_T$$QEAH@Z

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c meow.cpp
meow.cpp

C:\Temp>dumpbin /symbols meow.obj | rg emplace_back
0B0 00000000 SECT1A notype ()    External     | ??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAA?A_T$$QEAH@Z (public: decltype(auto) __cdecl std::vector<int,class std::allocator<int> >::emplace_back<int>(int &&))
19C 00000000 SECT74 notype       Static       | $unwind$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAA?A_T$$QEAH@Z
19F 00000000 SECT75 notype       Static       | $pdata$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAA?A_T$$QEAH@Z

potentially creating an ODR violation when linking C++14 and C++17 TUs that instantiate the same emplace function. If a C++17 TU uses the returned reference, but the linker chooses the instantiation from a C++14 TU that returns no such reference, terrible things will happen.
@CaseyCarter CaseyCarter added bug Something isn't working decision needed We need to choose something before working on this labels Sep 17, 2024
@CaseyCarter
Copy link
Member Author

IIRC our motivation for conditionally making the change was to avoid any possible breakage of C++14 code in the wild. Given the potential for ODR violations, I now think it would be preferable to make the change unconditional and deal with any breakage. I'll mark this "decision needed" so we can determine if the other maintainers agree. Feel free to comment here.

@frederick-vs-ja
Copy link
Contributor

Looks like that it's too eager to use decltype(auto) to reduce conditional compilation, and it will be fine to use different concrete return types in C++14/17.

@CaseyCarter
Copy link
Member Author

Looks like that it's too eager to use decltype(auto) to reduce conditional compilation, and it will be fine to use different concrete return types in C++14/17.

Yes, it looks like #4963's concrete return types address the ODR problem since we encode return types in mangled member names:

C:\tmp>cl /EHsc /nologo /W4 /std:c++17 /c /I c:\stl\stl\inc repro.cpp && dumpbin/symbols repro.obj | findstr emplace
repro.cpp
0AC 00000000 SECT19 notype ()    External     | ??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAAAEAH$$QEAH@Z (public: int & __cdecl std::vector<int,class std::allocator<int> >::emplace_back<int>(int &&))
196 00000000 SECT73 notype       Static       | $unwind$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAAAEAH$$QEAH@Z
199 00000000 SECT74 notype       Static       | $pdata$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAAAEAH$$QEAH@Z

C:\tmp>cl /EHsc /nologo /W4 /std:c++14 /c /I c:\stl\stl\inc repro.cpp && dumpbin/symbols repro.obj | findstr emplace
repro.cpp
0AC 00000000 SECT19 notype ()    External     | ??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAAX$$QEAH@Z (public: void __cdecl std::vector<int,class std::allocator<int> >::emplace_back<int>(int &&))
196 00000000 SECT73 notype       Static       | $unwind$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAAX$$QEAH@Z
199 00000000 SECT74 notype       Static       | $pdata$??$emplace_back@H@?$vector@HV?$allocator@H@std@@@std@@QEAAX$$QEAH@Z

There's no need to risk breaking (very weird) C++14 code.

@CaseyCarter CaseyCarter removed the decision needed We need to choose something before working on this label Sep 18, 2024
@CaseyCarter CaseyCarter linked a pull request Sep 18, 2024 that will close this issue
Don't deduce the return type of emplace{,_back,_front} #4963
Open
@CaseyCarter CaseyCarter linked a pull request Sep 18, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't deduce the return type of emplace{,_back,_front} CaseyCarter/STL
2 participants
@CaseyCarter @frederick-vs-ja