[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