Page MenuHomePhabricator

DWARFContext: Make loading of sections thread-safe
ClosedPublic

Authored by labath on May 23 2019, 7:22 AM.

Details

Summary

SymbolFileDWARF used to load debug sections in a thread-safe manner.
When we moved to DWARFContext, we dropped the thread-safe part, because
we thought it was not necessary.

It turns out this was only mostly correct.

The "mostly" part is there because this is a problem only if we use the
manual index, as that is the only source of intra-module paralelism.
Also, this only seems to occur for extremely simple files (like the ones
I've been creating for tests lately), where we've managed to start
indexing before loading the debug_str section. Then, two threads start
to load the section simultaneously and produce wrong results.

On more complex files, something seems to be loading the debug_str section
before we start indexing, as I haven't been able to reproduce this
there, but I have not investigated what it is.

I've tried to come up with a test for this, but I haven't been able to
reproduce the problem reliably. Still, while doing so, I created a way
to generate many compile units on demand. Given that most of our tests
work with only one or two compile units, it seems like this could be
useful anyway.

Event Timeline

labath created this revision.May 23 2019, 7:22 AM

Two other options I see are:

  • initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.
  • have the manual index preload the sections it needs. it already does a bunch of preloading in order to speed up the access to everything, so this wouldn't look completely out of place there.
clayborg accepted this revision.May 23 2019, 7:29 AM
clayborg added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFContext.h
25

is llvm::once_flag better than std::once_flag?

This revision is now accepted and ready to land.May 23 2019, 7:29 AM

Two other options I see are:

  • initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.

I'd be fine with this.

  • have the manual index preload the sections it needs. it already does a bunch of preloading in order to speed up the access to everything, so this wouldn't look completely out of place there.

I like either your current solution or the load all on creation better that this,

labath marked an inline comment as done.May 24 2019, 12:40 AM

Two other options I see are:

  • initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.

I'd be fine with this.

Ok, so let's go with the current solution to restore status quo, and I'll return to this idea later.

source/Plugins/SymbolFile/DWARF/DWARFContext.h
25

Not really, but it's needed because std::once_flag does not work on some more exotic platforms. Elsewhere, it's equivalent to std::once_flag.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 1:01 AM

Two other options I see are:

  • initialize the sections immediately after creating the dwarf context. The main advantage of that would that it alings to code more with llvm (which also loads the sections up-front), and slighly faster subsequent accesses to the debug info. I don't think this should negatively impact the start up time, as the files are mmapped anyway, and so the "loading" will consist of some basic pointer arithmetic. Also, the SymbolFileDWARF object as a whole is created lazily, so the fact that it is being created means that somebody is going to access it immediately after that. And he cannot do anything with the symbol file without touching at least the debug_info section, which accounts for about 80% of all debug info.

I'd be fine with this.

Ok, so let's go with the current solution to restore status quo, and I'll return to this idea later.

I've realized that this may negatively affect the modules which are being read from process memory (this does not really work for ELF or PECOFF, but MachO implements it, and loads sections lazily). Given that it also seems to be possible to create the llvm DWARFContext with a custom implementation of DWARFObject (which could probably implement lazy loading, if needed), this does not seem to be that important right now. So, I've decided to shelve the idea for the time being.