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.

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

SSO breaks backwards compatibility of ABI #3273

Closed
RIscRIpt opened this issue Dec 7, 2022 · 7 comments
Closed

SSO breaks backwards compatibility of ABI #3273

RIscRIpt opened this issue Dec 7, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@RIscRIpt
Copy link

RIscRIpt commented Dec 7, 2022

STL shipped with MSVC 14.34 introduced SSO (1ea95df). SSO implementation has changed _STD::_String_val::_Bxty::_Bxty() constructor: previously it zero-initialized _Ptr field (in MSVC 14.33), now it initializes _Buf field (in MSVC 14.34). SSO implementation relies on the fact that _Buf is initialized with zeroes, however this assumption breaks when std::string comes from TU compiled using older STL, where _Ptr field was initialized instead. When std::string in old TU is initialized using a small (>8 and <16) string literal, std::string::c_str() would return pointer to buffer which is not zero terminated properly.

Command-line test case

Compiling TU using MSVC 14.33 without SSO

//projects/msvc-sso-14_33-34_bug/lib_1433
❯ msvc-env x64 14.33

//projects/msvc-sso-14_33-34_bug/lib_1433
❯ type .\lib.cpp
#include <string>
static_assert(_MSC_VER == 1933);
std::string GetStringOld(const char* p) {
    return std::string(p);
}

//projects/msvc-sso-14_33-34_bug/lib_1433
❯ cl /c /O2 /EHsc lib.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.33.31631 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

lib.cpp

Compiling TU using MSVC 14.34 with SSO

//projects/msvc-sso-14_33-34_bug/exe_1434
❯ msvc-env x64 14.34

//projects/msvc-sso-14_33-34_bug/exe_1434
❯ type exe.cpp
#include <iostream>
#include <string>

static_assert(_MSC_VER == 1934);

std::string GetStringOld(const char* p);

__declspec(noinline) std::string GetStringNew(const char* p) {
    return std::string(p);
}

__declspec(noinline) void dirty_stack() {
    char trash[32];
    memset(trash, '@', sizeof(trash) - 1);
    trash[sizeof(trash) - 1] = '\0';
    std::cout << trash << '\n';
}

__declspec(noinline) void nobug_new() {
    std::cout << GetStringNew("01234567#$").c_str() << '\n';
}

__declspec(noinline) void bug_old() {
    std::cout << GetStringOld("01234567#$").c_str() << '\n';
}

int main() {
    dirty_stack();
    bug_old();
    dirty_stack();
    nobug_new();
    return 0;
}

//projects/msvc-sso-14_33-34_bug/exe_1434
❯ cl /O2 /EHsc .\exe.cpp ..\lib_1433\lib.obj
Microsoft (R) C/C++ Optimizing Compiler Version 19.34.31935 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

exe.cpp
Microsoft (R) Incremental Linker Version 14.34.31935.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:exe.exe
exe.obj
..\lib_1433\lib.obj

//projects/msvc-sso-14_33-34_bug/exe_1434
❯ .\exe.exe
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
01234567#$@@@@@@

@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
01234567#$

Note: it's important to have GetStringNew, which references std::basic_string functions forcing linker to choose new std::string implementation for GetStringOld.

Expected behavior

I expect the following assertion to succeed

assert(!strcmp(GetStringOld("01234567--").c_str(), "01234567--"));

Namely GetStringOld should be able to initialize std::string in a way that std::string::c_str() would return a pointer to buffer which is zero-terminated correctly.

Additional context
This problem was discovered when I compiled a project using MSVC 14.34 re-using LLVM compiled using MSVC 14.33. The problematic part was in llvm::Twine::str().

For your convenience I created a repo with reproducible example in Visual Studio solution.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Dec 7, 2022
@StephanTLavavej
Copy link
Member

Thanks for the detailed report - this is a known bug that we've already fixed for VS 2022 17.6 Preview 1 and 17.5 Preview 3. We're currently working on backporting it to a 17.4.x patch release.

@RIscRIpt
Copy link
Author

RIscRIpt commented Dec 9, 2022

Indeed, I see it was fixed in #3235.

@CaseyCarter CaseyCarter self-assigned this Dec 9, 2022
@CaseyCarter
Copy link
Member

Indeed, I see it was fixed in #3235.

Yes, that change fixes the ABI incompatibility for VS 2022 version 17.6 when it eventually ships. We've gotten servicing approval to ship the fix in 17.5 and 17.4 as well. We tentatively expect the fix to ship in 17.5 Preview 3 and 17.4.4, but those may slip a release given how close we are to ship deadlines.

@AF9036
Copy link

AF9036 commented Jan 11, 2023

Is the fix already integrated into new release of VS 17.4.4 ?

@RIscRIpt
Copy link
Author

Is the fix already integrated into new release of VS 17.4.4 ?

No, I cannot see that b9adfa8 changes are present in 14.34.31933\include\xstring of VS 17.4.4.

I think MSFT team would notify us when the fix would get shipped. Let's be patient 🙂

@StephanTLavavej
Copy link
Member

@RIscRIpt is correct - we tried to get the fix into 17.4.4, but didn't get it in time for an internal deadline. It is now planned for 17.4.5. (It was successfully merged into 17.5 Preview 3 and should be available when that's released.)

Thanks for your patience, we know this is a bad bug.

@CaseyCarter
Copy link
Member

Update: The fix for 17.4 has been merged for 17.4.5 and will ship in that release barring some unforeseen catastrophe. Thanks for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants