This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by fixathon on Aug 16 2022, 11:08 AM.

Details

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

Diff Detail

Event Timeline

fixathon created this revision.Aug 16 2022, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 11:08 AM
fixathon published this revision for review.Aug 16 2022, 12:05 PM
fixathon added inline comments.
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.

Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 12:05 PM
hawkinsw added inline comments.
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 added inline comments.Aug 16 2022, 3:08 PM
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 accepted this revision.Aug 16 2022, 3:15 PM
clayborg added inline comments.
lldb/source/Commands/CommandObjectThread.cpp
66

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

This revision is now accepted and ready to land.Aug 16 2022, 3:15 PM
This revision was automatically updated to reflect the committed changes.
fixathon marked 3 inline comments as done.
hawkinsw added inline comments.Aug 16 2022, 3:33 PM
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

fixathon reopened this revision.Aug 17 2022, 12:16 AM
fixathon marked an inline comment as done.

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.

1012

@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()

1027

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()

This revision is now accepted and ready to land.Aug 17 2022, 12:16 AM
labath added a subscriber: labath.Aug 17 2022, 1:50 AM
labath added inline comments.
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 planned changes to this revision.Aug 17 2022, 11:15 AM
fixathon marked 2 inline comments as done.

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 updated this revision to Diff 453392.Aug 17 2022, 12:17 PM
fixathon marked an inline comment as done.

Back out changes for checking the return val of the function

This revision is now accepted and ready to land.Aug 17 2022, 12:17 PM
This revision was landed with ongoing or failed builds.Aug 17 2022, 12:20 PM
This revision was automatically updated to reflect the committed changes.

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)) {
      |                                                     ~~~~~~~~^~~