[Sample Profile Loader] Fix potential invalidated reference #73181
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-pgo Author: William Junda Huang (huangjd) ChangesThere is a potential issue in ProfiledCallGraph where pointers to ProfiledCallGraphNode are used to construct edges, while ProfiledCallGraphNode instances are being added to a hash map ProfiledFunctions simultaneously. If rehash happens, those pointers are invalidated, resulting in undefined behavior/crash. Previously (before md5phase2) ProfiledFunctions is a llvm::StringMap, which also have the same issue theoretically when rehashing but was not observed. This patch fixes this potential issue by using a backing buffer for ProrfiledCallGraphNode that does not relocate. Full diff: https://github.com/llvm/llvm-project/pull/73181.diff 1 Files Affected:
diff --git a/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h b/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h
index 3f986caeb547287..3d844cc92aa0a8e 100644
--- a/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h
+++ b/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h
@@ -140,8 +140,11 @@ class ProfiledCallGraph {
if (!ProfiledFunctions.count(Name)) {
// Link to synthetic root to make sure every node is reachable
// from root. This does not affect SCC order.
- ProfiledFunctions[Name] = ProfiledCallGraphNode(Name);
- Root.Edges.emplace(&Root, &ProfiledFunctions[Name], 0);
+ // Store the pointer of the node because the map can be rehashed.
+ auto &Node =
+ ProfiledCallGraphNodeList.emplace_back(ProfiledCallGraphNode(Name));
+ ProfiledFunctions[Name] = &Node;
+ Root.Edges.emplace(&Root, ProfiledFunctions[Name], 0);
}
}
@@ -152,9 +155,9 @@ class ProfiledCallGraph {
auto CalleeIt = ProfiledFunctions.find(CalleeName);
if (CalleeIt == ProfiledFunctions.end())
return;
- ProfiledCallGraphEdge Edge(&ProfiledFunctions[CallerName],
- &CalleeIt->second, Weight);
- auto &Edges = ProfiledFunctions[CallerName].Edges;
+ ProfiledCallGraphEdge Edge(ProfiledFunctions[CallerName],
+ CalleeIt->second, Weight);
+ auto &Edges = ProfiledFunctions[CallerName]->Edges;
auto EdgeIt = Edges.find(Edge);
if (EdgeIt == Edges.end()) {
Edges.insert(Edge);
@@ -193,7 +196,7 @@ class ProfiledCallGraph {
return;
for (auto &Node : ProfiledFunctions) {
- auto &Edges = Node.second.Edges;
+ auto &Edges = Node.second->Edges;
auto I = Edges.begin();
while (I != Edges.end()) {
if (I->Weight <= Threshold)
@@ -205,8 +208,9 @@ class ProfiledCallGraph {
}
ProfiledCallGraphNode Root;
- HashKeyMap<std::unordered_map, FunctionId, ProfiledCallGraphNode>
- ProfiledFunctions;
+ // backing buffer for ProfiledCallGraphNodes.
+ std::list<ProfiledCallGraphNode> ProfiledCallGraphNodeList;
+ llvm::DenseMap<FunctionId, ProfiledCallGraphNode*> ProfiledFunctions;
};
} // end namespace sampleprof
|
You can test this locally with the following command:git-clang-format --diff 227654e87117d7705c78e0ad1d4d9261bbc6af55 b10add58c92716b52027f229c73da6920c8cf1f0 -- llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.hView the diff from clang-format here.diff --git a/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h b/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h
index 5381ada37f..eb1e69b718 100644
--- a/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h
+++ b/llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h
@@ -155,8 +155,8 @@ private:
auto CalleeIt = ProfiledFunctions.find(CalleeName);
if (CalleeIt == ProfiledFunctions.end())
return;
- ProfiledCallGraphEdge Edge(ProfiledFunctions[CallerName],
- CalleeIt->second, Weight);
+ ProfiledCallGraphEdge Edge(ProfiledFunctions[CallerName], CalleeIt->second,
+ Weight);
auto &Edges = ProfiledFunctions[CallerName]->Edges;
auto EdgeIt = Edges.find(Edge);
if (EdgeIt == Edges.end()) {
@@ -210,7 +210,7 @@ private:
ProfiledCallGraphNode Root;
// backing buffer for ProfiledCallGraphNodes.
std::list<ProfiledCallGraphNode> ProfiledCallGraphNodeList;
- HashKeyMap<llvm::DenseMap, FunctionId, ProfiledCallGraphNode*>
+ HashKeyMap<llvm::DenseMap, FunctionId, ProfiledCallGraphNode *>
ProfiledFunctions;
};
|
There's no general solution, as this is a "feature" in C++ that the user is responsible for checking that. I searched other usages of hash maps in SampleProfile* to make sure there's no more use case like that (or use map which does not invalidate reference). The problem can be detected with memory sanitizer. Developers should be careful especially working with graphs, because it is likely to involve pointers onto a map. |
This is again the consequence of you breaking reference stability with the new container. Before your change, there is no issue with StringMap -- it guarantees reference stability and rehash does not cause the underlying StringMapEntryBase to be reallocated. The code works with the assumption of reference stability. |
| ProfiledFunctions; | ||
| // backing buffer for ProfiledCallGraphNodes. | ||
| std::list<ProfiledCallGraphNode> ProfiledCallGraphNodeList; | ||
| llvm::DenseMap<FunctionId, ProfiledCallGraphNode*> ProfiledFunctions; |
There was a problem hiding this comment.
Why moving away from HashKeyMap? Don't you still want the unification between MD5 and string name in this case?
In all fairness, the old code was probably written without reference stability in mind -- the StringMap might be implemented without that guarantee. I think making code not depending on that behavior can be more robust and immune to breakage in the future. |
I'm pretty sure that's not the case. FWIW, I remember instances of such cases where originally in prototype backing container is used just so we can have reference stability. But such backing container was later removed/optimized away because other container provide stability guarantee. As for guarantees, LLVM is fairly explicit, document points out DenseMap doesn't have reference stability, and StringMap has it. My main point is, certain changes are just riskier, and removing reference stability is one of them. Breakage is fine, let's just acknowledge it and fix forward, rather than keep trying to attribute breakages to some "existing bug". |
Many good points, thanks. DenseMap does document invalidation of iterator on insertion, but StringMap does not document the opposite (though that property can be inferred from the implementation?): https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h Having a backing container that guarantees the behavior (and with good unit test for it) to support exposing element address sounds like a good idea if it is so desired program pattern, but removing such dependency might be better. |
After looking into the source code StringMap's implementation seems to have reference stability, however it is neither explicitly mentioned in (https://llvm.org/docs/ProgrammersManual.html) nor clearly stated in the source code's comments. In this case I think it's better to use a backing container, in case the implementation of the map is changed. |
There is a potential issue in ProfiledCallGraph where pointers to ProfiledCallGraphNode are used to construct edges, while ProfiledCallGraphNode instances are being added to a hash map ProfiledFunctions simultaneously. If rehash happens, those pointers are invalidated, resulting in undefined behavior/crash. Previously (before md5phase2) ProfiledFunctions is a llvm::StringMap, which also have the same issue theoretically when rehashing but was not observed. This patch fixes this potential issue by using a backing buffer for ProrfiledCallGraphNode that does not relocate.