From 5eb68f977265494321d7b7b60299ac2e56adde0e Mon Sep 17 00:00:00 2001
From: Andrei Homescu <ah@immunant.com>
Date: Fri, 10 May 2024 01:05:50 -0700
Subject: [PATCH] Add a set of comments and question on PDG construction

---
 pdg/src/builder.rs | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/pdg/src/builder.rs b/pdg/src/builder.rs
index e8b30ef3e4..6ec9bc488f 100644
--- a/pdg/src/builder.rs
+++ b/pdg/src/builder.rs
@@ -44,6 +44,7 @@ impl EventKindExt for EventKind {
         use EventKind::*;
         Some(match *self {
             CopyPtr(lhs) => lhs,
+            // Question: this takes a pointer as input and produces another?
             Field(ptr, ..) => ptr,
             Free { ptr } => ptr,
             Ret(ptr) => ptr,
@@ -54,10 +55,12 @@ impl EventKindExt for EventKind {
             StoreValue(ptr) => ptr,
             CopyRef => return None, // FIXME
             ToInt(ptr) => ptr,
+            // Question: this has two pointers, is the input one sufficient?
             Realloc { old_ptr, .. } => old_ptr,
             FromInt(lhs) => lhs,
             Alloc { ptr, .. } => ptr,
             AddrOfLocal(lhs, _) => lhs,
+            // Question: same, is the base pointer enough?
             Offset(ptr, _, _) => ptr,
             Done | BeginFuncBody => return None,
         })
@@ -120,6 +123,7 @@ fn update_provenance(
             }
         }
         Realloc { new_ptr, .. } => {
+            // Nit: might be worth removing the old pointer, just in case
             provenances.insert(new_ptr, mapping);
         }
         Offset(_, _, new_ptr) => {
@@ -131,6 +135,9 @@ fn update_provenance(
         AddrOfLocal(ptr, _) => {
             provenances.insert(ptr, mapping);
         }
+        // Question: it feels like Free should remove a pointer?
+        // It might not matter much since the next Alloc with
+        // the same address will overwrite the map entry
         _ => {}
     }
 }
@@ -163,15 +170,33 @@ pub fn add_node(
         statement_idx = 0;
     }
 
+    // Question: is "provenance" exactly tied to the memory address of this pointer?
+    // Why don't we just use that for everything?
+    // Answer attempt: provenance only covers the original
+    // source of the pointer, and not any copies like LoadValue
     let provenance = event
         .kind
         .ptr(event_metadata)
         .and_then(|ptr| provenances.get(&ptr).cloned());
+
+    // Question 1: why do we need another source (or even two of them)?
+    // Answer attempt: "provenance" returns the original source of the
+    // pointer; direct_source covers the last time this pointer was
+    // assigned to this specific local?
+    //
+    // Question 2: if that is the case, could this be simpler, e.g.,
+    // by using another hash table like "provenances" that covers
+    // all event kinds?
+    //
+    // Question 3: shouldn't the second argument be _first_nid_ref?
     let direct_source = provenance.and_then(|(gid, _last_nid_ref)| {
         graphs.graphs[gid]
             .nodes
             .iter()
             .rposition(|n| {
+                // Question/nitpick: event_metadata is captured here, so it
+                // will be constant for the entire iteration; if event_metadata
+                // is None, we could skip this whole thing?
                 if let (Some(d), Some(s)) = (&n.dest, &event_metadata.source) {
                     d == s
                 } else {
@@ -181,13 +206,21 @@ pub fn add_node(
             .map(|nid| (gid, NodeId::from(nid)))
     });
 
+    // Question: why do we need a third source?
+    // How is this different from direct_source?
     let source = direct_source.or_else(|| {
         event_metadata.source.as_ref().and_then(|src| {
+            // Question: will this always return a node from the current graph,
+            // always from a different graph, or a mix or both?
+            //
+            // Question: why would the latest assignment be in a different graph?
             let latest_assignment = graphs.latest_assignment.get(&(src_fn, src.local)).cloned();
             if !src.projection.is_empty() {
                 if let Some((gid, _)) = latest_assignment {
+                    // Question: why only the last element?
                     if let Some((nid, n)) = graphs.graphs[gid].nodes.iter_enumerated().rev().next()
                     {
+                        // Question: why does the field id not matter here?
                         if let NodeKind::Field(..) = n.kind {
                             return Some((gid, nid));
                         }
@@ -195,11 +228,18 @@ pub fn add_node(
                 }
             }
 
+            // Question: what's the logic behind these specific conditions?
+            // Why is AddrOfLocal an exception here?
             if !matches!(event.kind, EventKind::AddrOfLocal(..)) && src.projection.is_empty() {
                 latest_assignment
             } else if let EventKind::Field(..) = event.kind {
+                // Note: we get here if (matches!(AddrOfLocal) || !src.projection.is_empty())
+                // Question: why latest_assignment for fields,
+                // but provenance for everything else?
                 latest_assignment
             } else {
+                // Nit: could we return None here?
+                // Since we have .or(provenance) anyway
                 provenance
             }
         })
@@ -223,7 +263,9 @@ pub fn add_node(
         info: None,
     };
 
+    // Question: why this exact priority?: source, then direct_source, then provenance
     let graph_id = source
+        // Question: if source == None, then wouldn't direct_source also be None?
         .or(direct_source)
         .or(provenance)
         .and_then(|p| parent(&node_kind, p))
@@ -245,6 +287,11 @@ pub fn add_node(
         if let Some(last @ (last_gid, last_nid)) =
             graphs.latest_assignment.insert(unique_place, last_setting)
         {
+            // Question 1: the logic here seems to be "prefer a
+            // non-projection over a projection"; why?
+            //
+            // Question 2: what about two different projections?
+            // Is it fine for the later one to overwrite the earlier?
             if !dest.projection.is_empty()
                 && graphs.graphs[last_gid].nodes[last_nid]
                     .dest