[LLDB][NFC] Fix optons parsing and misc. reliability in CommandObjectThread

[fixathon]

Summary

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

[fixathon]

lldb/source/Commands/CommandObjectThread.cpp

66

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.

71

Note how we check input_count that's never been set (beyond the initial value). This is broken.

[hawkinsw]

lldb/source/Commands/CommandObjectThread.cpp

66

I am not sure if this helps or not, but ...

getAsInteger is the exact interface that the LLVM command-line parsing library uses for parsing numeric options. I looked through the codebase for other places that do the same type of operation to see if they use a different technique and could not find anything.

To address your non-atomic comment, we could introduce a temporary? Given your description of how this code will be used (single-threaded), that seems like overkill?

I am no expert and I hope that being part of the discussion is helpful.

[clayborg]

lldb/source/Commands/CommandObjectThread.cpp

66

Can we rely on ordering here? Or do we need to have two if statements, first one for getAsInteger and a second nested on for "m_count < 0"? I would be safe if we don't know the right answer

[clayborg]

lldb/source/Commands/CommandObjectThread.cpp

66

Actually this should work just fine. Otherwise things like "if (ptr && ptr->...)" wouldn't work.

[hawkinsw]

lldb/source/Commands/CommandObjectThread.cpp

66

Hope this doesn't screw up Phabricator, but I just wanted to confirm with @clayborg that, yes, that is specified:

http://eel.is/c++draft/expr.log.and

[stella.stamenova]

This broke the windows lldb bot: https://lab.llvm.org/buildbot/#/builders/83/builds/22557/steps/7/logs/stdio

[fixathon]

Thank you, @stella.stamenova , will investigate. Added comments with the initial findings

lldb/source/Commands/CommandObjectThread.cpp

66

Yes. Keeping it simple.

66

afaik, it's guaranteed by the language spec to be sequenced, but agree the code, generally, shouldn't be written like this because it's too hard to reason about its correctness.

66

Thanks for your review and posting the link to the spec!

992

Will explicitly initialize end_ptr tо UINT32_MAX so that we don't rely on the whims of FindLineEntryByAddress() for initialization that currently happens even when the call does not succeed. Note, this is not currently broken.

1013

@clayborg Adding this early return is causing buildbot test failures on x86_64-debian and x64-windows, but not on MacOS. Will investigate if this is expected behavior for FindLineEntryByAddress()

1021

Adding this early return is causing buildbot test failures on x86_64-debian and x64-windows, but not on MacOS. Will investigate if this is expected behavior for FindLineEntryByAddress()

[labath]

lldb/source/Commands/CommandObjectThread.cpp

66

There is an llvm::to_integer. One of the motivations for it was to change the awkwardness to the getAsInteger return value. It hasn't taken the world by storm because it wasn't followed by a conversion of a critical mass of call sites, but you should feel free to use it.

[fixathon]

Planning to undo checking the return value of FindLineEntryByAddress() since it's causing an issue on some platform, and re-evaluate the underlying issue.

lldb/source/Commands/CommandObjectThread.cpp

66

@labath Thanks for the pointer to llvm::to_integer

[fixathon]

Back out changes for checking the return val of the function

[mstorsjo]

This commit caused warnings when building with GCC:

../tools/lldb/source/Commands/CommandObjectThread.cpp: In member function ‘virtual lldb_private::Status CommandObjectThreadBacktrace::CommandOptions::SetOptionValue(uint32_t, llvm::StringRef, lldb_private::ExecutionContext*)’:
../tools/lldb/source/Commands/CommandObjectThread.cpp:66:61: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   66 |         if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
      |                                                     ~~~~~~~~^~~

lldb/source/Commands/CommandObjectThread.cpp

public:
     ~CommandOptions() override = default;
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                           ExecutionContext *execution_context) override {
       Status error;
       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(
               "invalid integer value for option '%c'", short_option);
         break;
       case 'e': {
         bool success;
         m_extended_backtrace =
if (process == nullptr) {
         if (line_table == nullptr) {
           result.AppendErrorWithFormat("Failed to resolve the line table for "
                                        "frame %u of thread id %" PRIu64 ".\n",
                                        m_options.m_frame_idx, thread->GetID());
           return false;
         }
 
         LineEntry function_start;
-        uint32_t index_ptr = 0, end_ptr;
+        uint32_t index_ptr = 0, end_ptr = UINT32_MAX;
         std::vector<addr_t> address_list;
 
         // Find the beginning & end index of the function, but first make
         // sure it is valid:
         if (!sc.function) {
           result.AppendErrorWithFormat("Have debug information but no "
                                        "function info - can't get until range.");
           return false;
         }
 
         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);
 
         Address fun_end_addr(fun_start_addr.GetSection(),
                              fun_start_addr.GetOffset() +
                                  fun_addr_range.GetByteSize());
 
         bool all_in_function = true;
 
         line_table->FindLineEntryByAddress(fun_end_addr, function_start,
                                            &end_ptr);
 
         // Since not all source lines will contribute code, check if we are
         // setting the breakpoint on the exact line number or the nearest
         // subsequent line number and set breakpoints at all the line table
         // entries of the chosen line number (exact or nearest subsequent).
         for (uint32_t line_number : line_numbers) {
           LineEntry line_entry;
           bool exact = false;
           uint32_t start_idx_ptr = index_ptr;
           start_idx_ptr = sc.comp_unit->FindLineEntry(
               index_ptr, line_number, nullptr, exact, &line_entry);
           if (start_idx_ptr != UINT32_MAX)
             line_number = line_entry.line;
           exact = true;