https://github.com/felipepiovezan created https://github.com/llvm/llvm-project/pull/74119
This PR cleans up OptionArgParser in a couple of ways: 1. We remove unnecessary std::string temporaries 2. Through else-after-return elimination, we prove the existence of unreachable code >From 6f502ee61bae59a16f90f30357eb72698ad577d0 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <fpiove...@apple.com> Date: Fri, 1 Dec 2023 09:07:11 -0800 Subject: [PATCH 1/3] [lldb][NFC] Remove unnecessary std::string temporaries The existing code was taking three substrings from a regex match and converting to std::strings prior to using them. This may have been done to address null-termination concerns, but this is not the case: 1. `name` was being used to call `c_str()` and then implicitly converted back to a `StringRef` on the call to `ToAddress`. While the path `const char *` -> `StringRef` requires null-termination, we can simply use the original StringRef. 2. `str_offset` was being converted back to a StringRef in order to call a member method. Member methods can't handle non-null termination. 3. `sign` simply had it's 0-th element accessed. --- lldb/source/Interpreter/OptionArgParser.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lldb/source/Interpreter/OptionArgParser.cpp b/lldb/source/Interpreter/OptionArgParser.cpp index ba2d3416e1838a9..9e53261ac885436 100644 --- a/lldb/source/Interpreter/OptionArgParser.cpp +++ b/lldb/source/Interpreter/OptionArgParser.cpp @@ -243,9 +243,9 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, llvm::SmallVector<llvm::StringRef, 4> matches; if (g_symbol_plus_offset_regex.Execute(sref, &matches)) { uint64_t offset = 0; - std::string name = matches[1].str(); - std::string sign = matches[2].str(); - std::string str_offset = matches[3].str(); + llvm::StringRef name = matches[1]; + llvm::StringRef sign = matches[2]; + llvm::StringRef str_offset = matches[3]; if (!llvm::StringRef(str_offset).getAsInteger(0, offset)) { Status error; addr = ToAddress(exe_ctx, name.c_str(), LLDB_INVALID_ADDRESS, &error); >From f7b4bece1f40ffbb22cffb1ef25d56fb34bfedb2 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <fpiove...@apple.com> Date: Fri, 1 Dec 2023 09:44:28 -0800 Subject: [PATCH 2/3] [lldb][NFC] Remove else after return in OptionArgParse This will enable us to prove that there is unreachable code in this function in a subsequent commit. --- lldb/source/Interpreter/OptionArgParser.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lldb/source/Interpreter/OptionArgParser.cpp b/lldb/source/Interpreter/OptionArgParser.cpp index 9e53261ac885436..8698f1de224de20 100644 --- a/lldb/source/Interpreter/OptionArgParser.cpp +++ b/lldb/source/Interpreter/OptionArgParser.cpp @@ -168,7 +168,6 @@ lldb::addr_t OptionArgParser::ToAddress(const ExecutionContext *exe_ctx, std::optional<lldb::addr_t> OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, Status *error_ptr) { - bool error_set = false; if (s.empty()) { if (error_ptr) error_ptr->SetErrorStringWithFormat("invalid address expression \"%s\"", @@ -212,6 +211,7 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, target->EvaluateExpression(s, exe_ctx->GetFramePtr(), valobj_sp, options); bool success = false; + bool error_set = false; if (expr_result == eExpressionCompleted) { if (valobj_sp) valobj_sp = valobj_sp->GetQualifiedRepresentationIfAvailable( @@ -223,16 +223,14 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, if (error_ptr) error_ptr->Clear(); return addr; - } else { - if (error_ptr) { - error_set = true; - error_ptr->SetErrorStringWithFormat( - "address expression \"%s\" resulted in a value whose type " - "can't be converted to an address: %s", - s.str().c_str(), valobj_sp->GetTypeName().GetCString()); - } } - + if (error_ptr) { + error_set = true; + error_ptr->SetErrorStringWithFormat( + "address expression \"%s\" resulted in a value whose type " + "can't be converted to an address: %s", + s.str().c_str(), valobj_sp->GetTypeName().GetCString()); + } } else { // Since the compiler can't handle things like "main + 12" we should try to // do this for now. The compiler doesn't like adding offsets to function @@ -252,8 +250,7 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, if (addr != LLDB_INVALID_ADDRESS) { if (sign[0] == '+') return addr + offset; - else - return addr - offset; + return addr - offset; } } } >From 25decf3ecd15f3782ffe8e5666423d8a750ff14d Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <fpiove...@apple.com> Date: Fri, 1 Dec 2023 09:47:57 -0800 Subject: [PATCH 3/3] [lldb][NFC] Delete unreachable code and dead variable in OptionArgParser With the combination of an early return and removing an else-after-return, it becomes evident that there is unreachable code in the function being changed. --- lldb/source/Interpreter/OptionArgParser.cpp | 62 +++++++++------------ 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/lldb/source/Interpreter/OptionArgParser.cpp b/lldb/source/Interpreter/OptionArgParser.cpp index 8698f1de224de20..2f2c510462dc79a 100644 --- a/lldb/source/Interpreter/OptionArgParser.cpp +++ b/lldb/source/Interpreter/OptionArgParser.cpp @@ -211,7 +211,6 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, target->EvaluateExpression(s, exe_ctx->GetFramePtr(), valobj_sp, options); bool success = false; - bool error_set = false; if (expr_result == eExpressionCompleted) { if (valobj_sp) valobj_sp = valobj_sp->GetQualifiedRepresentationIfAvailable( @@ -224,48 +223,39 @@ OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s, error_ptr->Clear(); return addr; } - if (error_ptr) { - error_set = true; + if (error_ptr) error_ptr->SetErrorStringWithFormat( "address expression \"%s\" resulted in a value whose type " "can't be converted to an address: %s", s.str().c_str(), valobj_sp->GetTypeName().GetCString()); - } - } else { - // Since the compiler can't handle things like "main + 12" we should try to - // do this for now. The compiler doesn't like adding offsets to function - // pointer types. - static RegularExpression g_symbol_plus_offset_regex( - "^(.*)([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*$"); - - llvm::SmallVector<llvm::StringRef, 4> matches; - if (g_symbol_plus_offset_regex.Execute(sref, &matches)) { - uint64_t offset = 0; - llvm::StringRef name = matches[1]; - llvm::StringRef sign = matches[2]; - llvm::StringRef str_offset = matches[3]; - if (!llvm::StringRef(str_offset).getAsInteger(0, offset)) { - Status error; - addr = ToAddress(exe_ctx, name.c_str(), LLDB_INVALID_ADDRESS, &error); - if (addr != LLDB_INVALID_ADDRESS) { - if (sign[0] == '+') - return addr + offset; - return addr - offset; - } - } - } + return {}; + } - if (error_ptr) { - error_set = true; - error_ptr->SetErrorStringWithFormat( - "address expression \"%s\" evaluation failed", s.str().c_str()); + // Since the compiler can't handle things like "main + 12" we should try to + // do this for now. The compiler doesn't like adding offsets to function + // pointer types. + static RegularExpression g_symbol_plus_offset_regex( + "^(.*)([-\\+])[[:space:]]*(0x[0-9A-Fa-f]+|[0-9]+)[[:space:]]*$"); + + llvm::SmallVector<llvm::StringRef, 4> matches; + if (g_symbol_plus_offset_regex.Execute(sref, &matches)) { + uint64_t offset = 0; + llvm::StringRef name = matches[1]; + llvm::StringRef sign = matches[2]; + llvm::StringRef str_offset = matches[3]; + if (!llvm::StringRef(str_offset).getAsInteger(0, offset)) { + Status error; + addr = ToAddress(exe_ctx, name.c_str(), LLDB_INVALID_ADDRESS, &error); + if (addr != LLDB_INVALID_ADDRESS) { + if (sign[0] == '+') + return addr + offset; + return addr - offset; + } } } - if (error_ptr) { - if (!error_set) - error_ptr->SetErrorStringWithFormat("invalid address expression \"%s\"", - s.str().c_str()); - } + if (error_ptr) + error_ptr->SetErrorStringWithFormat( + "address expression \"%s\" evaluation failed", s.str().c_str()); return {}; } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits