Skip to content
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.

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

[lldb][Test] Add C++ tests for DumpValueObjectOptions and enums #93158

Merged
merged 2 commits into from
May 28, 2024

Conversation

DavidSpickett
Copy link
Collaborator

DumpValueObjectOptions can only be created and modified from C++. This means it's currently only testable from Python by calling some command that happens to use one, and even so, you can't pick which options get chosen.

So we have decent coverage for the major options that way, but I want to add more niche options that will be harder to test from Python (register field options).

So this change adds some "unit tests", though it's stretching the definition to the point it's more "test written in C++". So we can test future options in isolation.

Since I want to add options specific to enums, that's all it covers. There is a test class that sets up the type system so it will be easy to test other types in future (e.g. structs, which register fields also use).

@llvmbot
Copy link
Collaborator

llvmbot commented May 23, 2024

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

DumpValueObjectOptions can only be created and modified from C++. This means it's currently only testable from Python by calling some command that happens to use one, and even so, you can't pick which options get chosen.

So we have decent coverage for the major options that way, but I want to add more niche options that will be harder to test from Python (register field options).

So this change adds some "unit tests", though it's stretching the definition to the point it's more "test written in C++". So we can test future options in isolation.

Since I want to add options specific to enums, that's all it covers. There is a test class that sets up the type system so it will be easy to test other types in future (e.g. structs, which register fields also use).


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

3 Files Affected:

  • (modified) lldb/unittests/CMakeLists.txt (+1)
  • (added) lldb/unittests/ValueObject/CMakeLists.txt (+10)
  • (added) lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp (+136)
diff --git a/lldb/unittests/CMakeLists.txt b/lldb/unittests/CMakeLists.txt
index c084fa5cca92b..dc1d8b69f0036 100644
--- a/lldb/unittests/CMakeLists.txt
+++ b/lldb/unittests/CMakeLists.txt
@@ -73,6 +73,7 @@ add_subdirectory(tools)
 add_subdirectory(UnwindAssembly)
 add_subdirectory(Utility)
 add_subdirectory(Thread)
+add_subdirectory(ValueObject)
 
 if(LLDB_CAN_USE_DEBUGSERVER AND LLDB_TOOL_DEBUGSERVER_BUILD AND NOT LLDB_USE_SYSTEM_DEBUGSERVER)
   add_subdirectory(debugserver)
diff --git a/lldb/unittests/ValueObject/CMakeLists.txt b/lldb/unittests/ValueObject/CMakeLists.txt
new file mode 100644
index 0000000000000..fb31f76506286
--- /dev/null
+++ b/lldb/unittests/ValueObject/CMakeLists.txt
@@ -0,0 +1,10 @@
+add_lldb_unittest(LLDBValueObjectTests
+  DumpValueObjectOptionsTests.cpp
+
+  LINK_LIBS
+    lldbPluginPlatformLinux
+    lldbPluginScriptInterpreterNone
+
+  LINK_COMPONENTS
+    Support
+  )
diff --git a/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp b/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp
new file mode 100644
index 0000000000000..e89ec4508f6d8
--- /dev/null
+++ b/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp
@@ -0,0 +1,136 @@
+//===-- DumpValueObjectOptionsTests.cpp -----------------------------------===//
+//
+// 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 "Plugins/Platform/Linux/PlatformLinux.h"
+#include "Plugins/ScriptInterpreter/None/ScriptInterpreterNone.h"
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/Symbol/ClangTestUtils.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Core/ValueObjectConstResult.h"
+#include "lldb/DataFormatters/DumpValueObjectOptions.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+struct MockProcess : Process {
+  MockProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+      : Process(target_sp, listener_sp) {}
+
+  llvm::StringRef GetPluginName() override { return "mock process"; }
+
+  bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
+    return false;
+  };
+
+  Status DoDestroy() override { return {}; }
+
+  void RefreshStateAfterStop() override {}
+
+  bool DoUpdateThreadList(ThreadList &old_thread_list,
+                          ThreadList &new_thread_list) override {
+    return false;
+  };
+
+  size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+                      Status &error) override {
+    // No need to read memory in these tests.
+    return size;
+  }
+};
+
+class ValueObjectMockProcessTest : public ::testing::Test {
+public:
+  void SetUp() override {
+    ArchSpec arch("i386-pc-linux");
+    Platform::SetHostPlatform(
+        platform_linux::PlatformLinux::CreateInstance(true, &arch));
+    m_debugger_sp = Debugger::CreateInstance();
+    ASSERT_TRUE(m_debugger_sp);
+    m_debugger_sp->GetTargetList().CreateTarget(*m_debugger_sp, "", arch,
+                                                eLoadDependentsNo,
+                                                m_platform_sp, m_target_sp);
+    ASSERT_TRUE(m_target_sp);
+    ASSERT_TRUE(m_target_sp->GetArchitecture().IsValid());
+    ASSERT_TRUE(m_platform_sp);
+    m_listener_sp = Listener::MakeListener("dummy");
+    m_process_sp = std::make_shared<MockProcess>(m_target_sp, m_listener_sp);
+    ASSERT_TRUE(m_process_sp);
+    m_exe_ctx = ExecutionContext(m_process_sp);
+
+    m_holder = std::make_unique<clang_utils::TypeSystemClangHolder>("test");
+    m_type_system = m_holder->GetAST();
+  }
+
+  ExecutionContext m_exe_ctx;
+  TypeSystemClang *m_type_system;
+
+private:
+  SubsystemRAII<FileSystem, HostInfo, platform_linux::PlatformLinux,
+                ScriptInterpreterNone>
+      m_subsystems;
+
+  std::unique_ptr<clang_utils::TypeSystemClangHolder> m_holder;
+  lldb::DebuggerSP m_debugger_sp;
+  lldb::TargetSP m_target_sp;
+  lldb::PlatformSP m_platform_sp;
+  lldb::ListenerSP m_listener_sp;
+  lldb::ProcessSP m_process_sp;
+};
+
+TEST_F(ValueObjectMockProcessTest, Enum) {
+  CompilerType uint_type = m_type_system->GetBuiltinTypeForEncodingAndBitSize(
+      lldb::eEncodingUint, 32);
+  CompilerType enum_type = m_type_system->CreateEnumerationType(
+      "test_enum", m_type_system->GetTranslationUnitDecl(),
+      OptionalClangModuleID(), Declaration(), uint_type, false);
+
+  m_type_system->StartTagDeclarationDefinition(enum_type);
+  Declaration decl;
+  // Each value sets one bit in the enum, to make this a "bitfield like enum".
+  m_type_system->AddEnumerationValueToEnumerationType(enum_type, decl, "test_2",
+                                                      2, 32);
+  m_type_system->AddEnumerationValueToEnumerationType(enum_type, decl, "test_4",
+                                                      4, 32);
+  m_type_system->CompleteTagDeclarationDefinition(enum_type);
+
+  std::vector<std::tuple<uint32_t, DumpValueObjectOptions, const char *>> enums{
+      {0, {}, "(test_enum) test_var =\n"},
+      {1, {}, "(test_enum) test_var = 0x1\n"},
+      {2, {}, "(test_enum) test_var = test_2\n"},
+      {4, {}, "(test_enum) test_var = test_4\n"},
+      {6, {}, "(test_enum) test_var = test_2 | test_4\n"},
+      {7, {}, "(test_enum) test_var = test_2 | test_4 | 0x1\n"},
+      {8, {}, "(test_enum) test_var = 0x8\n"},
+      {1, DumpValueObjectOptions().SetHideRootName(true), "(test_enum) 0x1\n"},
+      {1, DumpValueObjectOptions().SetHideRootType(true), "test_var = 0x1\n"},
+      {1, DumpValueObjectOptions().SetHideRootName(true).SetHideRootType(true),
+       "0x1\n"},
+      {1, DumpValueObjectOptions().SetHideName(true), "(test_enum) 0x1\n"},
+      {1, DumpValueObjectOptions().SetHideValue(true),
+       "(test_enum) test_var =\n"},
+      {1, DumpValueObjectOptions().SetHideName(true).SetHideValue(true),
+       "(test_enum) \n"},
+  };
+
+  StreamString strm;
+  ExecutionContextScope *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
+  ConstString var_name("test_var");
+  ByteOrder endian = endian::InlHostByteOrder();
+  for (auto [value, options, expected] : enums) {
+    DataExtractor data_extractor{&value, sizeof(value), endian, 4};
+    ValueObjectConstResult::Create(exe_scope, enum_type, var_name,
+                                   data_extractor)
+        ->Dump(strm, options);
+    ASSERT_STREQ(strm.GetString().str().c_str(), expected);
+    strm.Clear();
+  }
+}

DumpValueObjectOptions can only be created and modified from C++.
This means it's currently only testable from Python by calling some
command that happens to use one, and even so, you can't pick
which options get chosen.

So we have decent coverage for the major options that way, but I
want to add more niche options that will be harder to test from
Python (register field options).

So this change adds some "unit tests", though it's stretching the
definition to the point it's more "test written in C++". So
we can test future options in isolation.

Since I want to add options specific to enums, that's all it covers.
There is a test class that sets up the type system so it will be
easy to test other types in future (e.g. structs, which register
fields also use).
@jimingham
Copy link
Collaborator

jimingham commented May 23, 2024

LGTM.

We do offer SBValue::GetDescription, which uses a default DumpValueObjectOptions. So it would be entirely reasonable to make an SBValueDescriptionOptions (or maybe a shorter name?) and add SBValue::GetDescription(SBValueDescriptionOptions) API.

But people can also cons up an equivalent Dump on their own using the other SBValue API's, so leaving the GetDescription as it is and doing the testing on the side like this is also fine.

@DavidSpickett
Copy link
Collaborator Author

I only intend to use the (soon to be) proposed new options from C++, so I'm going to land this as is, but we certainly could pursue the API route later.

@DavidSpickett DavidSpickett merged commit 7cdd53d into llvm:main May 28, 2024
3 checks passed
@DavidSpickett DavidSpickett deleted the valueobject branch May 28, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants