This is an archive of the discontinued LLVM Phabricator instance.

[Support] move FileCollector from LLDB to llvm/Support
ClosedPublic

Authored by arphaman on Jul 24 2019, 11:29 AM.

Details

Summary

The file collector class is useful for creating reproducers, not just for LLDB, but for other tools. That's why it should live in LLVM. I'm planning to use it in Clang to generate reproducers for clang-scan-deps.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jul 24 2019, 11:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 24 2019, 11:29 AM
arphaman updated this revision to Diff 211591.Jul 24 2019, 12:56 PM

Small bug fix.

jkorous added inline comments.Jul 24 2019, 1:00 PM
llvm/include/llvm/Support/FileCollector.h
40 ↗(On Diff #211574)

TLDR: private?

I'm just wondering if we could make the class safer or the correct use more obvious for classes deriving from it (if we want to support that).

The couple protected members and methods seem suggesting it's fine to use these from a derived class implementation. IIUC m_mutex is guarding m_vfs_writer, m_seen and m_symlink_map and while it is being locked in implementation of public method AddFile, protected methods seem to be assuming their callers lock the mutex. Specifically there's a potential for a data race in AddFileImpl (calling GetRealPath which uses m_symlink_map) and in AddFileToMapping if either is called directly from a derived class.

Since lldb_private::FileCollector seems to be using only the public interface we might just declare the above private (and maybe add a doc comment when caller of a method is expected to lock the mutex)?

arphaman updated this revision to Diff 211603.Jul 24 2019, 2:42 PM

Use private as suggested by Jan.

JDevlieghere accepted this revision.Jul 24 2019, 3:23 PM
This revision is now accepted and ready to land.Jul 24 2019, 3:23 PM
This revision was automatically updated to reflect the committed changes.
lldb/trunk/source/Utility/FileCollector.cpp