This is an archive of the discontinued LLVM Phabricator instance.

[Function] Lock the function when parsing call site info
ClosedPublic

Authored by vsk on Jul 7 2020, 4:59 PM.

Details

Summary

DWARF-parsing methods in SymbolFileDWARF which update module state
typically take the module lock. ParseCallEdgesInFunction doesn't do
this, but higher-level locking within lldb::Function (which owns the
storage for parsed call edges) is necessary.

The lack of locking could explain some as-of-yet unreproducible crashes
which occur in Function::GetTailCallingEdges(). In these crashes, the
m_call_edges vector is non-empty but contains a nullptr, which
shouldn't be possible. (If this vector is non-empty, it _must_ contain a
non-null unique_ptr.)

This may address rdar://55622443 and rdar://65119458.

Diff Detail

Event Timeline

vsk created this revision.Jul 7 2020, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 4:59 PM
Herald added a subscriber: aprantl. · View Herald Transcript
vsk planned changes to this revision.Jul 7 2020, 5:05 PM

Hm, this doesn't totally fix the race. If the mutex is contested, the Function instance may overwrite its m_call_edges vector.

vsk updated this revision to Diff 276280.Jul 7 2020, 5:38 PM

Move locking up into lldb::Function, and leave a comment in SymbolFileDWARF explaining why.

vsk retitled this revision from [SymbolFileDWARF] Lock the module when parsing call site info to [Function] Lock the function when parsing call site info.Jul 7 2020, 5:38 PM
vsk edited the summary of this revision. (Show Details)
aprantl accepted this revision.Jul 8 2020, 3:38 PM

Looks plausible.

lldb/include/lldb/Symbol/Function.h
661 ↗(On Diff #276280)

nit: When inline comments span multiple lines it's IMO less confusing to convert them to up-front comments.

/// Exclusive lock that controls read/write
/// access to m_call_edges and m_call_edges_resolved.
std::mutex m_call_edges_lock;

I also doubt that the ones in this file are formatted correctly, I think the continuation needs to also use ///<.

lldb/source/Symbol/Function.cpp
293 ↗(On Diff #276280)

Can this be called on the same thread and would we benefit from a recursive_mutex?

This revision is now accepted and ready to land.Jul 8 2020, 3:38 PM
vsk added inline comments.Jul 9 2020, 10:29 AM
lldb/include/lldb/Symbol/Function.h
661 ↗(On Diff #276280)

Sounds good. I'll reformat all of the data member doxygen comments in this file in a follow up.

lldb/source/Symbol/Function.cpp
293 ↗(On Diff #276280)

Function::GetCallEdges() cannot recursively call itself, so a recursive_mutex isn't necessary. Actually if the mutex were taken recursively, that would allow the m_call_edges vector to be clobbered by the n-1 GetCallEdges callers who took the lock before the n-th caller.

This revision was automatically updated to reflect the committed changes.