draft: [lldb] Upgrade ValueObject::GetData to return llvm::Expected #130516

wizardengineer · 2025-03-09T20:57:34Z

Original mentioned here: Discourse

This patch consist of changing GetData method in the ValueObject to return a llvm::Expected in order to have more meaningful error messages.

cc: @adrian-prantl

github-actions · 2025-03-09T20:59:17Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

wizardengineer · 2025-03-09T20:59:40Z

This isn't finish of course, I'm not to sure on the changes so far and wanted to know if I was going toward the right path in terms what's being change. Before making any more big changes to the code, this presumably might be a big change, considering how many places the GetData(...) is scattered.

github-actions · 2025-03-09T21:21:24Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff a7d5b3f711b7c852aa1337e329661dd36025865a 02ecd560188ba3eba7148f96ea2d478c2bc8481a --extensions h,cpp -- lldb/include/lldb/ValueObject/ValueObject.h lldb/source/API/SBValue.cpp lldb/source/Breakpoint/Watchpoint.cpp lldb/source/DataFormatters/TypeFormat.cpp lldb/source/Expression/Materializer.cpp lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp lldb/source/Plugins/ABI/LoongArch/ABISysV_loongarch.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp lldb/source/Plugins/ABI/X86/ABIMacOSX_i386.cpp lldb/source/Plugins/ABI/X86/ABISysV_i386.cpp lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp lldb/source/Plugins/Language/ObjC/Cocoa.cpp lldb/source/Plugins/Language/ObjC/NSString.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp lldb/source/ValueObject/ValueObject.cpp lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp

View the diff from clang-format here.

diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp index a23c6ccab3..1cb5575d63 100644 --- a/lldb/source/Breakpoint/Watchpoint.cpp +++ b/lldb/source/Breakpoint/Watchpoint.cpp @@ -235,7 +235,7 @@ bool Watchpoint::WatchedValueReportable(const ExecutionContext &exe_ctx) { if (!new_data_or_err) { LLDB_LOG_ERRORV(GetLog(LLDBLog::Watchpoints), new_data_or_err.takeError(), - "Failed to extract watchpoint new data: {0}"); + "Failed to extract watchpoint new data: {0}"); return true; } @@ -244,7 +244,7 @@ bool Watchpoint::WatchedValueReportable(const ExecutionContext &exe_ctx) { if (!old_data_or_err) { LLDB_LOG_ERRORV(GetLog(LLDBLog::Watchpoints), old_data_or_err.takeError(), - "Failed to extract watchpoint old data: {0}"); + "Failed to extract watchpoint old data: {0}"); return true; } diff --git a/lldb/source/Expression/Materializer.cpp b/lldb/source/Expression/Materializer.cpp index 31babfebb7..db7f53a53f 100644 --- a/lldb/source/Expression/Materializer.cpp +++ b/lldb/source/Expression/Materializer.cpp @@ -579,8 +579,8 @@ public: m_temporary_allocation_size = data.GetByteSize(); - m_original_data = std::make_shared<DataBufferHeap>( - data.GetDataStart(), data.GetByteSize()); + m_original_data = std::make_shared<DataBufferHeap>(data.GetDataStart(), + data.GetByteSize()); if (!alloc_error.Success()) { err = Status::FromErrorStringWithFormat( diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp index c6a3b40afb..988b825f94 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp @@ -277,9 +277,9 @@ ValueObjectSP ForwardListFrontEnd::GetChildAtIndex(uint32_t idx) { if (!data) return nullptr; - return CreateValueObjectFromData( - llvm::formatv("[{0}]", idx).str(), *data, - m_backend.GetExecutionContextRef(), m_element_type); + return CreateValueObjectFromData(llvm::formatv("[{0}]", idx).str(), *data, + m_backend.GetExecutionContextRef(), + m_element_type); } lldb::ChildCacheState ForwardListFrontEnd::Update() { diff --git a/lldb/source/Plugins/Language/ObjC/Cocoa.cpp b/lldb/source/Plugins/Language/ObjC/Cocoa.cpp index 376a290544..70ce306782 100644 --- a/lldb/source/Plugins/Language/ObjC/Cocoa.cpp +++ b/lldb/source/Plugins/Language/ObjC/Cocoa.cpp @@ -1209,8 +1209,8 @@ bool lldb_private::formatters::ObjCSELSummaryProvider( if (!data) return false; - valobj_sp = ValueObject::CreateValueObjectFromData("text", *data, - exe_ctx, charstar); + valobj_sp = ValueObject::CreateValueObjectFromData("text", *data, exe_ctx, + charstar); } if (!valobj_sp) diff --git a/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp b/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp index c725b6d882..519ed175a2 100644 --- a/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp +++ b/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp @@ -88,7 +88,7 @@ public: var_name, data_extractor); if (llvm::Error error = valobj_sp->Dump(strm, options)) llvm::consumeError(std::move(error)); -  + ASSERT_STREQ(strm.GetString().str().c_str(), expected); strm.Clear(); }

lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

wizardengineer · 2025-03-11T02:16:59Z

lldb/source/DataFormatters/TypeFormat.cpp

 ExecutionContext exe_ctx(valobj->GetExecutionContextRef());
- DataExtractor data;

+ auto data_or_err = valobj->GetData();


I think this is a practical approach?

Yeah, it seems to be fine. I don't think there should be any side-effects from this refactor.

wizardengineer · 2025-03-11T02:24:26Z

I added another wave of refactors, I'll add more changes, once this current wave(commit) is approved. And I'll continue to incrementally add changes to make it easier to manage.

wizardengineer · 2025-03-15T12:22:06Z

lldb/source/Expression/Materializer.cpp

 "couldn't read contents of reference variable %s: %s",
- GetName().AsCString(), extract_error.AsCString());
+ GetName().AsCString(), llvm::toString(std::move(error)).c_str());
 return;


shouldn't we just do this from now on?

Suggested change

return;

return Status::FromErrorStringWithFormat(

"couldn't read contents of reference variable %s: %s",

GetName().AsCString(), llvm::toString(std::move(error)).c_str());

Eventually, yes. You just have to draw the line somewhere for each commit.

wizardengineer · 2025-03-15T14:33:59Z

Hey @adrian-prantl, if you can and when you have time. I was wondering if it'd be okay if you can review this and just so i can make sure it aligns with the standards for this PR, before submitting the final results.

I was also curious to know if I'd have to change any test code too, I've tried to unit test but got error relating to "expected" values. Would I need to change something in DumpValueObjectOptionsTests.cpp?

Error output for `ninja check-lldb-unit`

******************** FAIL: lldb-unit :: ValueObject/./LLDBValueObjectTests/0/11 (252 of 275) ******************** TEST 'lldb-unit :: ValueObject/./LLDBValueObjectTests/0/11' FAILED ******************** Script(shard): -- GTEST_OUTPUT=json:/Users/juliusalexandre/GitDownloads/llvm-project/build/tools/lldb/unittests/ValueObject/./LLDBValueObjectTests-lldb-unit-37800-0-11.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=11 GTEST_SHARD_INDEX=0 /Users/juliusalexandre/GitDownloads/llvm-project/build/tools/lldb/unittests/ValueObject/./LLDBValueObjectTests -- Script: -- /Users/juliusalexandre/GitDownloads/llvm-project/build/tools/lldb/unittests/ValueObject/./LLDBValueObjectTests --gtest_filter=ValueObjectMockProcessTest.EmptyEnum -- /Users/juliusalexandre/GitDownloads/llvm-project/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp:91: Failure Expected equality of these values: strm.GetString().str().c_str() Which is: "(TestEnum) test_var =\n" expected Which is: "(TestEnum) test_var = 0\n" /Users/juliusalexandre/GitDownloads/llvm-project/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp:91: Failure Expected equality of these values: strm.GetString().str().c_str() Which is: "(TestEnum) test_var =\n" expected Which is: "(TestEnum) test_var = -2\n" /Users/juliusalexandre/GitDownloads/llvm-project/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp:91Expected equality of these values: strm.GetString().str().c_str() Which is: "(TestEnum) test_var =\n" expected Which is: "(TestEnum) test_var = 0\n" /Users/juliusalexandre/GitDownloads/llvm-project/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp:91 Expected equality of these values: strm.GetString().str().c_str() Which is: "(TestEnum) test_var =\n" expected Which is: "(TestEnum) test_var = -2\n"

adrian-prantl

Looks like a great start! I made a couple of suggestions inline.

lldb/source/Expression/Materializer.cpp

lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

lldb/source/Plugins/Language/ObjC/Cocoa.cpp

lldb/source/Plugins/Language/ObjC/NSString.cpp

adrian-prantl · 2025-03-17T21:26:10Z

I was also curious to know if I'd have to change any test code too, I've tried to unit test but got error relating to "expected" values. Would I need to change something in DumpValueObjectOptionsTests.cpp?

Usually wrapping calls in llvm::expectedToOptional() is the cheapest way to just adapt the API in places where the error handling doesn't matter. You could even write a wrapper for that test that creates a Status from the error and imitates the old API, just inside the unit tests.

wizardengineer · 2025-03-25T21:35:29Z

This is taking longer than expected, I'm encountering a lot of linker bugs unrelated the patches I've made. Even after create a fresh clone of llvm without the patches. I'm still encountering issues.

One them being #132845 (comment)

wizardengineer · 2025-03-26T20:18:49Z

You could even write a wrapper for that test that creates a Status from the error and imitates the old API, just inside the unit tests.

how would i go about doing that?

wizardengineer · 2025-03-27T01:19:40Z

lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp

 DataExtractor data_extractor{&value, sizeof(value), endian, 4};
 auto valobj_sp = ValueObjectConstResult::Create(exe_scope, enum_type,
 var_name, data_extractor);
 if (llvm::Error error = valobj_sp->Dump(strm, options))


Hey @adrian-prantl,

I didn't make any changes to the unit tests yet. I was a bit confuse on how I'd go about writing a wrapper for this test to imitates the old API.

The old function signature was

uint64_t GetData(DataExtractor &data, Status &error);

the new one is

llvm::Expected<DataExtractor> GetData();

so you can do something like

uint64_t GetDataWithStatus(DataExtractor &data, ValueObject &valobj, Status &error) { auto data_extractor_or_err = valobj.GetData(); if (data_extractor_or_err) { data = *data_extractor_or_err; return data.GetSize(); } error = Status::FromError(data_extractor_or_err.takeError()); return 0; }

wizardengineer · 2025-03-27T13:55:27Z

lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp

+ auto data_or_err = new_value_sp->GetData();
+ if (auto err = data_or_err.takeError())
+ return Status::FromError(llvm::joinErrors(
+ llvm::createStringError("Couldn't convert return value to raw data"),


I think it should be this instead?

Suggested change

llvm::createStringError("Couldn't convert return value to raw data"),

llvm::createStringError("Couldn't convert return value to raw data: "),

wizardengineer added 2 commits March 9, 2025 16:20

Change ValueObject::GetData to return llvm::Expected

161bdb3

clang format

7d886e2

more changes

c6d271b

format

ac8dacc

adrian-prantl reviewed Mar 10, 2025

View reviewed changes

lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp Outdated Show resolved Hide resolved

adrian-prantl reviewed Mar 10, 2025

View reviewed changes

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp Show resolved Hide resolved

wizardengineer added 2 commits March 10, 2025 21:54

third wave of refactoring

dc92635

third wave: clean up

69da662

wizardengineer commented Mar 11, 2025

View reviewed changes

third wave: more clean up

2a3d589

wizardengineer requested a review from adrian-prantl March 15, 2025 12:19

wizardengineer commented Mar 15, 2025

View reviewed changes

adrian-prantl reviewed Mar 17, 2025

View reviewed changes

wizardengineer added 3 commits March 26, 2025 20:43

third wave: fixed the readablility

c3ba85d

third wave: fixed format

613dc2a

forth wave: ABI related code refactoring

ae2276f

wizardengineer commented Mar 27, 2025

View reviewed changes

wizardengineer added 5 commits March 26, 2025 21:22

forth wave: fix format

058a503

forth wave: fix more formats

83dcfb0

fix error message

ea4f3c3

error message fix

48e9535

s390x ABI error message fix

437917b

wizardengineer added 7 commits March 26, 2025 22:27

more fix error messaging and format

9980b08

fixed logic in TypeFormat.cpp

c075196

fixed more logic

8fcb205

fixed more logic

5f58a8b

made changes to type system and apple obj imp

eda2415

simple fix RT impl

7a1c4be

fixed error

02ecd56

wizardengineer commented Mar 27, 2025

View reviewed changes

wizardengineer requested a review from adrian-prantl March 27, 2025 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

draft: [lldb] Upgrade ValueObject::GetData to return llvm::Expected #130516

draft: [lldb] Upgrade ValueObject::GetData to return llvm::Expected #130516

Uh oh!

wizardengineer commented Mar 9, 2025

github-actions bot commented Mar 9, 2025

wizardengineer commented Mar 9, 2025 •

edited

Loading

github-actions bot commented Mar 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

wizardengineer Mar 11, 2025

wizardengineer Mar 11, 2025

wizardengineer commented Mar 11, 2025

wizardengineer Mar 15, 2025 •

edited

Loading

adrian-prantl Mar 20, 2025

wizardengineer commented Mar 15, 2025 •

edited

Loading

adrian-prantl left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-prantl commented Mar 17, 2025

wizardengineer commented Mar 25, 2025

wizardengineer commented Mar 26, 2025

wizardengineer Mar 27, 2025

adrian-prantl Apr 1, 2025

wizardengineer Mar 27, 2025

Labels

2 participants

	llvm::createStringError("Couldn't convert return value to raw data"),
	llvm::createStringError("Couldn't convert return value to raw data: "),

draft: [lldb] Upgrade ValueObject::GetData to return llvm::Expected #130516

Are you sure you want to change the base?

draft: [lldb] Upgrade ValueObject::GetData to return llvm::Expected #130516

Uh oh!

Conversation

wizardengineer commented Mar 9, 2025

github-actions bot commented Mar 9, 2025

wizardengineer commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

github-actions bot commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wizardengineer Mar 11, 2025

Choose a reason for hiding this comment

wizardengineer Mar 11, 2025

Choose a reason for hiding this comment

wizardengineer commented Mar 11, 2025

wizardengineer Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

adrian-prantl Mar 20, 2025

Choose a reason for hiding this comment

wizardengineer commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-prantl commented Mar 17, 2025

wizardengineer commented Mar 25, 2025

wizardengineer commented Mar 26, 2025

wizardengineer Mar 27, 2025

Choose a reason for hiding this comment

adrian-prantl Apr 1, 2025

Choose a reason for hiding this comment

wizardengineer Mar 27, 2025

Choose a reason for hiding this comment

Labels

2 participants

wizardengineer commented Mar 9, 2025 •

edited

Loading

github-actions bot commented Mar 9, 2025 •

edited

Loading

wizardengineer Mar 15, 2025 •

edited

Loading

wizardengineer commented Mar 15, 2025 •

edited

Loading