[PATCH] [compiler-rt] Implement AddressSanitizer suppressions
Alexey Samsonov
vonosmas at gmail.com
Wed Nov 26 11:51:57 PST 2014
First part of comments is about SuppressionContext() extension. I think this should go in as a separate change, before we use this functionality in ASan suppressions.
================
Comment at: lib/sanitizer_common/sanitizer_suppressions.h:47
@@ -43,3 +46,3 @@
void Parse(const char *str);
bool Match(const char* str, SuppressionType type, Suppression **s);
uptr SuppressionCount() const;
----------------
Now you can add fast path to SuppressionContext::Match() - if there are no suppressions for a given type, just return false.
================
Comment at: lib/sanitizer_common/sanitizer_suppressions.h:49
@@ -45,2 +48,3 @@
uptr SuppressionCount() const;
+ bool IsSuppressionTypeActive(SuppressionType type) const;
const Suppression *SuppressionAt(uptr i) const;
----------------
I'd prefer this to be named HasSuppressonType().
================
Comment at: lib/sanitizer_common/sanitizer_suppressions.h:61
@@ -56,2 +60,3 @@
private:
- SuppressionContext() : suppressions_(1), can_parse_(true) {}
+ SuppressionContext() : suppressions_(1), suppression_type_active_(),
+ can_parse_(true) {}
----------------
I'm slightly worried about default initialization for the array. We try to avoid compiler from inserting memset() into runtime code. Consider zero-initializating the array with the call to `internal_memset` (it makes sense to move ctor to .cc file in this case).
================
Comment at: lib/sanitizer_common/sanitizer_suppressions.h:64
@@ -58,2 +63,3 @@
InternalMmapVector<Suppression> suppressions_;
+ bool suppression_type_active_[SuppressionTypeCount];
bool can_parse_;
----------------
bool has_suppresson_type_[SuppressionTypeCount];
================
Comment at: lib/sanitizer_common/tests/sanitizer_suppressions_test.cc:84
@@ -77,3 +83,3 @@
class SuppressionContextTest : public ::testing::Test {
public:
----------------
Please add unit test for the new public method (`SuppressionContext::HasSuppressionType`).
http://reviews.llvm.org/D6280
More information about the llvm-commits
mailing list