Skip to content

[LLDB][Value] Require type size when reading a scalar #153386

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 2 commits into from
Aug 22, 2025
Merged

[LLDB][Value] Require type size when reading a scalar #153386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dc9c3b
[LLDB][Value] Require type size when reading a scalar
Nerixyz Aug 13, 2025
96b896b
add a unittest
Nerixyz Aug 16, 2025
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
3 changes: 3 additions & 0 deletions lldb/source/Core/Value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ Status Value::GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data,
else
data.SetAddressByteSize(sizeof(void *));

if (!type_size)
return Status::FromErrorString("type does not have a size");
Copy link
Member

@Michael137 Michael137 Aug 14, 2025

Choose a reason for hiding this comment

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

Looks like this is a regression from (#151350):

Author: Ilia Kuklin <[email protected]>
Date:   Wed Aug 6 14:32:19 2025 +0500

    [lldb] Add `ValueObject::CreateValueObjectFromScalar` and fix `Scalar::GetData` (#151350)
    
    Add `ValueObject::CreateValueObjectFromScalar` function and adjust
    `Scalar::GetData` to be able to both extend and truncate the data bytes
    in Scalar to the specified size.

Previously we would return an error if type_size was unset.

So this pretty much brings us back to previous behaviour.

Any way we can test this in lldb/unittests/Utility/ScalarTest.cpp? I guess the tricky bit will be creating a type with invalid byte_size. Maybe with an incomplete AST type? Haven't looked at what's available in that test though

Copy link
Collaborator

@labath labath Aug 14, 2025

Choose a reason for hiding this comment

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

I don't know about the rest, but testing something about the ValueObject class inside ScalarTest.cpp sounds very wrong.

Copy link
Member

@Michael137 Michael137 Aug 14, 2025

Choose a reason for hiding this comment

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

Might be missing something but I was thinking just testing Value::GetValueAsData. Which we could accomplish by creating a Scalar.

The PR I linked was fixing something for ValueObject, but did so by adjusting something in Value. Maybe that's where the confusion came from?

Copy link
Contributor Author

@Nerixyz Nerixyz Aug 14, 2025

Choose a reason for hiding this comment

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

I don't think we need an incomplete type here. A value without a compiler type should suffice. So something like

Value v(Scalar(42));
DataExtractor extractor;
Status status = v.GetValueAsData(nullptr, extractor, nullptr);
ASSERT_TRUE(status.Fail());

I could add a file in unittests/ValueObject. There are no unittests that call GetValueAsData yet.


uint32_t result_byte_size = *type_size;
if (m_value.GetData(data, result_byte_size))
return error; // Success;
Expand Down
1 change: 1 addition & 0 deletions lldb/unittests/Core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ add_lldb_unittest(LLDBCoreTests
SourceManagerTest.cpp
TelemetryTest.cpp
UniqueCStringMapTest.cpp
Value.cpp

LINK_COMPONENTS
Support
Expand Down
39 changes: 39 additions & 0 deletions lldb/unittests/Core/Value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "lldb/Core/Value.h"
#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
#include "TestingSupport/SubsystemRAII.h"
#include "TestingSupport/Symbol/ClangTestUtils.h"

#include "lldb/Utility/DataExtractor.h"

#include "gtest/gtest.h"

using namespace lldb_private;
using namespace lldb_private::clang_utils;

TEST(ValueTest, GetValueAsData) {
SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX> subsystems;
auto holder = std::make_unique<clang_utils::TypeSystemClangHolder>("test");
auto *clang = holder->GetAST();

Value v(Scalar(42));
DataExtractor extractor;

// no compiler type
Status status = v.GetValueAsData(nullptr, extractor, nullptr);
ASSERT_TRUE(status.Fail());

// with compiler type
v.SetCompilerType(clang->GetBasicType(lldb::BasicType::eBasicTypeChar));

status = v.GetValueAsData(nullptr, extractor, nullptr);
ASSERT_TRUE(status.Success());
}
Loading