Page MenuHomePhabricator

[asan_symbolize] Add a simple plugin architecture

Authored by delcypher on Apr 10 2019, 12:23 PM.



This change adds a simple plugin architecture to
The motivation here is that sometimes it's necessary to perform extra
work to figure out where binaries with debug symbols can actually be
found. For example it might be the case that a remote service needs
to be queried for binaries and then copied to the local system.

This "extra work" can be extremely site-specific such that adding the
code directly into the would just clutter the code
for a very niche use case. To avoid this, the can
now load external code via a new --plugins command line option.

These plugins are loaded before main command line argument parsing so
that they can add their own command line options.

Right now the only hook into the behaviour of symbolization is the
filter_binary_path() function which assumes a very similar role
to the binary_name_filter function that was previously in the code.
We can add more hooks as necessary.

Code in the script does not call plugin code
directly. Instead it uses a AsanSymbolizerPlugInProxy object.
This object

  • Loads plugins from files.
  • Manages the lifetime of the plugins.
  • Provides an interface for calling into plugin functions and handles calling into multiple plugins.

To unify the way binary paths are filtered the old sysroot_path_filter
function (and associated code) has been turned into a simple plugin
(SysRootFilterPlugIn) that is always loaded. The plugin unloads
itself if the -s option is not present on the command line. Users
should not see any functional change relating to this command line

Some simple tests are provided to illustrate what plugin code looks
like and also to check the functionality continues to work.


Diff Detail


Event Timeline

delcypher created this revision.Apr 10 2019, 12:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 10 2019, 12:23 PM
Herald added subscribers: Restricted Project, srhines. · View Herald Transcript
delcypher updated this revision to Diff 194588.Apr 10 2019, 2:25 PM

Use cstdlib rather than stdlib.h

vitalybuka accepted this revision.Apr 16 2019, 2:11 PM
vitalybuka added inline comments.
631 ↗(On Diff #194588)

why do you need to extract -s into plugin?

This revision is now accepted and ready to land.Apr 16 2019, 2:11 PM
delcypher marked an inline comment as done.Apr 16 2019, 4:03 PM
delcypher added inline comments.
631 ↗(On Diff #194588)

We don't need to do this, but there is a reason I did this.

I did this to keep the logic in symbolize_address() simple, i.e. we just have this:

binary = self.plugin_proxy.filter_binary_path(binary)

So all binary path filtering goes through the plugin_proxy, i.e. the sys_root filter is not a special case that has to be handled inside symbolize_address(). There doesn't seem to be anything special about the sys root filter so treating it as a special case seems unnecessary.

I personally prefer the SysRootFilterPlugIn class because it keeps all the sys root filter stuff together in a small class. It comes at the cost of readability though because anyone reading the code has to roughly understand how the plugin interface works, whereas previously this was not a requirement.

There is a trade-off here and I've picked the one I prefer. You might have a different opinion.

delcypher marked an inline comment as done.Apr 17 2019, 1:25 AM
delcypher added inline comments.
631 ↗(On Diff #194588)

@vitalybuka Does that answer your question? I see you've approved this patch but I want to be sure you're happy with everything before I land it.

This revision was automatically updated to reflect the committed changes.

Looks like the build broke some bots ( It seems the code doesn't work properly in some really old versions of Python 2. I've managed to reproduce the issue using Python 2.7.6 and I've landed a fix