[pull] main from google:main

call_graph.cc

@@ -20,11 +20,11 @@
 #include <cstddef>
 #include <cstdint>
 #include <numeric>
-#include <stdexcept>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "third_party/absl/log/check.h"
 #include "third_party/absl/log/log.h"
 #include "third_party/absl/status/status.h"
 #include "third_party/absl/strings/str_cat.h"
@@ -173,45 +173,42 @@ absl::Status CallGraph::Read(const BinExport2& proto,
   return absl::OkStatus();
 }
 
-void CallGraph::AttachFlowGraph(FlowGraph* flow_graph) {
-  if (!flow_graph) {
-    throw std::runtime_error(
-        "AttachFlowGraph: invalid flow graph (null pointer)");
-  }
-
-  auto entry_point_address = flow_graph->GetEntryPointAddress();
+absl::Status CallGraph::AttachFlowGraph(FlowGraph& flow_graph) {
+  auto entry_point_address = flow_graph.GetEntryPointAddress();
   auto vertex = GetVertex(entry_point_address);
   if (vertex == kInvalidVertex) {
-    throw std::runtime_error(absl::StrCat(
-        "AttachFlowGraph: couldn't find call graph node for flow graph ",
-        FormatAddress(entry_point_address)));
+    return absl::FailedPreconditionError(
+        absl::StrCat("AttachFlowGraph: couldn't find call graph node for flow "
+                     "graph ",
+                     FormatAddress(entry_point_address)));
   }
 
   if (graph_[vertex].flow_graph_ != nullptr) {
-    throw std::runtime_error(
+    return absl::FailedPreconditionError(
         absl::StrCat("AttachFlowGraph: flow graph already attached ",
                      FormatAddress(entry_point_address)));
   }
 
-  graph_[vertex].flow_graph_ = flow_graph;
-  flow_graph->SetCallGraph(this);
+  graph_[vertex].flow_graph_ = &flow_graph;
+  flow_graph.SetCallGraph(this);
+  return absl::OkStatus();
 }
 
-void CallGraph::DetachFlowGraph(FlowGraph* flow_graph) {
-  if (!flow_graph || flow_graph->GetCallGraph() != this) {
-    throw std::runtime_error("DetachFlowGraph: invalid graph");
+absl::Status CallGraph::DetachFlowGraph(FlowGraph& flow_graph) {
+  if (flow_graph.GetCallGraph() != this) {
+    return absl::InternalError("DetachFlowGraph: invalid graph");
   }
 
-  auto entry_point_address = flow_graph->GetEntryPointAddress();
-  auto vertex = GetVertex(entry_point_address);
-  if (vertex == kInvalidVertex) {
+  auto entry_point_address = flow_graph.GetEntryPointAddress();
+  if (auto vertex = GetVertex(entry_point_address); vertex == kInvalidVertex) {
     LOG(INFO) << absl::StrCat(
         "DetachFlowGraph: couldn't find call graph node for flow graph ",
         FormatAddress(entry_point_address));
   } else {
     graph_[vertex].flow_graph_ = nullptr;
   }
-  flow_graph->SetCallGraph(nullptr);
+  flow_graph.SetCallGraph(nullptr);
+  return absl::OkStatus();
 }
 
 CallGraph::Vertex CallGraph::GetVertex(Address address) const {

call_graph.h

@@ -18,6 +18,7 @@
 #include <boost/graph/compressed_sparse_row_graph.hpp>  // NOLINT
 #include <cstdint>
 #include <limits>
+#include <memory>
 #include <string>
 #include <utility>
 
@@ -80,16 +81,20 @@ class CallGraph {
   // A constant denoting a non-existent vertex.
   static constexpr Vertex kInvalidVertex = std::numeric_limits<Vertex>::max();
 
-  // Constructs an empty call graph.
-  CallGraph() = default;
-
-  virtual ~CallGraph() = default;
-
   // Reads and initializes the call graph from "proto". "filename" is passed in
   // and remembered for informational purposes only (we want to be able to
   // construct default save filenames with it for example).
+  static absl::StatusOr<std::unique_ptr<CallGraph>> FromProto(
+    const BinExport2& proto, const std::string& filename);
+
+  // Like FromProto, but initializes an existing (possibly empty) call graph.
   absl::Status Read(const BinExport2& proto, const std::string& filename);
 
+  // Constructs an empty call graph.
+  CallGraph() = default;
+
+  virtual ~CallGraph() = default;
+
   // Gets just the filename part (without path or extension) passed into Read().
   std::string GetFilename() const;
 
@@ -139,8 +144,9 @@ class CallGraph {
 
   // Associates the given flow graph with the corresponding call graph vertex.
   // The call graph will _not_ take ownership of the flow graph!
-  void AttachFlowGraph(FlowGraph* flow_graph);
-  void DetachFlowGraph(FlowGraph* flow_graph);
+  absl::Status AttachFlowGraph(FlowGraph& flow_graph);
+  absl::Status DetachFlowGraph(FlowGraph& flow_graph);
+
   // TODO(cblichmann): Remove!!!
   FlowGraph* GetFlowGraph(Address address) const;
   FlowGraph* GetFlowGraph(Vertex vertex) const {
@@ -200,8 +206,8 @@ class CallGraph {
 
   // Accesses comments. The call graph stores these globally even for operands
   // because we don't want to store them multiple times for shared basic blocks.
-  CommentsByOperatorId& GetComments() { return comments_; }
-  const CommentsByOperatorId& GetComments() const { return comments_; }
+  CommentsByOperatorId& comments() { return comments_; }
+  const CommentsByOperatorId& comments() const { return comments_; }
 
   // Reduces the graph to the immediate vicinity of "edge" and recalculates MD
   // indices on that subgraph. The idea is to become resilient against non-local
@@ -214,6 +220,8 @@ class CallGraph {
   void DeleteVertices(Address from, Address to);
 
  protected:
+  friend class CallGraphPeer;
+
   void Init();
   double CalculateProximityMdIndex(Edge edge);
 

call_graph_test.cc

@@ -17,7 +17,6 @@
 #include <boost/graph/compressed_sparse_row_graph.hpp>  // NOLINT
 #include <limits>
 #include <memory>
-#include <stdexcept>
 #include <string>
 #include <utility>
 
@@ -60,21 +59,13 @@ TEST(EmptyCallGraphTest, Construction) {
   call_graph.SetMdIndex(47.0);
   EXPECT_THAT(call_graph.GetMdIndex(), Eq(47.0));
 
-  EXPECT_THAT(call_graph.GetComments(), IsEmpty());
-}
-
-TEST(EmptyCallGraphTest, AddOrRemoveNullFlowGraphThrows) {
-  CallGraph call_graph;  // Empty
-
-  EXPECT_THROW(call_graph.AttachFlowGraph(nullptr), std::runtime_error);
-  EXPECT_THROW(call_graph.DetachFlowGraph(nullptr), std::runtime_error);
+  EXPECT_THAT(call_graph.comments(), IsEmpty());
 }
 
 TEST(EmptyCallGraphDeathTest, QueryingVerticesCrashes) {
   CallGraph call_graph;  // Empty
 
-  // These should fail in all builds
-  // TODO(cblichmann): Implement bound checks in debug mode.
+  // These should fail in all builds.
   EXPECT_DEATH_IF_SUPPORTED(call_graph.GetAddress(CallGraph::kInvalidVertex),
                             "");
   EXPECT_DEATH_IF_SUPPORTED(call_graph.GetMdIndex(CallGraph::kInvalidVertex),
@@ -84,21 +75,19 @@ TEST(EmptyCallGraphDeathTest, QueryingVerticesCrashes) {
 }
 
 TEST(EmptyCallGraphTest, CrossPlatformFileBasenames) {
-  class CallGraphForTesting : public CallGraph {
-   public:
-    void set_filename(std::string value) { filename_ = std::move(value); }
-  } call_graph;  // Empty
+  CallGraph call_graph;  // Empty
+  CallGraphPeer call_graph_peer(call_graph);
 
   // Plain filename
-  call_graph.set_filename("primary.v1.test.exe");
+  call_graph_peer.set_filename("primary.v1.test.exe");
   EXPECT_THAT(call_graph.GetFilename(), StrEq("primary.v1.test"));
 
   // Windows style
-  call_graph.set_filename(R"(C:\TEMP\RE.project\primary.v1.test.exe)");
+  call_graph_peer.set_filename(R"(C:\TEMP\RE.project\primary.v1.test.exe)");
   EXPECT_THAT(call_graph.GetFilename(), StrEq("primary.v1.test"));
 
   // Posix style
-  call_graph.set_filename(R"(/tmp/RE.project/primary.v1.test.exe)");
+  call_graph_peer.set_filename(R"(/tmp/RE.project/primary.v1.test.exe)");
   EXPECT_THAT(call_graph.GetFilename(), StrEq("primary.v1.test"));
 }
 
@@ -133,7 +122,7 @@ class SimpleCallGraphTest : public ::testing::Test {
                                      {InstructionBuilder("call func_a")
                                           .SetCallsFunction("func_a"),
                                       InstructionBuilder("ret")})})})
-                    .Build(&cache_)),
+                    .Build(cache_)),
         call_graph_(binary_->call_graph) {}
 
   Instruction::Cache cache_;

database_writer.cc

@@ -94,17 +94,17 @@ absl::Status ReadInfos(const std::string& filename, CallGraph& call_graph,
   for (const auto& flow_graph_proto : proto.flow_graph()) {
     // Create an ephemeral FlowGraph instance to update the instruction cache
     // and to use it to parse the BinExport2 information.
-    FlowGraph flow_graph;
-    ABSL_RETURN_IF_ERROR(flow_graph.Read(proto, flow_graph_proto, &call_graph,
-                                         &instruction_cache));
+    ABSL_ASSIGN_OR_RETURN(std::unique_ptr<FlowGraph> flow_graph,
+                          FlowGraph::FromProto(proto, flow_graph_proto,
+                                               call_graph, instruction_cache));
 
     Counts counts;
-    Count(flow_graph, &counts);
-    Address address = flow_graph.GetEntryPointAddress();
+    Count(*flow_graph, &counts);
+    Address address = flow_graph->GetEntryPointAddress();
     FlowGraphInfo& info = flow_graph_infos[address];
     info.address = address;
-    info.name = &flow_graph.GetName();
-    info.demangled_name = &flow_graph.GetDemangledName();
+    info.name = &flow_graph->GetName();
+    info.demangled_name = &flow_graph->GetDemangledName();
     info.basic_block_count = counts[Counts::kBasicBlocksLibrary] +
                              counts[Counts::kBasicBlocksNonLibrary];
     info.edge_count =

differ.cc

@@ -102,19 +102,13 @@ absl::Status AddSubsToCallGraph(CallGraph* absl_nonnull call_graph,
   for (auto [it, end] = boost::vertices(call_graph->GetGraph()); it != end;
        ++it) {
     const CallGraph::Vertex vertex = *it;
-    const Address address = call_graph->GetAddress(vertex);
     if (call_graph->GetFlowGraph(vertex)) {
       continue;
     }
+    const Address address = call_graph->GetAddress(vertex);
 
-    std::unique_ptr<FlowGraph> flow_graph;
-    // Temporary try-catch block. A follow-up change will refactor to
-    // absl::StatusOr<std::unique_ptr<FlowGraph>>.
-    try {
-      flow_graph = std::make_unique<FlowGraph>(call_graph, address);
-    } catch (const std::runtime_error& e) {
-      return absl::FailedPreconditionError(e.what());
-    }
+    ABSL_ASSIGN_OR_RETURN(std::unique_ptr<FlowGraph> flow_graph,
+                          FlowGraph::Create(*call_graph, address));
     call_graph->SetStub(vertex, true);
     call_graph->SetLibrary(vertex, true);
     if (!flow_graphs->insert(flow_graph.release()).second) {
@@ -135,10 +129,10 @@ absl::Status SetupGraphsFromProto(
     if (proto_flow_graph.basic_block_index_size() == 0) {
       continue;
     }
-    auto flow_graph = absl::make_unique<FlowGraph>();
-    ABSL_RETURN_IF_ERROR(flow_graph->Read(proto, proto_flow_graph, call_graph,
-                                          instruction_cache));
-
+    ABSL_ASSIGN_OR_RETURN(
+        std::unique_ptr<FlowGraph> flow_graph,
+        FlowGraph::FromProto(proto, proto_flow_graph, *call_graph,
+                             *instruction_cache));
     Counts counts;
     Count(*flow_graph, &counts);