Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a set of comments and question on PDG construction #1092

Closed
wants to merge 1 commit into from
+47 −0
Closed

Add a set of comments and question on PDG construction #1092

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions pdg/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +47 to 48
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: this takes a pointer as input and produces another?

That's what I would expect. In the static analysis, we have a similar notion of PlaceElem::Field as a "pointer projection" that takes a pointer to the struct and produces a pointer to the field. This is similar to (some uses of) offset, which takes a pointer to (the first element of) an array and produces a pointer to an element, and also similar to LLVM's GetElementPtr operation.

Free { ptr } => ptr,
Ret(ptr) => ptr,
Expand All @@ -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,
Comment on lines +63 to 64
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the base pointer enough?

At one point, we planned to distinguish forward offsets (OFFSET_ADD) from backward ones (OFFSET_SUB). Offsets that statically only go forward can be implemented on slices with a subslice operation, but offsets that might go backward need a different type that tracks how far back it can go and remain in bounds.

Done | BeginFuncBody => return None,
})
Expand Down Expand Up @@ -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) => {
Expand All @@ -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
_ => {}
}
}
Expand Down Expand Up @@ -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
Comment on lines +173 to +176
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an answer for this case, but in general it's possible for two pointers to have the same address but different provenance. In C:

int x;
int y;
int* p = &x + 1;
int* q = &y;

It's possible that p == q, but q has &y's provenance and is legal to dereference, while p has &x's provenance and dereferencing it is UB (it's out of bounds for the object it points to).

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 {
Expand All @@ -181,25 +206,40 @@ 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));
}
}
}
}

// 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
}
})
Expand All @@ -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))
Expand All @@ -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
Expand Down
Loading