[lldb] Only get child if m_storage and m_element_type is valid

[da-viper]

This causes a crash because lldb-dap will check the first child to see if it is array like to lazy load the children.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

This causes a crash because lldb-dap will check the first child to see if it is array like to lazy load the children.

---

Full diff: https://github.com/llvm/llvm-project/pull/163077.diff

1 Files Affected:

• (modified) lldb/source/Plugins/Language/CPlusPlus/MsvcStlAtomic.cpp (+1-1)

diff --git a/lldb/source/Plugins/Language/CPlusPlus/MsvcStlAtomic.cpp b/lldb/source/Plugins/Language/CPlusPlus/MsvcStlAtomic.cpp
index 3ec324577ac76..372b6c7afdd78 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/MsvcStlAtomic.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/MsvcStlAtomic.cpp
@@ -50,7 +50,7 @@ llvm::Expected<uint32_t> lldb_private::formatters::
 lldb::ValueObjectSP
 lldb_private::formatters::MsvcStlAtomicSyntheticFrontEnd::GetChildAtIndex(
     uint32_t idx) {
-  if (idx == 0)
+  if (idx == 0 && m_storage && m_element_type.IsValid())
     return m_storage->Cast(m_element_type)->Clone(ConstString("Value"));
   return nullptr;
 }

[da-viper]

Not sure the best way to test this, As it only happens because MSVC std::atomic formatter is loaded on linux because of the matching regex ^std::atomic<+*>

llvm-project/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

Lines 1865 to 1868 in 2a96d19

	AddCXXSynthetic(cpp_category_sp, MsvcStlAtomicSyntheticFrontEndCreator,
	"MSVC STL std::atomic synthetic children",
	"^std::atomic<.+>$", stl_synth_flags, true);

.

maybe on MSVC the std::atomic is a typedef to an underlying type and match against that ?

[Nerixyz]

The changes to the atomic synthetic children look good to me.

Not sure the best way to test this

You could create a mock atomic like

namespace std {
template <typename T>
struct atomic {
	int foo;
};
}

[Michael137]

Not sure the best way to test this, As it only happens because MSVC std::atomic formatter is loaded on linux because of the matching regex ^std::atomic<+*>

llvm-project/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

Lines 1865 to 1868 in 2a96d19

	AddCXXSynthetic(cpp_category_sp, MsvcStlAtomicSyntheticFrontEndCreator,
	"MSVC STL std::atomic synthetic children",
	"^std::atomic<.+>$", stl_synth_flags, true);

.
maybe on MSVC the std::atomic is a typedef to an underlying type and match against that ?

By "loaded" do you mean it's actually trying to use it? That seems wrong. I thought the formatters have a callback to check that we are on MSVC vs. Linux. Maybe that check isn't working?

[Michael137]

Ah we don't have such a callback because libstdc++ doesn't have a std::atomic formatter

[Michael137]

Maybe the MSVC formatters (even ones that don't have a libstdc++ counterpart) should be check if the type is an actually valid MSVC type. Like we do for the common STL formatters. @Nerixyz ?

[Nerixyz]

Maybe the MSVC formatters (even ones that don't have a libstdc++ counterpart) should be check if the type is an actually valid MSVC type. Like we do for the common STL formatters. @Nerixyz ?

They should. This patch is still good to avoid crashes with other STLs and potential changes to MS' one. Should the MSVC check happen in this PR or in another one?

[Michael137]

Maybe the MSVC formatters (even ones that don't have a libstdc++ counterpart) should be check if the type is an actually valid MSVC type. Like we do for the common STL formatters. @Nerixyz ?

They should. This patch is still good to avoid crashes with other STLs and potential changes to MS' one. Should the MSVC check happen in this PR or in another one?

Separate PR would make sense!

[da-viper]

It seems #163176 covers the test case.

[Michael137]

It seems #163176 covers the test case.

Technically it doesnt. This patch adds a null check for the case where we picked the MSVC formatter but the layout wasnt the one that the MSVC formatter expected. A test-case for this would look like the simulator test that @Nerixyz added in the patch you linked. But we would need a dummy atomic type that has a _Storage field (so the msvc formatter is picked) but had an invalid type template argument (e.g., an integral non-type template parameter).

Not saying such a test is all that interesting since MSVC STL is unlikely to change in that way and the null check is good to have anyway. But it would've been good to wait for another round of reviews/approvals before merging