From 3ebb33632a1509450fdbc1fb6a21107a0a513072 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Sun, 14 May 2023 19:58:16 -0700 Subject: [PATCH] [lldb] Complete OptionValue cleanup (NFC) Make the `Get.*Value` and `Set.*Value` function private and migrate the last remaining call sites to the new overloaded/templated functions. --- lldb/include/lldb/Core/Debugger.h | 4 +- lldb/include/lldb/Interpreter/OptionValue.h | 100 ++++++++++----------- lldb/source/Commands/CommandObjectBreakpoint.cpp | 8 +- lldb/source/Commands/CommandObjectMemory.cpp | 7 +- lldb/source/Commands/CommandObjectRegister.cpp | 5 +- lldb/source/Core/Debugger.cpp | 4 +- lldb/source/Core/Disassembler.cpp | 5 +- lldb/source/Interpreter/OptionValue.cpp | 9 +- lldb/source/Interpreter/OptionValueArgs.cpp | 2 +- lldb/source/Interpreter/OptionValueArray.cpp | 2 +- .../Instruction/ARM/EmulateInstructionARM.cpp | 2 +- .../Plugins/Instruction/ARM/EmulationStateARM.cpp | 9 +- .../Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | 4 +- .../Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp | 2 +- lldb/source/Target/Process.cpp | 6 +- lldb/unittests/Interpreter/TestOptionValue.cpp | 14 +-- 16 files changed, 94 insertions(+), 89 deletions(-) diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h index f05d8d4..54f7d5c 100644 --- a/lldb/include/lldb/Core/Debugger.h +++ b/lldb/include/lldb/Core/Debugger.h @@ -285,7 +285,7 @@ public: uint64_t GetTerminalWidth() const; - bool SetTerminalWidth(uint32_t term_width); + bool SetTerminalWidth(uint64_t term_width); llvm::StringRef GetPrompt() const; @@ -351,7 +351,7 @@ public: uint64_t GetTabSize() const; - bool SetTabSize(uint32_t tab_size); + bool SetTabSize(uint64_t tab_size); lldb::DWIMPrintVerbosity GetDWIMPrintVerbosity() const; diff --git a/lldb/include/lldb/Interpreter/OptionValue.h b/lldb/include/lldb/Interpreter/OptionValue.h index 730ec65..9e65c80 100644 --- a/lldb/include/lldb/Interpreter/OptionValue.h +++ b/lldb/include/lldb/Interpreter/OptionValue.h @@ -17,6 +17,8 @@ #include "lldb/Utility/FileSpec.h" #include "lldb/Utility/FileSpecList.h" #include "lldb/Utility/Status.h" +#include "lldb/Utility/StringList.h" +#include "lldb/Utility/UUID.h" #include "lldb/lldb-defines.h" #include "lldb/lldb-private-enumerations.h" #include "lldb/lldb-private-interfaces.h" @@ -260,58 +262,8 @@ public: const OptionValueFormatEntity *GetAsFormatEntity() const; - std::optional GetBooleanValue() const; - - bool SetBooleanValue(bool new_value); - - std::optional GetCharValue() const; - - char SetCharValue(char new_value); - - std::optional GetEnumerationValue() const; - - bool SetEnumerationValue(int64_t value); - - std::optional GetFileSpecValue() const; - - bool SetFileSpecValue(FileSpec file_spec); - bool AppendFileSpecValue(FileSpec file_spec); - std::optional GetFileSpecListValue() const; - - std::optional GetFormatValue() const; - - bool SetFormatValue(lldb::Format new_value); - - std::optional GetLanguageValue() const; - - bool SetLanguageValue(lldb::LanguageType new_language); - - const FormatEntity::Entry *GetFormatEntity() const; - - const RegularExpression *GetRegexValue() const; - - std::optional GetSInt64Value() const; - - bool SetSInt64Value(int64_t new_value); - - std::optional GetStringValue() const; - - bool SetStringValue(llvm::StringRef new_value); - - std::optional GetUInt64Value() const; - - bool SetUInt64Value(uint64_t new_value); - - UUID GetUUIDValue() const; - - bool SetUUIDValue(const UUID &uuid); - - std::optional GetArchSpecValue() const; - - bool SetArchSpecValue(ArchSpec arch_spec); - bool OptionWasSet() const { return m_value_was_set; } void SetOptionWasSet() { m_value_was_set = true; } @@ -373,10 +325,20 @@ public: bool SetValueAs(bool v) { return SetBooleanValue(v); } + bool SetValueAs(char v) { return SetCharValue(v); } + + bool SetValueAs(uint64_t v) { return SetUInt64Value(v); } + + bool SetValueAs(int64_t v) { return SetSInt64Value(v); } + + bool SetValueAs(UUID v) { return SetUUIDValue(v); } + bool SetValueAs(llvm::StringRef v) { return SetStringValue(v); } bool SetValueAs(lldb::LanguageType v) { return SetLanguageValue(v); } + bool SetValueAs(lldb::Format v) { return SetFormatValue(v); } + bool SetValueAs(FileSpec v) { return SetFileSpecValue(v); } bool SetValueAs(ArchSpec v) { return SetArchSpecValue(v); } @@ -401,6 +363,44 @@ protected: // set from the command line or as a setting, // versus if we just have the default value that // was already populated in the option value. +private: + std::optional GetArchSpecValue() const; + bool SetArchSpecValue(ArchSpec arch_spec); + + std::optional GetBooleanValue() const; + bool SetBooleanValue(bool new_value); + + std::optional GetCharValue() const; + bool SetCharValue(char new_value); + + std::optional GetEnumerationValue() const; + bool SetEnumerationValue(int64_t value); + + std::optional GetFileSpecValue() const; + bool SetFileSpecValue(FileSpec file_spec); + + std::optional GetFileSpecListValue() const; + + std::optional GetSInt64Value() const; + bool SetSInt64Value(int64_t new_value); + + std::optional GetUInt64Value() const; + bool SetUInt64Value(uint64_t new_value); + + std::optional GetFormatValue() const; + bool SetFormatValue(lldb::Format new_value); + + std::optional GetLanguageValue() const; + bool SetLanguageValue(lldb::LanguageType new_language); + + std::optional GetStringValue() const; + bool SetStringValue(llvm::StringRef new_value); + + std::optional GetUUIDValue() const; + bool SetUUIDValue(const UUID &uuid); + + const FormatEntity::Entry *GetFormatEntity() const; + const RegularExpression *GetRegexValue() const; }; } // namespace lldb_private diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp index 3debbb3..30d2434 100644 --- a/lldb/source/Commands/CommandObjectBreakpoint.cpp +++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp @@ -1740,7 +1740,7 @@ protected: BreakpointSP bp_sp; if (m_bp_id.m_breakpoint.OptionWasSet()) { lldb::break_id_t bp_id = - m_bp_id.m_breakpoint.GetUInt64Value().value_or(0); + m_bp_id.m_breakpoint.GetValueAs().value_or(0); bp_sp = target.GetBreakpointByID(bp_id); if (!bp_sp) { result.AppendErrorWithFormatv("Could not find specified breakpoint {0}", @@ -1756,8 +1756,10 @@ protected: if (!bp_name) continue; if (m_bp_id.m_help_string.OptionWasSet()) - bp_name->SetHelp( - m_bp_id.m_help_string.GetStringValue().value_or("").str().c_str()); + bp_name->SetHelp(m_bp_id.m_help_string.GetValueAs() + .value_or("") + .str() + .c_str()); if (bp_sp) target.ConfigureBreakpointName(*bp_name, bp_sp->GetOptions(), diff --git a/lldb/source/Commands/CommandObjectMemory.cpp b/lldb/source/Commands/CommandObjectMemory.cpp index a8896f8..ba5aad3d 100644 --- a/lldb/source/Commands/CommandObjectMemory.cpp +++ b/lldb/source/Commands/CommandObjectMemory.cpp @@ -1048,7 +1048,7 @@ protected: if (m_memory_options.m_string.OptionWasSet()) { llvm::StringRef str = - m_memory_options.m_string.GetStringValue().value_or(""); + m_memory_options.m_string.GetValueAs().value_or(""); if (str.empty()) { result.AppendError("search string must have non-zero length."); return false; @@ -1059,8 +1059,9 @@ protected: ValueObjectSP result_sp; if ((eExpressionCompleted == process->GetTarget().EvaluateExpression( - m_memory_options.m_expr.GetStringValue().value_or(""), frame, - result_sp)) && + m_memory_options.m_expr.GetValueAs().value_or( + ""), + frame, result_sp)) && result_sp) { uint64_t value = result_sp->GetValueAsUnsigned(0); std::optional size = diff --git a/lldb/source/Commands/CommandObjectRegister.cpp b/lldb/source/Commands/CommandObjectRegister.cpp index 80813cd..7c1155b 100644 --- a/lldb/source/Commands/CommandObjectRegister.cpp +++ b/lldb/source/Commands/CommandObjectRegister.cpp @@ -171,8 +171,9 @@ protected: const size_t set_array_size = m_command_options.set_indexes.GetSize(); if (set_array_size > 0) { for (size_t i = 0; i < set_array_size; ++i) { - set_idx = m_command_options.set_indexes[i]->GetUInt64Value().value_or( - UINT32_MAX); + set_idx = + m_command_options.set_indexes[i]->GetValueAs().value_or( + UINT32_MAX); if (set_idx < reg_ctx->GetRegisterSetCount()) { if (!DumpRegisterSet(m_exe_ctx, strm, reg_ctx, set_idx)) { if (errno) diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index e398799..1d92f2f 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -349,7 +349,7 @@ uint64_t Debugger::GetTerminalWidth() const { idx, g_debugger_properties[idx].default_uint_value); } -bool Debugger::SetTerminalWidth(uint32_t term_width) { +bool Debugger::SetTerminalWidth(uint64_t term_width) { if (auto handler_sp = m_io_handler_stack.Top()) handler_sp->TerminalSizeChanged(); @@ -544,7 +544,7 @@ uint64_t Debugger::GetTabSize() const { idx, g_debugger_properties[idx].default_uint_value); } -bool Debugger::SetTabSize(uint32_t tab_size) { +bool Debugger::SetTabSize(uint64_t tab_size) { const uint32_t idx = ePropertyTabSize; return SetPropertyAtIndex(idx, tab_size); } diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp index db91dc5..bc9bf4f 100644 --- a/lldb/source/Core/Disassembler.cpp +++ b/lldb/source/Core/Disassembler.cpp @@ -908,7 +908,7 @@ bool Instruction::TestEmulation(Stream *out_stream, const char *file_name) { return false; } - SetDescription(value_sp->GetStringValue().value_or("")); + SetDescription(value_sp->GetValueAs().value_or("")); value_sp = data_dictionary->GetValueForKey(triple_key); if (!value_sp) { @@ -918,7 +918,8 @@ bool Instruction::TestEmulation(Stream *out_stream, const char *file_name) { } ArchSpec arch; - arch.SetTriple(llvm::Triple(value_sp->GetStringValue().value_or(""))); + arch.SetTriple( + llvm::Triple(value_sp->GetValueAs().value_or(""))); bool success = false; std::unique_ptr insn_emulator_up( diff --git a/lldb/source/Interpreter/OptionValue.cpp b/lldb/source/Interpreter/OptionValue.cpp index d29f86d..7fa34c5 100644 --- a/lldb/source/Interpreter/OptionValue.cpp +++ b/lldb/source/Interpreter/OptionValue.cpp @@ -272,7 +272,7 @@ std::optional OptionValue::GetCharValue() const { return {}; } -char OptionValue::SetCharValue(char new_value) { +bool OptionValue::SetCharValue(char new_value) { OptionValueChar *option_value = GetAsChar(); if (option_value) { option_value->SetCurrentValue(new_value); @@ -415,11 +415,10 @@ bool OptionValue::SetUInt64Value(uint64_t new_value) { return false; } -UUID OptionValue::GetUUIDValue() const { - const OptionValueUUID *option_value = GetAsUUID(); - if (option_value) +std::optional OptionValue::GetUUIDValue() const { + if (const OptionValueUUID *option_value = GetAsUUID()) return option_value->GetCurrentValue(); - return UUID(); + return {}; } bool OptionValue::SetUUIDValue(const UUID &uuid) { diff --git a/lldb/source/Interpreter/OptionValueArgs.cpp b/lldb/source/Interpreter/OptionValueArgs.cpp index e9f73e6..9636516 100644 --- a/lldb/source/Interpreter/OptionValueArgs.cpp +++ b/lldb/source/Interpreter/OptionValueArgs.cpp @@ -16,6 +16,6 @@ using namespace lldb_private; size_t OptionValueArgs::GetArgs(Args &args) const { args.Clear(); for (const auto &value : m_values) - args.AppendArgument(value->GetStringValue().value_or("")); + args.AppendArgument(value->GetValueAs().value_or("")); return args.GetArgumentCount(); } diff --git a/lldb/source/Interpreter/OptionValueArray.cpp b/lldb/source/Interpreter/OptionValueArray.cpp index 796b652..08b5f86 100644 --- a/lldb/source/Interpreter/OptionValueArray.cpp +++ b/lldb/source/Interpreter/OptionValueArray.cpp @@ -153,7 +153,7 @@ size_t OptionValueArray::GetArgs(Args &args) const { args.Clear(); const uint32_t size = m_values.size(); for (uint32_t i = 0; i < size; ++i) { - std::optional string_value = m_values[i]->GetStringValue(); + auto string_value = m_values[i]->GetValueAs(); if (string_value) args.AppendArgument(*string_value); } diff --git a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp index a8cc86c..7c79f2a 100644 --- a/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp +++ b/lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp @@ -14364,7 +14364,7 @@ bool EmulateInstructionARM::TestEmulation(Stream *out_stream, ArchSpec &arch, out_stream->Printf("TestEmulation: Error reading opcode from test file.\n"); return false; } - test_opcode = value_sp->GetUInt64Value().value_or(0); + test_opcode = value_sp->GetValueAs().value_or(0); if (arch.GetTriple().getArch() == llvm::Triple::thumb || arch.IsAlwaysThumbInstructions()) { diff --git a/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp b/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp index fd78755..c11d4c6 100644 --- a/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp +++ b/lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp @@ -267,7 +267,7 @@ bool EmulationStateARM::LoadRegistersStateFromDictionary( OptionValueSP value_sp = reg_dict->GetValueForKey(sstr.GetString()); if (value_sp.get() == nullptr) return false; - uint64_t reg_value = value_sp->GetUInt64Value().value_or(0); + uint64_t reg_value = value_sp->GetValueAs().value_or(0); StorePseudoRegisterValue(first_reg + i, reg_value); } @@ -296,7 +296,7 @@ bool EmulationStateARM::LoadStateFromDictionary( if (value_sp.get() == nullptr) return false; else - start_address = value_sp->GetUInt64Value().value_or(0); + start_address = value_sp->GetValueAs().value_or(0); value_sp = mem_dict->GetValueForKey(data_key); OptionValueArray *mem_array = value_sp->GetAsArray(); @@ -310,7 +310,7 @@ bool EmulationStateARM::LoadStateFromDictionary( value_sp = mem_array->GetValueAtIndex(i); if (value_sp.get() == nullptr) return false; - uint64_t value = value_sp->GetUInt64Value().value_or(0); + uint64_t value = value_sp->GetValueAs().value_or(0); StoreToPseudoAddress(address, value); address = address + 4; } @@ -330,7 +330,8 @@ bool EmulationStateARM::LoadStateFromDictionary( value_sp = reg_dict->GetValueForKey(cpsr_name); if (value_sp.get() == nullptr) return false; - StorePseudoRegisterValue(dwarf_cpsr, value_sp->GetUInt64Value().value_or(0)); + StorePseudoRegisterValue(dwarf_cpsr, + value_sp->GetValueAs().value_or(0)); // Load s/d Registers // To prevent you giving both types in a state and overwriting diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp index 905226d..30ff0e5 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp @@ -318,8 +318,8 @@ size_t ObjectFilePECOFF::GetModuleSpecifications( llvm::Triple::EnvironmentType env; if (module_env_option) env = - (llvm::Triple::EnvironmentType)module_env_option->GetEnumerationValue() - .value_or(0); + module_env_option->GetValueAs().value_or( + static_cast(0)); else env = GetGlobalPluginProperties().ABI(); diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp index 7f98423..9267a1a 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp @@ -887,7 +887,7 @@ public: "the --command option must be set to a valid command byte"); } else { const uint64_t command_byte = - m_command_byte.GetOptionValue().GetUInt64Value().value_or(0); + m_command_byte.GetOptionValue().GetValueAs().value_or(0); if (command_byte > 0 && command_byte <= UINT8_MAX) { ProcessKDP *process = (ProcessKDP *)m_interpreter.GetExecutionContext().GetProcessPtr(); diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 0f7c15a..9e9b1da 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -225,7 +225,7 @@ uint32_t ProcessProperties::GetVirtualAddressableBits() const { void ProcessProperties::SetVirtualAddressableBits(uint32_t bits) { const uint32_t idx = ePropertyVirtualAddressableBits; - SetPropertyAtIndex(idx, bits); + SetPropertyAtIndex(idx, static_cast(bits)); } void ProcessProperties::SetPythonOSPluginPath(const FileSpec &file) { const uint32_t idx = ePropertyPythonOSPluginPath; @@ -483,10 +483,10 @@ Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp, OptionValueSP value_sp = m_collection_sp->GetPropertyAtIndex(ePropertyMemCacheLineSize) ->GetValue(); - uint32_t platform_cache_line_size = + uint64_t platform_cache_line_size = target_sp->GetPlatform()->GetDefaultMemoryCacheLineSize(); if (!value_sp->OptionWasSet() && platform_cache_line_size != 0) - value_sp->SetUInt64Value(platform_cache_line_size); + value_sp->SetValueAs(platform_cache_line_size); RegisterAssertFrameRecognizer(this); } diff --git a/lldb/unittests/Interpreter/TestOptionValue.cpp b/lldb/unittests/Interpreter/TestOptionValue.cpp index 64adcff..7753d3d 100644 --- a/lldb/unittests/Interpreter/TestOptionValue.cpp +++ b/lldb/unittests/Interpreter/TestOptionValue.cpp @@ -41,11 +41,11 @@ TEST(OptionValueString, DeepCopy) { ASSERT_TRUE(copy_sp); ASSERT_EQ(copy_sp->GetParent().get(), nullptr); ASSERT_TRUE(copy_sp->OptionWasSet()); - ASSERT_EQ(copy_sp->GetStringValue(), "ab"); + ASSERT_EQ(copy_sp->GetValueAs(), "ab"); // Trigger the callback. copy_sp->SetValueFromString("c", eVarSetOperationAppend); - ASSERT_EQ(copy_sp->GetStringValue(), "abc"); + ASSERT_EQ(copy_sp->GetValueAs(), "abc"); } // Test an aggregate class. @@ -67,15 +67,15 @@ TEST(OptionValueArgs, DeepCopy) { auto *args_copy_ptr = copy_sp->GetAsArgs(); ASSERT_EQ(args_copy_ptr->GetSize(), 2U); ASSERT_EQ((*args_copy_ptr)[0]->GetParent(), copy_sp); - ASSERT_EQ((*args_copy_ptr)[0]->GetStringValue(), "A"); + ASSERT_EQ((*args_copy_ptr)[0]->GetValueAs(), "A"); ASSERT_EQ((*args_copy_ptr)[1]->GetParent(), copy_sp); - ASSERT_EQ((*args_copy_ptr)[1]->GetStringValue(), "B"); + ASSERT_EQ((*args_copy_ptr)[1]->GetValueAs(), "B"); // Trigger the callback. copy_sp->SetValueFromString("C", eVarSetOperationAppend); ASSERT_TRUE(args_copy_ptr); ASSERT_EQ(args_copy_ptr->GetSize(), 3U); - ASSERT_EQ((*args_copy_ptr)[2]->GetStringValue(), "C"); + ASSERT_EQ((*args_copy_ptr)[2]->GetValueAs(), "C"); } class TestProperties : public OptionValueProperties { @@ -149,12 +149,12 @@ TEST(TestProperties, DeepCopy) { auto value_ptr = dict_copy_ptr->GetValueForKey("A"); ASSERT_TRUE(value_ptr); ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr); - ASSERT_EQ(value_ptr->GetUInt64Value(), 1U); + ASSERT_EQ(value_ptr->GetValueAs(), 1U); value_ptr = dict_copy_ptr->GetValueForKey("B"); ASSERT_TRUE(value_ptr); ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr); - ASSERT_EQ(value_ptr->GetUInt64Value(), 2U); + ASSERT_EQ(value_ptr->GetValueAs(), 2U); // Test the second child. auto file_list_copy_ptr = props_copy_ptr->GetFileList(); -- 2.7.4