Skip to content

[LLDB] Avoid crashes when inspecting MSVC STL types #140761

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

Merged
merged 1 commit into from
May 30, 2025
Merged

[LLDB] Avoid crashes when inspecting MSVC STL types #140761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lldb/source/Plugins/Language/CPlusPlus/GenericOptional.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,10 @@ lldb::ChildCacheState GenericOptionalFrontend::Update() {

if (m_stdlib == StdLib::LibCxx)
engaged_sp = m_backend.GetChildMemberWithName("__engaged_");
else if (m_stdlib == StdLib::LibStdcpp)
engaged_sp = m_backend.GetChildMemberWithName("_M_payload")
->GetChildMemberWithName("_M_engaged");
else if (m_stdlib == StdLib::LibStdcpp) {
if (ValueObjectSP payload = m_backend.GetChildMemberWithName("_M_payload"))
engaged_sp = payload->GetChildMemberWithName("_M_engaged");
}

if (!engaged_sp)
return lldb::ChildCacheState::eRefetch;
Expand Down
3 changes: 3 additions & 0 deletions lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,9 @@ LibStdcppSharedPtrSyntheticFrontEnd::CalculateNumChildren() {

lldb::ValueObjectSP
LibStdcppSharedPtrSyntheticFrontEnd::GetChildAtIndex(uint32_t idx) {
if (!m_ptr_obj)
return nullptr;

if (idx == 0)
return m_ptr_obj->GetSP();
if (idx == 1) {
Expand Down
56 changes: 56 additions & 0 deletions lldb/test/Shell/Process/Windows/msstl_smoke.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// This smoke test ensures that LLDB doesn't crash when formatting types from MSVC's STL.
// FIXME: LLDB currently has no built-in formatters for MSVC's STL (#24834)

// REQUIRES: target-windows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here to explain why this test exists? I.e., that this just ensures that debugging with MSVC STL doesn't crash, etc.

// RUN: %build --compiler=clang-cl -o %t.exe --std c++20 -- %s
// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "run" -o "fr v" -o c | FileCheck %s

#include <bitset>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not running the data-formatter tests on Windows at all? There's definitely tests for all of these elsewhere in the API test-suite

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any specific tests? Ideally ones that test optional and shared_ptr. I can look at them later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests in lldb/test/API/functionalities/data-formatter/data-formatter-stl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these tests are unsupported due to being in the libc++/libstdc++ categories which aren't supported on Windows ("Don't know how to build with libc++ on windows"). The only tests that run are libstdcpp/unique_ptr/invalid and libcxx-simulators/*.

Copy link
Member

@Michael137 Michael137 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm right that makes sense. Should we then have a Windows data-formatter subdirectory in lldb/test/API/functionalities/data-formatter/data-formatter-stl? I think we should ideally have two test configurations:

  1. Tests that ensure we at least don't crash when using the MSVC STL. E.g., at least for std::optional and std::shared_ptr. This would test your current PR
  2. Tests for libc++ on Windows (which AFAIU, isn't the recommended configuration on Windows but is definitely used in the wild)

Not sure how easy it is to set up (2). That shouldn't block this PR, but would be nice to get around to it.

But (1) should be doable. Though not sure how equipped the API test infrastructure is to compile against MSVC STL. Worst case we could go for Shell tests like you did here

CC @DavidSpickett @labath @omjavaid for opinions on Windows testing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I wonder if that was leftover documentation from when the data-formatters were all written in python? But yea, now they're all in the cplusplus category IIUC

Copy link
Member

@Michael137 Michael137 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup looks like it is. At least when browsing back to:

commit 78453eed1219b83dff87d34a4bbbe48cc65eda70 (HEAD)
Author: Enrico Granata <[email protected]>
Date:   Thu May 3 01:08:44 2012 +0000

    Adding a new 'type category disable *' feature that disables all categories - This is intended as a quick kill switch for data formatters for cases where the user wants as little extra processing as possible to be done on their target, or to override major data formatters bug, should they occur
    
    llvm-svn: 156044

And searching the repo for gnu-libstdc++, you'll see that the libcxx and lisbtdcpp formatters were written in python and were added to separate categories. Not 100% why they got combined back into the same category. Presumably the idea was that you never want to have both categories enabled anyway, so if a cplusplus category can just turn on/off the STL formatters for you wholesale, that'd be more intuitive.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume all API tests on Windows are compiling against MSVC STL when compiled on Windows (it has nothing else to use). If you're wondering whether there's a good way to detect that is the case, yeah, that might be tricky.

For number 2, I'm not into the idea of changing the existing bot to build everything against libcxx, but if we can enable the runtime and then libcxx specific tests will be run as well, that's fine. Windows is supported and tested for libcxx as long as clang/clang-cl is the compiler, which we use already on the bot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional context David. I'm not super familiar with the Windows test bot setup

Not sure what the best testing strategy here is then. Since the STL formatters in LLDB currently don't support MSVC STL, and we just want to make sure they don't crash, maybe we could even omit tests for this PR until we get proper support for it? Though I don't mind the smoke test added here either tbh, maybe with a FIXME? Just diverges a bit from the rest of our formatter testing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this smoke test ensures they don't crash horribly, which I think it does, we should accept it. If we have issue(s) talking about MSVC formatter support link to them, or if we don't have an overall one, open one and link to that.

If this test fails way too often and it's holding up formatter development elsewhere, then we can revisit it of course.

#include <coroutine>
#include <deque>
#include <forward_list>
#include <list>
#include <map>
#include <memory>
#include <optional>
#include <set>
#include <string>
#include <tuple>
#include <unordered_map>
#include <unordered_set>
#include <variant>
#include <vector>

int main() {
std::shared_ptr<int> foo;
std::weak_ptr<int> weak = foo;
std::unique_ptr<int> unique(new int(42));
std::optional<int> opt;
std::string str = "str";
std::string longStr = "string that is long enough such that no SSO can happen";
std::wstring wStr = L"wstr";
std::wstring longWStr = L"string that is long enough such that no SSO can happen";
std::tuple<int, bool, float> tuple{1, false, 4.2};
std::coroutine_handle<> coroHandle;
std::bitset<16> bitset(123);

std::map<int, int> map{{1, 2}, {2, 4}, {3, 6}, {4, 8}, {5, 10}};
auto mapIt = map.find(3);
auto mapItEnd = map.find(9);
std::set<int> set{1, 2, 3};
std::multimap<int, int> mMap{{1, 2}, {1, 1}, {2, 4}};
std::multiset<int> mSet{1, 2, 3};

std::variant<int, float, std::string, std::monostate> variant;
std::list<int> list{1, 2, 3};
std::forward_list<int> fwList{1, 2, 3};

std::unordered_map<int, int> uMap{{1, 2}, {2, 4}, {3, 6}};
std::unordered_set<int> uSet{1, 2, 4};
std::unordered_multimap<int, int> uMMap{{1, 2}, {1, 1}, {2, 4}};
std::unordered_multiset<int> uMSet{1, 1, 2};
std::deque<int> deque{1, 2, 3};
std::vector<int> vec{1, 2, 3};
}

// CHECK: Process {{.*}} exited with status = 0 (0x00000000)
Loading