Page MenuHomePhabricator

[lldb] Simplify the is_finalized logic in process and make it thread safe.
ClosedPublic

Authored by JDevlieghere on Dec 17 2020, 11:35 AM.

Details

Summary

This is a speculative fix when looking at the finalization code in Process. It tackles the following issues:

  • Adds synchronization to prevent races between threads.
  • Marks the process as finalized/invalid as soon as Finalize is called rather than at the end.
  • Simplifies the code by using only a single instance variable to track finalization.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 17 2020, 11:35 AM
JDevlieghere requested review of this revision.Dec 17 2020, 11:35 AM
jingham accepted this revision.Dec 18 2020, 5:58 PM

LGTM. It looks like we never made use of the distinction between "started to finalize" and "done finalizing", so just marking it at the start of finalization seems fine.

I quibble a bit with "m_finalized" because when it gets set to true we really are starting to finalize, we aren't done finalizing. And it's a little weird to have tests for "am I shutting down", if so don't do X. You might wonder, if we're really all the way done, who would ever ask me this question? I think m_finalizing is a more accurate name. But I don't feel strongly about this.

lldb/source/Target/Process.cpp
1568–1569

I don't think that comment refers to anything still in the code... Might as well ax it here.

This revision is now accepted and ready to land.Dec 18 2020, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2020, 6:41 PM
shafik added a subscriber: shafik.Dec 18 2020, 6:53 PM
shafik added inline comments.
lldb/include/lldb/Target/Process.h
2837
std::atomic<bool> m_finalizing{false};

honestly the whole thing should be refactored, the mem-initializer list is kind of ridiculous.

Thanks Jim, I addressed both issues in the commit.