[Lldb-commits] [PATCH] D131983: [LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

[Slava Gurevich via Phabricator via lldb-commits]

fixathon created this revision.
fixathon added reviewers: clayborg, JDevlieghere, DavidSpickett, jasonmolenda.
Herald added a project: All.
fixathon published this revision for review.
fixathon added inline comments.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+      case 'c':
+        if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
           m_count = UINT32_MAX;
----------------
If getAsInteger() fails we trigger the error path.
If getAsInteger() succeeds, but the fetched value (side-effect) is negative we trigger the error path.

I'm not 100% happy with my own code here..
getAsInteger() somewhat counter-intuitively returns *true* on failure, and there's reliance on the short-circuit evaluation sequence-points for correct evaluation when the 1st part is setting value via side-effect, and the second validates it. This makes the expression somewhat hard to reason about.

Also mutating **m_count** directly may result in a non-atomic transition to its new valid state in case when getAsInteger() fetches negative input, which is less than perfect.

That said, this code is just parsing command options, in a single-threaded manner, so that shouldn't matter.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:71
               "invalid integer value for option '%c'", short_option);
-        } else if (input_count < 0)
-          m_count = UINT32_MAX;
----------------
Note how we check **input_count** that's never been set (beyond the initial value). This is broken.


- Improve reliability by checking return results for calls to FindLineEntryByAddress()
- Fix broken option parsing in SetOptionValue()


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131983

Files:
  lldb/source/Commands/CommandObjectThread.cpp


Index: lldb/source/Commands/CommandObjectThread.cpp
===================================================================
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -62,15 +62,13 @@
       const int short_option = m_getopt_table[option_idx].val;
 
       switch (short_option) {
-      case 'c': {
-        int32_t input_count = 0;
-        if (option_arg.getAsInteger(0, m_count)) {
+      case 'c':
+        if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
           m_count = UINT32_MAX;
           error.SetErrorStringWithFormat(
               "invalid integer value for option '%c'", short_option);
-        } else if (input_count < 0)
-          m_count = UINT32_MAX;
-      } break;
+        }
+        break;
       case 's':
         if (option_arg.getAsInteger(0, m_start))
           error.SetErrorStringWithFormat(
@@ -1004,8 +1002,15 @@
 
         AddressRange fun_addr_range = sc.function->GetAddressRange();
         Address fun_start_addr = fun_addr_range.GetBaseAddress();
-        line_table->FindLineEntryByAddress(fun_start_addr, function_start,
-                                           &index_ptr);
+
+        if (!line_table->FindLineEntryByAddress(fun_start_addr, function_start,
+                                                &index_ptr)) {
+          result.AppendErrorWithFormat(
+              "Failed to find line entry by address for "
+              "frame %u of thread id %" PRIu64 ".\n",
+              m_options.m_frame_idx, thread->GetID());
+          return false;
+        }
 
         Address fun_end_addr(fun_start_addr.GetSection(),
                              fun_start_addr.GetOffset() +
@@ -1013,8 +1018,14 @@
 
         bool all_in_function = true;
 
-        line_table->FindLineEntryByAddress(fun_end_addr, function_start,
-                                           &end_ptr);
+        if (!line_table->FindLineEntryByAddress(fun_end_addr, function_start,
+                                                &end_ptr)) {
+          result.AppendErrorWithFormat(
+              "Failed to find line entry by address for "
+              "frame %u of thread id %" PRIu64 ".\n",
+              m_options.m_frame_idx, thread->GetID());
+          return false;
+        }
 
         // Since not all source lines will contribute code, check if we are
         // setting the breakpoint on the exact line number or the nearest


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D131983.453076.patch
Type: text/x-patch
Size: 2442 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220816/f2a65c10/attachment.bin>