[PATCH] [compiler-rt] Spring cleaning of compiler-rt symbolizers

Alexey Samsonov vonosmas at gmail.com
Tue Feb 24 11:58:26 PST 2015


Thanks for working on this! Let's work on landing this patch in pieces before moving on to http://reviews.llvm.org/D6588. The main comment is: this patch is still way too large to digest. Let's try to split it into bite-size pieces and land them one by one (move Extract routines, introduce and move SymbolizerProcess to header, change the interface, etc. For now a few high-level comments:

- think of splitting the code in sanitizer_symbolizer_posix_libcdep.cc into more files, or moving it around. Some code shouldn't really be POSIX-specific (e.g. Extract routines).
- think of adding more headers. For example, sanitizer_symbolizer.h is the header used by actual sanitizers, and Symbolizer is the class they are interested in. In the meantime, SymbolizerInterface (btw. the naming is really poor, considering we already have Symbolizer, maybe SymbolizerTool?) is an implementation detail, and could be hidden in another header.
- I'm fine with general approach "use the chain of symbolizers - try the first one, fallback to the next if necessary, then to the next etc.". But this approach then should be shared between POSIX and Windows - that is, WinSymbolizer will be no different from, say, InternalSymbolizer - it will be just another symbolizer in a chain. In particular, probably Symbolizer class will not even need to be abstract, and you could move all the logic to common sources.
- please re-use IntrusiveList instead of re-implementing linked lists, if possible.

SymbolizeData family of methods is really confusing. We should have renamed it to SymbolizeGlobal long ago (same for DataInfo -> GlobalInfo), I will do it soon.


http://reviews.llvm.org/D7827

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list