[Lldb-commits] [PATCH] D114288: [NFC] Refactor symbol table parsing.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D114288#3152709 <https://reviews.llvm.org/D114288#3152709>, @clayborg wrote:

> Do all .dwo files claim the main executable that references them is their owning module?

The short answer is "yes".

The reason for that is that the dwo files do not constitute a module in the normal sense (not even in the sense that is used by DebugMapModule in SymbolFileDWARFDebugMap). In contains some debug info, but only a part of it, and that part is meaningless without the information in the main file. Like, you can (syntactically) parse the DIE tree, but you won't be able to resolve any of the addresses contained therein. In an DebugMap module, the addresses will resolve to sections of the .o file, and then there's a separate fixup step (independent of dwarf) to remap them to the main file. OTOH, in a dwo file you just get nothing.

*However*, dwo files are still regular (elf) object files, and the object file component can be parsed independently of anything else. In this sense, it would be reasonable to create a separate module for them, to ensure their isolation. In fact, we currently need to jump through some hoops to ensure that the dwo object information does not leak into the main module (see the update_module_section_list argument to ObjectFile::GetSectionList). Having a separate Module object would remove the need for that.

Overall, in either case (a separate module or not) some part of the interface will end up being weird (a separate module is better for the object file class, a single module for the symbol file). It's possible the overall messiness would be reduced in the separate module version, but it's hard to say without trying to implement that.

However, maybe we don't figure that out now. See the inline comment.



================
Comment at: lldb/source/Symbol/ObjectFile.cpp:723
+Symtab *ObjectFile::GetSymtab() {
+  // std::unique_ptr is thread safe to access. If it is already set, then we
+  // can hand out the existing pointer. The symbol table has it's own mutex
----------------
labath wrote:
> This is not true. unique_ptr makes no guarantees about the safety of concurrent accesses to a single unique_ptr object. While this may work on current hardware (it will most likely work on x86), it will make tsan very unhappy.
That said, I think the fix could be as simple removing these two lines. call_once already has a fast path for already-initialized case, anyway, and going through the call_once function would ensure that a unique_ptr read cannot happen concurrently with any write to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114288