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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
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. 
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131983