https://github.com/kastiglione updated https://github.com/llvm/llvm-project/pull/212568
>From 78b429be5118a7b2ed831c8c88b3b5b7b6f7ebc3 Mon Sep 17 00:00:00 2001 From: Dave Lee <[email protected]> Date: Tue, 28 Jul 2026 10:57:59 -0700 Subject: [PATCH 1/2] [lldb][bytecode] Improve testing of bytecode section loading --- .../DataFormatters/FormatterSection.cpp | 30 +- .../DataFormatter/FormatterSectionTest.cpp | 345 ++++++++++++++++++ 2 files changed, 368 insertions(+), 7 deletions(-) diff --git a/lldb/source/DataFormatters/FormatterSection.cpp b/lldb/source/DataFormatters/FormatterSection.cpp index 7ddfdc6ec41d0..c0e495806522c 100644 --- a/lldb/source/DataFormatters/FormatterSection.cpp +++ b/lldb/source/DataFormatters/FormatterSection.cpp @@ -73,6 +73,13 @@ static void ForEachFormatterInModule( uint64_t version = section.getULEB128(cursor); uint64_t record_size = section.getULEB128(cursor); + if (cursor && record_size > section_size - cursor.tell()) { + LLDB_LOG(GetLog(LLDBLog::DataFormatters), + "Record size {0} exceeds the remaining size of the embedded " + "formatter section in {1}; ignoring the rest of the section.", + record_size, module.GetFileSpec()); + break; + } if (version == 1) { llvm::DataExtractor record( section.getData().drop_front(cursor.tell()).take_front(record_size), @@ -173,26 +180,30 @@ void LoadFormattersForModule(ModuleSP module_sp) { std::unique_ptr<llvm::MemoryBuffer> summary_func_up; std::array<std::unique_ptr<llvm::MemoryBuffer>, kSignatureCount> synthetic_methods; + bool has_synthetic_method = false; using Signatures = FormatterBytecode::Signatures; while (cursor && cursor.tell() < extractor.size()) { auto signature = static_cast<Signatures>(extractor.getU8(cursor)); uint64_t size = extractor.getULEB128(cursor); llvm::StringRef bytecode = extractor.getBytes(cursor, size); - if (!cursor) { - LLDB_LOG_ERROR(GetLog(LLDBLog::DataFormatters), cursor.takeError(), - "{0}"); + if (!cursor) break; - } auto buffer_up = llvm::MemoryBuffer::getMemBufferCopy(bytecode); if (signature == Signatures::sig_summary) summary_func_up = std::move(buffer_up); - else if (signature <= Signatures::sig_update) + else if (signature <= Signatures::sig_update) { synthetic_methods[signature] = std::move(buffer_up); - else + has_synthetic_method = true; + } else LLDB_LOG(GetLog(LLDBLog::DataFormatters), "Unsupported formatter signature {0} for '{1}' in {2}", signature, type_name, module_sp->GetFileSpec()); } + if (!cursor) { + LLDB_LOG_ERROR(GetLog(LLDBLog::DataFormatters), cursor.takeError(), + "{0}"); + return; + } FormatterMatchType match_type = eFormatterMatchExact; if (type_name.front() == '^') @@ -205,7 +216,7 @@ void LoadFormattersForModule(ModuleSP module_sp) { LLDB_LOG(GetLog(LLDBLog::DataFormatters), "Loaded embedded type summary for '{0}' from {1}.", type_name, module_sp->GetFileSpec()); - } else { + } else if (has_synthetic_method) { BytecodeSyntheticChildren::SyntheticBytecodeImplementation impl = CreateSyntheticImpl(synthetic_methods); auto synthetic_children_sp = @@ -215,6 +226,11 @@ void LoadFormattersForModule(ModuleSP module_sp) { LLDB_LOG(GetLog(LLDBLog::DataFormatters), "Loaded embedded type synthetic for '{0}' from {1}.", type_name, module_sp->GetFileSpec()); + } else { + LLDB_LOG(GetLog(LLDBLog::DataFormatters), + "No summary or synthetic methods found for '{0}' in {1}, " + "not registering a formatter.", + type_name, module_sp->GetFileSpec()); } }); } diff --git a/lldb/unittests/DataFormatter/FormatterSectionTest.cpp b/lldb/unittests/DataFormatter/FormatterSectionTest.cpp index 7da7a34d84ef9..4ee0bf8e68c66 100644 --- a/lldb/unittests/DataFormatter/FormatterSectionTest.cpp +++ b/lldb/unittests/DataFormatter/FormatterSectionTest.cpp @@ -15,17 +15,100 @@ #include "lldb/Core/Debugger.h" #include "lldb/Core/Module.h" #include "lldb/DataFormatters/DataVisualization.h" +#include "lldb/DataFormatters/FormatterBytecode.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" #include "lldb/Target/Platform.h" #include "lldb/ValueObject/ValueObjectConstResult.h" +#include "llvm/Support/LEB128.h" #include "gtest/gtest.h" +#include <optional> +#include <string> +#include <vector> using namespace lldb; using namespace lldb_private; namespace { +// --- Helpers for hand-assembling the embedded formatter/summary record +// format read by FormatterSection.cpp, so malformed inputs can be expressed +// as typed fields instead of raw hex blobs. --- + +void AppendULEB(std::vector<uint8_t> &bytes, uint64_t value) { + uint8_t buf[10]; + unsigned len = llvm::encodeULEB128(value, buf); + bytes.insert(bytes.end(), buf, buf + len); +} + +void AppendBytes(std::vector<uint8_t> &bytes, llvm::StringRef data) { + bytes.insert(bytes.end(), data.begin(), data.end()); +} + +void AppendBytes(std::vector<uint8_t> &bytes, llvm::ArrayRef<uint8_t> data) { + bytes.insert(bytes.end(), data.begin(), data.end()); +} + +// Appends one length-framed record: [version][record_size][type_size] +// [type_name][entry]. `record_size` is declared honestly as the size of +// [type_size][type_name][entry] unless a test overrides it to exercise a +// mismatched/corrupt size. +void AppendRecord(std::vector<uint8_t> §ion, uint64_t version, + llvm::StringRef type_name, + llvm::ArrayRef<uint8_t> entry, + std::optional<uint64_t> record_size_override = {}) { + std::vector<uint8_t> body; + AppendULEB(body, type_name.size()); + AppendBytes(body, type_name); + AppendBytes(body, entry); + + AppendULEB(section, version); + AppendULEB(section, record_size_override.value_or(body.size())); + AppendBytes(section, llvm::ArrayRef<uint8_t>(body)); +} + +std::string ToHex(llvm::ArrayRef<uint8_t> bytes) { + static const char digits[] = "0123456789ABCDEF"; + std::string hex; + hex.reserve(bytes.size() * 2); + for (uint8_t b : bytes) { + hex.push_back(digits[b >> 4]); + hex.push_back(digits[b & 0xF]); + } + return hex; +} + +// Builds a minimal ELF with a single section named `section_name` whose +// contents are exactly `content` (no implicit padding). +std::string BuildSectionYaml(llvm::StringRef section_name, + llvm::ArrayRef<uint8_t> content) { + return ("--- !ELF\n" + "FileHeader:\n" + " Class: ELFCLASS64\n" + " Data: ELFDATA2LSB\n" + " Type: ET_DYN\n" + " Machine: EM_X86_64\n" + "Sections:\n" + " - Name: " + + section_name.str() + + "\n" + " Type: SHT_PROGBITS\n" + " Flags: [ ]\n" + " Address: 0x2010\n" + " AddressAlign: 0x10\n" + " Content: " + + ToHex(content) + + "\n" + " Size: " + + std::to_string(content.size()) + + "\n" + "...\n"); +} + +} // namespace + +namespace { + struct MockProcess : Process { MockProcess(TargetSP target_sp, ListenerSP listener_sp) : Process(target_sp, listener_sp) {} @@ -54,6 +137,15 @@ struct MockProcess : Process { class FormatterSectionTest : public ::testing::Test { public: void SetUp() override { + // The "default" category lives in a process-wide FormatManager, so start + // each test from a clean slate regardless of what earlier tests in this + // binary registered. + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), + category); + if (category) + category->Clear(); + ArchSpec arch("x86_64-pc-linux"); Platform::SetHostPlatform( platform_linux::PlatformLinux::CreateInstance(true, &arch)); @@ -142,3 +234,256 @@ TEST_F(FormatterSectionTest, LoadFormattersForModule) { rect_summary_sp->FormatObject(valobj.get(), dest, TypeSummaryOptions())); ASSERT_EQ(dest, "BBBBB"); } + +/// A lone continuation byte (high bit set) is not a complete ULEB128 value, +/// so even the leading version number can't be decoded. This must not read +/// out of bounds or crash. +TEST_F(FormatterSectionTest, MalformedULEBAtStart) { + std::vector<uint8_t> section = {0x80}; + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 0u); +} + +/// A record whose version isn't 1 is unsupported and should be skipped over +/// (using its honestly-declared record_size) without disturbing a +/// well-formed record that follows it. +TEST_F(FormatterSectionTest, SkipsRecordWithUnsupportedVersion) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*flags=*/0); + entry.push_back(FormatterBytecode::Signatures::sig_summary); + AppendULEB(entry, /*bytecode_size=*/2); + AppendBytes(entry, llvm::ArrayRef<uint8_t>({0xAA, 0xBB})); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/2, "Bogus", entry); + AppendRecord(section, /*version=*/1, "Good", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetSummaryForType(std::make_shared<TypeNameSpecifierImpl>( + "Bogus", lldb::eFormatterMatchExact)), + nullptr); + EXPECT_NE(category->GetSummaryForType(std::make_shared<TypeNameSpecifierImpl>( + "Good", lldb::eFormatterMatchExact)), + nullptr); +} + +/// The record declares a type name of length 10, but the record itself +/// (honestly sized by the outer record_size field) only has room for 3 +/// bytes of name and nothing else. The type name read must fail cleanly +/// instead of reading past the record's bounds, and a well-formed record +/// that follows must still be reached. +TEST_F(FormatterSectionTest, InnerTypeSizeExceedsRecordBounds) { + std::vector<uint8_t> body; + AppendULEB(body, /*type_size=*/10); + AppendBytes(body, llvm::StringRef("Foo")); + + std::vector<uint8_t> section; + AppendULEB(section, /*version=*/1); + AppendULEB(section, /*record_size=*/body.size()); + AppendBytes(section, llvm::ArrayRef<uint8_t>(body)); + + std::vector<uint8_t> entry; + AppendULEB(entry, /*flags=*/0); + entry.push_back(FormatterBytecode::Signatures::sig_summary); + AppendULEB(entry, /*bytecode_size=*/2); + AppendBytes(entry, llvm::ArrayRef<uint8_t>({0xAA, 0xBB})); + AppendRecord(section, /*version=*/1, "Good", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 1u); + EXPECT_NE(category->GetSummaryForType(std::make_shared<TypeNameSpecifierImpl>( + "Good", lldb::eFormatterMatchExact)), + nullptr); +} + +/// A record_size far larger than the number of bytes actually remaining in +/// the section is an internally-inconsistent (corrupt/truncated) record: its +/// own declared size can't be trusted to locate the next record, so it must +/// be rejected rather than silently parsed from whatever bytes happen to be +/// left. A well-formed record preceding it is unaffected. +TEST_F(FormatterSectionTest, RecordSizeExceedsRemainingSectionIsRejected) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*flags=*/0); + entry.push_back(FormatterBytecode::Signatures::sig_summary); + AppendULEB(entry, /*bytecode_size=*/2); + AppendBytes(entry, llvm::ArrayRef<uint8_t>({0xAA, 0xBB})); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/1, "Good", entry); + AppendRecord(section, /*version=*/1, "Oversized", entry, + /*record_size_override=*/1000000); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 1u); + EXPECT_NE(category->GetSummaryForType(std::make_shared<TypeNameSpecifierImpl>( + "Good", lldb::eFormatterMatchExact)), + nullptr); + EXPECT_EQ(category->GetSummaryForType(std::make_shared<TypeNameSpecifierImpl>( + "Oversized", lldb::eFormatterMatchExact)), + nullptr); +} + +/// An unrecognized signature byte (with otherwise well-formed size/bytecode +/// framing) is logged and skipped without preventing a later, valid +/// signature in the same entry from being picked up. +TEST_F(FormatterSectionTest, UnsupportedSignatureByteIsSkippedWithinEntry) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*flags=*/0); + entry.push_back(0xFF); + AppendULEB(entry, /*size=*/2); + AppendBytes(entry, llvm::ArrayRef<uint8_t>({0x11, 0x22})); + entry.push_back(FormatterBytecode::Signatures::sig_summary); + AppendULEB(entry, /*size=*/2); + AppendBytes(entry, llvm::ArrayRef<uint8_t>({0xAA, 0xBB})); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/1, "Widget", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_NE(category->GetSummaryForType(std::make_shared<TypeNameSpecifierImpl>( + "Widget", lldb::eFormatterMatchExact)), + nullptr); +} + +/// The declared bytecode size (500) is far larger than the 0 bytes that +/// actually remain in the entry, so reading it fails cleanly rather than +/// reading out of bounds. Since no summary and no synthetic method was +/// successfully parsed, nothing should be registered for the type. +TEST_F(FormatterSectionTest, TruncatedBytecodeSizeAbortsEntryParsing) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*flags=*/0); + entry.push_back(FormatterBytecode::Signatures::sig_init); + AppendULEB(entry, /*size=*/500); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/1, "Broken", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 0u); + EXPECT_EQ(category->GetSyntheticForType(std::make_shared<TypeNameSpecifierImpl>( + "Broken", lldb::eFormatterMatchExact)), + nullptr); +} + +/// An entry that has flags but no summary or synthetic-method sub-entries +/// at all (valid framing, just empty) must not register a formatter either. +TEST_F(FormatterSectionTest, EmptyEntryRegistersNothing) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*flags=*/0); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/1, "Empty", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadFormattersForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 0u); +} + +/// An embedded type summary with an empty summary string is dropped instead +/// of being registered. +TEST_F(FormatterSectionTest, EmptySummaryStringIsNotRegistered) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*summary_size=*/0); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/1, "Empty", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbsummaries", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadTypeSummariesForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 0u); +} + +/// A declared summary_size larger than the bytes actually available in the +/// entry must fail cleanly instead of reading out of bounds, and the +/// summary must not be registered. +TEST_F(FormatterSectionTest, SummarySizeExceedsAvailableBytes) { + std::vector<uint8_t> entry; + AppendULEB(entry, /*summary_size=*/50); + AppendBytes(entry, llvm::StringRef("short")); + + std::vector<uint8_t> section; + AppendRecord(section, /*version=*/1, "Oops", entry); + + auto ExpectedFile = + TestFile::fromYaml(BuildSectionYaml(".lldbsummaries", section)); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); + + LoadTypeSummariesForModule(module_sp); + + TypeCategoryImplSP category; + DataVisualization::Categories::GetCategory(ConstString("default"), category); + ASSERT_TRUE(category != nullptr); + EXPECT_EQ(category->GetCount(), 0u); +} >From 04bfbb320a8ccaf5c564b49bb61712762e39bd51 Mon Sep 17 00:00:00 2001 From: Dave Lee <[email protected]> Date: Fri, 31 Jul 2026 18:18:37 -0700 Subject: [PATCH 2/2] Review updates --- .../DataFormatters/FormatterSection.cpp | 6 +- .../data-formatter/bytecode-summary/main.cpp | 4 +- .../DataFormatter/FormatterSectionTest.cpp | 138 +++++++----------- 3 files changed, 58 insertions(+), 90 deletions(-) diff --git a/lldb/source/DataFormatters/FormatterSection.cpp b/lldb/source/DataFormatters/FormatterSection.cpp index c0e495806522c..c1e715ae498b3 100644 --- a/lldb/source/DataFormatters/FormatterSection.cpp +++ b/lldb/source/DataFormatters/FormatterSection.cpp @@ -223,9 +223,9 @@ void LoadFormattersForModule(ModuleSP module_sp) { std::make_shared<BytecodeSyntheticChildren>(std::move(impl)); category->AddTypeSynthetic(type_name, match_type, synthetic_children_sp); - LLDB_LOG(GetLog(LLDBLog::DataFormatters), - "Loaded embedded type synthetic for '{0}' from {1}.", - type_name, module_sp->GetFileSpec()); + LLDB_LOG_VERBOSE(GetLog(LLDBLog::DataFormatters), + "Loaded embedded type synthetic for '{0}' from {1}.", + type_name, module_sp->GetFileSpec()); } else { LLDB_LOG(GetLog(LLDBLog::DataFormatters), "No summary or synthetic methods found for '{0}' in {1}, " diff --git a/lldb/test/API/functionalities/data-formatter/bytecode-summary/main.cpp b/lldb/test/API/functionalities/data-formatter/bytecode-summary/main.cpp index eba0f57149f73..8508813edd822 100644 --- a/lldb/test/API/functionalities/data-formatter/bytecode-summary/main.cpp +++ b/lldb/test/API/functionalities/data-formatter/bytecode-summary/main.cpp @@ -34,13 +34,13 @@ int main(int argc, char **argv) { __attribute__((used, section(FORMATTER_SECTION))) unsigned char _MyOptional_type_summary[] = "\x01" // version - "\xa4" // record size + "\xa2" // record size "\x01" // record size "\x10" // type name size "^MyOptional<.+>$" // type name "\x00" // flags "\x00" // sig_summary - "\x8e" // program size + "\x8d" // program size "\x01" // program size "\x1\x22\x7Storage#\x12\x60\x1,C\x10\x1\x5\x11\x2\x1\x22\x6hasVal#" "\x12\x60\x1,\x10\x1e\x2\x22\x1b<could not read MyOptional>\x10G#!\x60 " diff --git a/lldb/unittests/DataFormatter/FormatterSectionTest.cpp b/lldb/unittests/DataFormatter/FormatterSectionTest.cpp index 4ee0bf8e68c66..35a24bef7b0aa 100644 --- a/lldb/unittests/DataFormatter/FormatterSectionTest.cpp +++ b/lldb/unittests/DataFormatter/FormatterSectionTest.cpp @@ -20,6 +20,8 @@ #include "lldb/Host/HostInfo.h" #include "lldb/Target/Platform.h" #include "lldb/ValueObject/ValueObjectConstResult.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/Support/LEB128.h" #include "gtest/gtest.h" #include <optional> @@ -29,34 +31,25 @@ using namespace lldb; using namespace lldb_private; -namespace { - -// --- Helpers for hand-assembling the embedded formatter/summary record -// format read by FormatterSection.cpp, so malformed inputs can be expressed -// as typed fields instead of raw hex blobs. --- - -void AppendULEB(std::vector<uint8_t> &bytes, uint64_t value) { - uint8_t buf[10]; - unsigned len = llvm::encodeULEB128(value, buf); - bytes.insert(bytes.end(), buf, buf + len); -} +// Helpers for building bytecode formatter records, embedded into a binary and +// then read by LoadFormattersForModule. -void AppendBytes(std::vector<uint8_t> &bytes, llvm::StringRef data) { +template <typename T> +static void AppendBytes(std::vector<uint8_t> &bytes, T data) { bytes.insert(bytes.end(), data.begin(), data.end()); } -void AppendBytes(std::vector<uint8_t> &bytes, llvm::ArrayRef<uint8_t> data) { - bytes.insert(bytes.end(), data.begin(), data.end()); +static void AppendULEB(std::vector<uint8_t> &bytes, uint64_t value) { + uint8_t buf[10]; + unsigned len = llvm::encodeULEB128(value, buf); + AppendBytes(bytes, llvm::ArrayRef(buf, len)); } -// Appends one length-framed record: [version][record_size][type_size] -// [type_name][entry]. `record_size` is declared honestly as the size of -// [type_size][type_name][entry] unless a test overrides it to exercise a -// mismatched/corrupt size. -void AppendRecord(std::vector<uint8_t> §ion, uint64_t version, - llvm::StringRef type_name, - llvm::ArrayRef<uint8_t> entry, - std::optional<uint64_t> record_size_override = {}) { +/// Append a bytecode formatter record to a section. +static void AppendRecord(std::vector<uint8_t> §ion, uint64_t version, + llvm::StringRef type_name, + llvm::ArrayRef<uint8_t> entry, + std::optional<uint64_t> record_size_override = {}) { std::vector<uint8_t> body; AppendULEB(body, type_name.size()); AppendBytes(body, type_name); @@ -67,21 +60,10 @@ void AppendRecord(std::vector<uint8_t> §ion, uint64_t version, AppendBytes(section, llvm::ArrayRef<uint8_t>(body)); } -std::string ToHex(llvm::ArrayRef<uint8_t> bytes) { - static const char digits[] = "0123456789ABCDEF"; - std::string hex; - hex.reserve(bytes.size() * 2); - for (uint8_t b : bytes) { - hex.push_back(digits[b >> 4]); - hex.push_back(digits[b & 0xF]); - } - return hex; -} - -// Builds a minimal ELF with a single section named `section_name` whose -// contents are exactly `content` (no implicit padding). -std::string BuildSectionYaml(llvm::StringRef section_name, - llvm::ArrayRef<uint8_t> content) { +/// Build a minimal ELF binary with a single named section with the given +/// contents. +static std::string BuildBinaryYaml(llvm::StringRef section_name, + llvm::ArrayRef<uint8_t> content) { return ("--- !ELF\n" "FileHeader:\n" " Class: ELFCLASS64\n" @@ -97,7 +79,7 @@ std::string BuildSectionYaml(llvm::StringRef section_name, " Address: 0x2010\n" " AddressAlign: 0x10\n" " Content: " + - ToHex(content) + + llvm::toHex(content) + "\n" " Size: " + std::to_string(content.size()) + @@ -105,8 +87,6 @@ std::string BuildSectionYaml(llvm::StringRef section_name, "...\n"); } -} // namespace - namespace { struct MockProcess : Process { @@ -142,7 +122,7 @@ class FormatterSectionTest : public ::testing::Test { // binary registered. TypeCategoryImplSP category; DataVisualization::Categories::GetCategory(ConstString("default"), - category); + category); if (category) category->Clear(); @@ -235,14 +215,13 @@ TEST_F(FormatterSectionTest, LoadFormattersForModule) { ASSERT_EQ(dest, "BBBBB"); } -/// A lone continuation byte (high bit set) is not a complete ULEB128 value, -/// so even the leading version number can't be decoded. This must not read -/// out of bounds or crash. +/// Test an invalid leading version number can't be decoded. TEST_F(FormatterSectionTest, MalformedULEBAtStart) { + // A lone continuation byte (high bit set) is not a complete ULEB128 value. std::vector<uint8_t> section = {0x80}; auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -254,9 +233,7 @@ TEST_F(FormatterSectionTest, MalformedULEBAtStart) { EXPECT_EQ(category->GetCount(), 0u); } -/// A record whose version isn't 1 is unsupported and should be skipped over -/// (using its honestly-declared record_size) without disturbing a -/// well-formed record that follows it. +/// A record whose version isn't 1 is unsupported and should be skipped. TEST_F(FormatterSectionTest, SkipsRecordWithUnsupportedVersion) { std::vector<uint8_t> entry; AppendULEB(entry, /*flags=*/0); @@ -269,7 +246,7 @@ TEST_F(FormatterSectionTest, SkipsRecordWithUnsupportedVersion) { AppendRecord(section, /*version=*/1, "Good", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -286,13 +263,10 @@ TEST_F(FormatterSectionTest, SkipsRecordWithUnsupportedVersion) { nullptr); } -/// The record declares a type name of length 10, but the record itself -/// (honestly sized by the outer record_size field) only has room for 3 -/// bytes of name and nothing else. The type name read must fail cleanly -/// instead of reading past the record's bounds, and a well-formed record -/// that follows must still be reached. -TEST_F(FormatterSectionTest, InnerTypeSizeExceedsRecordBounds) { +/// Test mismatch of decalred type name size and actual length of type name. +TEST_F(FormatterSectionTest, TypeNameSizeExceedsLengthOfTypeName) { std::vector<uint8_t> body; + // Declare a type name of incorrect length (name: "Foo", length: 10). AppendULEB(body, /*type_size=*/10); AppendBytes(body, llvm::StringRef("Foo")); @@ -309,7 +283,7 @@ TEST_F(FormatterSectionTest, InnerTypeSizeExceedsRecordBounds) { AppendRecord(section, /*version=*/1, "Good", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -324,11 +298,7 @@ TEST_F(FormatterSectionTest, InnerTypeSizeExceedsRecordBounds) { nullptr); } -/// A record_size far larger than the number of bytes actually remaining in -/// the section is an internally-inconsistent (corrupt/truncated) record: its -/// own declared size can't be trusted to locate the next record, so it must -/// be rejected rather than silently parsed from whatever bytes happen to be -/// left. A well-formed record preceding it is unaffected. +// Test that a record does not extend past the section it is within. TEST_F(FormatterSectionTest, RecordSizeExceedsRemainingSectionIsRejected) { std::vector<uint8_t> entry; AppendULEB(entry, /*flags=*/0); @@ -339,10 +309,10 @@ TEST_F(FormatterSectionTest, RecordSizeExceedsRemainingSectionIsRejected) { std::vector<uint8_t> section; AppendRecord(section, /*version=*/1, "Good", entry); AppendRecord(section, /*version=*/1, "Oversized", entry, - /*record_size_override=*/1000000); + /*record_size_override=*/1000000); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -360,12 +330,11 @@ TEST_F(FormatterSectionTest, RecordSizeExceedsRemainingSectionIsRejected) { nullptr); } -/// An unrecognized signature byte (with otherwise well-formed size/bytecode -/// framing) is logged and skipped without preventing a later, valid -/// signature in the same entry from being picked up. -TEST_F(FormatterSectionTest, UnsupportedSignatureByteIsSkippedWithinEntry) { +// Test that an unrecognized signature skips the current formatter entry. +TEST_F(FormatterSectionTest, UnsupportedSignatureSkipsEntry) { std::vector<uint8_t> entry; AppendULEB(entry, /*flags=*/0); + // Invalid signature. entry.push_back(0xFF); AppendULEB(entry, /*size=*/2); AppendBytes(entry, llvm::ArrayRef<uint8_t>({0x11, 0x22})); @@ -377,7 +346,7 @@ TEST_F(FormatterSectionTest, UnsupportedSignatureByteIsSkippedWithinEntry) { AppendRecord(section, /*version=*/1, "Widget", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -391,21 +360,19 @@ TEST_F(FormatterSectionTest, UnsupportedSignatureByteIsSkippedWithinEntry) { nullptr); } -/// The declared bytecode size (500) is far larger than the 0 bytes that -/// actually remain in the entry, so reading it fails cleanly rather than -/// reading out of bounds. Since no summary and no synthetic method was -/// successfully parsed, nothing should be registered for the type. +/// Test a signature body being declared with too large a size. TEST_F(FormatterSectionTest, TruncatedBytecodeSizeAbortsEntryParsing) { std::vector<uint8_t> entry; AppendULEB(entry, /*flags=*/0); entry.push_back(FormatterBytecode::Signatures::sig_init); + // Declared bytecode size is larger than the 0 bytes of the entry. AppendULEB(entry, /*size=*/500); std::vector<uint8_t> section; AppendRecord(section, /*version=*/1, "Broken", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -415,13 +382,14 @@ TEST_F(FormatterSectionTest, TruncatedBytecodeSizeAbortsEntryParsing) { DataVisualization::Categories::GetCategory(ConstString("default"), category); ASSERT_TRUE(category != nullptr); EXPECT_EQ(category->GetCount(), 0u); - EXPECT_EQ(category->GetSyntheticForType(std::make_shared<TypeNameSpecifierImpl>( - "Broken", lldb::eFormatterMatchExact)), - nullptr); + EXPECT_EQ( + category->GetSyntheticForType(std::make_shared<TypeNameSpecifierImpl>( + "Broken", lldb::eFormatterMatchExact)), + nullptr); } -/// An entry that has flags but no summary or synthetic-method sub-entries -/// at all (valid framing, just empty) must not register a formatter either. +/// Test that an entry which has flags but neither summary or synthetic +/// signature (valid framing, but empty) must not register a formatter either. TEST_F(FormatterSectionTest, EmptyEntryRegistersNothing) { std::vector<uint8_t> entry; AppendULEB(entry, /*flags=*/0); @@ -430,7 +398,7 @@ TEST_F(FormatterSectionTest, EmptyEntryRegistersNothing) { AppendRecord(section, /*version=*/1, "Empty", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbformatters", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbformatters", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -442,8 +410,8 @@ TEST_F(FormatterSectionTest, EmptyEntryRegistersNothing) { EXPECT_EQ(category->GetCount(), 0u); } -/// An embedded type summary with an empty summary string is dropped instead -/// of being registered. +/// Test that an embedded type summary with an empty summary string is dropped +/// instead of being registered. TEST_F(FormatterSectionTest, EmptySummaryStringIsNotRegistered) { std::vector<uint8_t> entry; AppendULEB(entry, /*summary_size=*/0); @@ -452,7 +420,7 @@ TEST_F(FormatterSectionTest, EmptySummaryStringIsNotRegistered) { AppendRecord(section, /*version=*/1, "Empty", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbsummaries", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbsummaries", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); @@ -464,8 +432,8 @@ TEST_F(FormatterSectionTest, EmptySummaryStringIsNotRegistered) { EXPECT_EQ(category->GetCount(), 0u); } -/// A declared summary_size larger than the bytes actually available in the -/// entry must fail cleanly instead of reading out of bounds, and the +/// Test that a declared summary size larger than the bytes actually available +/// in the entry must fail cleanly instead of reading out of bounds, and the /// summary must not be registered. TEST_F(FormatterSectionTest, SummarySizeExceedsAvailableBytes) { std::vector<uint8_t> entry; @@ -476,7 +444,7 @@ TEST_F(FormatterSectionTest, SummarySizeExceedsAvailableBytes) { AppendRecord(section, /*version=*/1, "Oops", entry); auto ExpectedFile = - TestFile::fromYaml(BuildSectionYaml(".lldbsummaries", section)); + TestFile::fromYaml(BuildBinaryYaml(".lldbsummaries", section)); ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
