i#5675 record filter: Improve interfaces.

clients/drcachesim/CMakeLists.txt

@@ -163,6 +163,7 @@ add_exported_library(drmemtrace_simulator STATIC
 add_exported_library(drmemtrace_record_filter STATIC
   tools/filter/record_filter.cpp
   tools/filter/cache_filter.h
+  tools/filter/cache_filter.cpp
   tools/filter/type_filter.h
   tools/filter/null_filter.h)
 target_link_libraries(drmemtrace_record_filter drmemtrace_simulator)
@@ -241,6 +242,7 @@ use_DynamoRIO_extension(drcachesim drutil_static)
 
 # This is to avoid ../ and common/ in the #includes of headers that we
 # export in a single dir for 3rd-party tool integration.
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/simulator)
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/common)
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/reader)
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/tracer)

clients/drcachesim/reader/compressed_file_reader.cpp

@@ -135,7 +135,6 @@ record_file_reader_t<gzip_reader_t>::~record_file_reader_t<gzip_reader_t>()
 {
     if (input_file_ != nullptr) {
         gzclose(input_file_->file);
-        delete input_file_;
     }
 }
 
@@ -147,15 +146,15 @@ record_file_reader_t<gzip_reader_t>::open_single_file(const std::string &path)
     if (!open_single_file_common(path, file))
         return false;
     VPRINT(this, 1, "Opened input file %s\n", path.c_str());
-    input_file_ = new gzip_reader_t(file);
+    input_file_ = std::unique_ptr<gzip_reader_t>(new gzip_reader_t(file));
     return true;
 }
 
 template <>
 bool
 record_file_reader_t<gzip_reader_t>::read_next_entry()
 {
-    if (!read_next_thread_entry_common(input_file_, &cur_entry_, &eof_))
+    if (!read_next_thread_entry_common(input_file_.get(), &cur_entry_, &eof_))
         return false;
     VPRINT(this, 4, "Read from file: type=%s (%d), size=%d, addr=%zu\n",
            trace_type_names[cur_entry_.type], cur_entry_.type, cur_entry_.size,

clients/drcachesim/reader/record_file_reader.cpp

@@ -41,19 +41,18 @@ template <>
 /* clang-format on */
 record_file_reader_t<std::ifstream>::~record_file_reader_t<std::ifstream>()
 {
-    if (input_file_ != nullptr)
-        delete input_file_;
 }
 
 template <>
 bool
 record_file_reader_t<std::ifstream>::open_single_file(const std::string &path)
 {
-    std::ifstream *fstream = new std::ifstream(path, std::ifstream::binary);
+    auto fstream =
+        std::unique_ptr<std::ifstream>(new std::ifstream(path, std::ifstream::binary));
     if (!*fstream)
         return false;
     VPRINT(this, 1, "Opened input file %s\n", path.c_str());
-    input_file_ = fstream;
+    input_file_ = std::move(fstream);
     return true;
 }
 

clients/drcachesim/reader/record_file_reader.h

@@ -39,6 +39,8 @@
 
 #include <assert.h>
 #include <iterator>
+#include <memory>
+
 #include "memtrace_stream.h"
 #include "reader.h"
 #include "trace_entry.h"
@@ -292,7 +294,7 @@ template <typename T> class record_file_reader_t : public record_reader_t {
         return true;
     }
 
-    T *input_file_ = nullptr;
+    std::unique_ptr<T> input_file_ = nullptr;
     std::string input_path_;
 
     bool

clients/drcachesim/tests/record_filter_unit_tests.cpp

@@ -72,9 +72,9 @@ static droption_t<std::string> op_tmp_output_dir(
 
 class test_record_filter_t : public dynamorio::drmemtrace::record_filter_t {
 public:
-    test_record_filter_t(const std::vector<record_filter_func_t *> &filters,
+    test_record_filter_t(std::vector<std::unique_ptr<record_filter_func_t>> filters,
                          uint64_t last_timestamp)
-        : record_filter_t("", filters, last_timestamp,
+        : record_filter_t("", std::move(filters), last_timestamp,
                           /*verbose=*/0)
     {
     }
@@ -130,9 +130,10 @@ local_create_dir(const char *dir)
 basic_counts_t::counters_t
 get_basic_counts(const std::string &trace_dir)
 {
-    analysis_tool_t *basic_counts_tool = new basic_counts_t(/*verbose=*/0);
+    auto basic_counts_tool =
+        std::unique_ptr<basic_counts_t>(new basic_counts_t(/*verbose=*/0));
     std::vector<analysis_tool_t *> tools;
-    tools.push_back(basic_counts_tool);
+    tools.push_back(basic_counts_tool.get());
     analyzer_t analyzer(trace_dir, &tools[0], static_cast<int>(tools.size()));
     if (!analyzer) {
         FATAL_ERROR("failed to initialize analyzer: %s",
@@ -141,9 +142,7 @@ get_basic_counts(const std::string &trace_dir)
     if (!analyzer.run()) {
         FATAL_ERROR("failed to run analyzer: %s", analyzer.get_error_string().c_str());
     }
-    basic_counts_t::counters_t counts =
-        ((basic_counts_t *)basic_counts_tool)->get_total_counts();
-    delete basic_counts_tool;
+    basic_counts_t::counters_t counts = basic_counts_tool->get_total_counts();
     return counts;
 }
 
@@ -214,7 +213,8 @@ test_cache_and_type_filter()
     for (int k = 0; k < 2; ++k) {
         auto stream = std::unique_ptr<local_stream_t>(new local_stream_t());
         // Construct record_filter_func_ts.
-        std::vector<dynamorio::drmemtrace::record_filter_t::record_filter_func_t *>
+        std::vector<
+            std::unique_ptr<dynamorio::drmemtrace::record_filter_t::record_filter_func_t>>
             filters;
         auto cache_filter =
             std::unique_ptr<dynamorio::drmemtrace::record_filter_t::record_filter_func_t>(
@@ -226,26 +226,26 @@ test_cache_and_type_filter()
                     cache_filter->get_error_string().c_str());
             return false;
         }
-        filters.push_back(cache_filter.get());
+        filters.push_back(std::move(cache_filter));
 
-        auto type_filter =
-            std::unique_ptr<dynamorio::drmemtrace::record_filter_t::record_filter_func_t>(
+        if (k == 0) {
+            auto type_filter = std::unique_ptr<
+                dynamorio::drmemtrace::record_filter_t::record_filter_func_t>(
                 new dynamorio::drmemtrace::type_filter_t({ TRACE_TYPE_ENCODING },
                                                          { TRACE_MARKER_TYPE_FUNC_ID,
                                                            TRACE_MARKER_TYPE_FUNC_RETADDR,
                                                            TRACE_MARKER_TYPE_FUNC_ARG }));
-        if (k == 0) {
             if (type_filter->get_error_string() != "") {
                 fprintf(stderr, "Couldn't construct a type_filter %s",
                         type_filter->get_error_string().c_str());
                 return false;
             }
-            filters.push_back(type_filter.get());
+            filters.push_back(std::move(type_filter));
         }
 
         // Construct record_filter_t.
         auto record_filter = std::unique_ptr<test_record_filter_t>(
-            new test_record_filter_t(filters, /*stop_timestamp_us=*/0xabcdee));
+            new test_record_filter_t(std::move(filters), /*stop_timestamp_us=*/0xabcdee));
         void *shard_data =
             record_filter->parallel_shard_init_stream(0, nullptr, stream.get());
         if (!*record_filter) {
@@ -324,18 +324,19 @@ test_null_filter()
     if (!local_create_dir(output_dir.c_str())) {
         FATAL_ERROR("Failed to create filtered trace output dir %s", output_dir.c_str());
     }
-
-    dynamorio::drmemtrace::record_filter_t::record_filter_func_t *null_filter =
-        new dynamorio::drmemtrace::null_filter_t();
-    std::vector<dynamorio::drmemtrace::record_filter_t::record_filter_func_t *>
+    auto null_filter =
+        std::unique_ptr<dynamorio::drmemtrace::record_filter_t::record_filter_func_t>(
+            new dynamorio::drmemtrace::null_filter_t());
+    std::vector<
+        std::unique_ptr<dynamorio::drmemtrace::record_filter_t::record_filter_func_t>>
         filter_funcs;
-    filter_funcs.push_back(null_filter);
-    record_analysis_tool_t *record_filter =
-        new dynamorio::drmemtrace::record_filter_t(output_dir, filter_funcs,
+    filter_funcs.push_back(std::move(null_filter));
+    auto record_filter = std::unique_ptr<dynamorio::drmemtrace::record_filter_t>(
+        new dynamorio::drmemtrace::record_filter_t(output_dir, std::move(filter_funcs),
                                                    /*stop_timestamp_us=*/0,
-                                                   /*verbosity=*/0);
+                                                   /*verbosity=*/0));
     std::vector<record_analysis_tool_t *> tools;
-    tools.push_back(record_filter);
+    tools.push_back(record_filter.get());
     record_analyzer_t record_analyzer(op_trace_dir.get_value(), &tools[0],
                                       static_cast<int>(tools.size()));
     if (!record_analyzer) {
@@ -346,8 +347,6 @@ test_null_filter()
         FATAL_ERROR("Failed to run record filter: %s",
                     record_analyzer.get_error_string().c_str());
     }
-    delete record_filter;
-    delete null_filter;
 
     basic_counts_t::counters_t c1 = get_basic_counts(op_trace_dir.get_value());
     basic_counts_t::counters_t c2 = get_basic_counts(output_dir);

clients/drcachesim/tools/filter/cache_filter.cpp

@@ -0,0 +1,108 @@
+/* **********************************************************
+ * Copyright (c) 2022 Google, Inc.  All rights reserved.
+ * **********************************************************/
+
+/*
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above copyright notice,
+ *   this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright notice,
+ *   this list of conditions and the following disclaimer in the documentation
+ *   and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Google, Inc. nor the names of its contributors may be
+ *   used to endorse or promote products derived from this software without
+ *   specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL VMWARE, INC. OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ */
+
+#include "cache_filter.h"
+#include "cache_lru.h"
+
+namespace dynamorio {
+namespace drmemtrace {
+
+class cache_filter_stats_t : public cache_stats_t {
+public:
+    cache_filter_stats_t(int block_size)
+        : cache_stats_t(block_size)
+        , did_last_access_hit_(false)
+    {
+    }
+    void
+    access(const memref_t &memref, bool hit, caching_device_block_t *cache_block) override
+    {
+        did_last_access_hit_ = hit;
+        cache_stats_t::access(memref, hit, cache_block);
+    }
+    // Returns whether the last access to the cache was a hit.
+    bool
+    did_last_access_hit()
+    {
+        return did_last_access_hit_;
+    }
+
+private:
+    bool did_last_access_hit_;
+};
+
+struct per_shard_t {
+    cache_lru_t cache;
+};
+
+void *
+cache_filter_t::parallel_shard_init(memtrace_stream_t *shard_stream)
+{
+    per_shard_t *per_shard = new per_shard_t;
+    if (!(per_shard->cache.init(cache_associativity_, cache_line_size_, cache_size_,
+                                nullptr, new cache_filter_stats_t(cache_line_size_),
+                                nullptr))) {
+        error_string_ = "Failed to initialize cache.";
+        return nullptr;
+    }
+    return per_shard;
+}
+bool
+cache_filter_t::parallel_shard_filter(const trace_entry_t &entry, void *shard_data)
+{
+    per_shard_t *per_shard = reinterpret_cast<per_shard_t *>(shard_data);
+    bool output = true;
+    // We don't process flush entries here.
+    if ((filter_data_ &&
+         (entry.type == TRACE_TYPE_READ || entry.type == TRACE_TYPE_WRITE ||
+          type_is_prefetch(static_cast<trace_type_t>(entry.type)))) ||
+        (filter_instrs_ && type_is_instr(static_cast<trace_type_t>(entry.type)))) {
+        memref_t ref;
+        ref.data.type = static_cast<trace_type_t>(entry.type);
+        ref.data.size = entry.size;
+        ref.data.addr = entry.addr;
+        per_shard->cache.request(ref);
+        output = !reinterpret_cast<cache_filter_stats_t *>(per_shard->cache.get_stats())
+                      ->did_last_access_hit();
+    }
+    return output;
+}
+bool
+cache_filter_t::parallel_shard_exit(void *shard_data)
+{
+    per_shard_t *per_shard = reinterpret_cast<per_shard_t *>(shard_data);
+    delete per_shard->cache.get_stats();
+    delete per_shard;
+    return true;
+}
+} // namespace drmemtrace
+} // namespace dynamorio

clients/drcachesim/tools/filter/cache_filter.h

@@ -34,35 +34,10 @@
 #define _CACHE_FILTER_H_ 1
 
 #include "record_filter.h"
-#include "../simulator/cache_lru.h"
 
 namespace dynamorio {
 namespace drmemtrace {
 
-class cache_filter_stats_t : public cache_stats_t {
-public:
-    cache_filter_stats_t(int block_size)
-        : cache_stats_t(block_size)
-        , did_last_access_hit_(false)
-    {
-    }
-    void
-    access(const memref_t &memref, bool hit, caching_device_block_t *cache_block) override
-    {
-        did_last_access_hit_ = hit;
-        cache_stats_t::access(memref, hit, cache_block);
-    }
-    // Returns whether the last access to the cache was a hit.
-    bool
-    did_last_access_hit()
-    {
-        return did_last_access_hit_;
-    }
-
-private:
-    bool did_last_access_hit_;
-};
-
 class cache_filter_t : public record_filter_t::record_filter_func_t {
 public:
     cache_filter_t(int cache_associativity, int cache_line_size, int cache_size,
@@ -75,51 +50,13 @@ class cache_filter_t : public record_filter_t::record_filter_func_t {
     {
     }
     void *
-    parallel_shard_init(memtrace_stream_t *shard_stream) override
-    {
-        per_shard_t *per_shard = new per_shard_t;
-        if (!(per_shard->cache.init(cache_associativity_, cache_line_size_, cache_size_,
-                                    nullptr, new cache_filter_stats_t(cache_line_size_),
-                                    nullptr))) {
-            error_string_ = "Failed to initialize cache.";
-            return nullptr;
-        }
-        return per_shard;
-    }
+    parallel_shard_init(memtrace_stream_t *shard_stream) override;
     bool
-    parallel_shard_filter(const trace_entry_t &entry, void *shard_data) override
-    {
-        per_shard_t *per_shard = reinterpret_cast<per_shard_t *>(shard_data);
-        bool output = true;
-        // We don't process flush entries here.
-        if ((filter_data_ &&
-             (entry.type == TRACE_TYPE_READ || entry.type == TRACE_TYPE_WRITE ||
-              type_is_prefetch(static_cast<trace_type_t>(entry.type)))) ||
-            (filter_instrs_ && type_is_instr(static_cast<trace_type_t>(entry.type)))) {
-            memref_t ref;
-            ref.data.type = static_cast<trace_type_t>(entry.type);
-            ref.data.size = entry.size;
-            ref.data.addr = entry.addr;
-            per_shard->cache.request(ref);
-            output =
-                !reinterpret_cast<cache_filter_stats_t *>(per_shard->cache.get_stats())
-                     ->did_last_access_hit();
-        }
-        return output;
-    }
+    parallel_shard_filter(const trace_entry_t &entry, void *shard_data) override;
     bool
-    parallel_shard_exit(void *shard_data) override
-    {
-        per_shard_t *per_shard = reinterpret_cast<per_shard_t *>(shard_data);
-        delete per_shard->cache.get_stats();
-        delete per_shard;
-        return true;
-    }
+    parallel_shard_exit(void *shard_data) override;
 
 private:
-    struct per_shard_t {
-        cache_lru_t cache;
-    };
     int cache_associativity_;
     int cache_line_size_;
     int cache_size_;