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

[Slava Gurevich via Phabricator via lldb-commits]

fixathon reopened this revision.
fixathon marked an inline comment as done.
fixathon added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+      case 'c':
+        if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
           m_count = UINT32_MAX;
----------------
hawkinsw wrote:
> 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
Thanks for your review and posting the link to the spec!


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:992
         LineEntry function_start;
         uint32_t index_ptr = 0, end_ptr;
         std::vector<addr_t> address_list;
----------------
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.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:1012
+              m_options.m_frame_idx, thread->GetID());
+          return false;
+        }
----------------
@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() 


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:1027
+              m_options.m_frame_idx, thread->GetID());
+          return false;
+        }
----------------
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() 


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+      case 'c':
+        if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
           m_count = UINT32_MAX;
----------------
hawkinsw wrote:
> fixathon wrote:
> > 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.
> 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. 
Yes. Keeping it simple.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+      case 'c':
+        if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
           m_count = UINT32_MAX;
----------------
clayborg wrote:
> fixathon wrote:
> > hawkinsw wrote:
> > > fixathon wrote:
> > > > 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.
> > > 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. 
> > Yes. Keeping it simple.
> 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
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131983/new/

https://reviews.llvm.org/D131983