Hi,
I was exploring the library's usage in my project.
I discovered the hard way that creating nested passes can cause iterator invalidation on m_passNodes and weird memory corruptions.
For example with a stupid naive approach one could have:
fg.addCallbackPass<ParentData>(
"Parent",
[&](FrameGraph::Builder& builder, auto& data) {
// ...
fg.addCallbackPass<ChildData>(
"Child",
[&](FrameGraph::Builder& builder, auto& data) {
// ...
},
[=](const auto&, const FrameGraphPassResources&, void* ctx) {
// ...
});
},
[=](const auto&, const FrameGraphPassResources&, void* ctx) {
// ...
});
I believe that the philosophy behind this library is probably not to have nested passes and that the choice behind a vector is for data locality reasons. But maybe that's an overlook and there's no hard rules against nested passes.
In any case, I believe this would be interesting to either fix the situation by using a safer container such as std::list or add some debug checks to ensure that the caller will never attempt to create nested passes. This would save up some headaches and can be simply implemented with something like that:
diff --git a/include/fg/FrameGraph.inl b/include/fg/FrameGraph.inl
index bd05c73..2a91f99 100644
--- a/include/fg/FrameGraph.inl
+++ b/include/fg/FrameGraph.inl
@@ -14,11 +14,14 @@ inline const Data &FrameGraph::addCallbackPass(const std::string_view name,
"Invalid exec callback");
static_assert(sizeof(Execute) < 1024, "Execute captures too much");
+ assert(m_numNestedPasses == 0u && "Creating nested passes is not allowed!");
+ m_numNestedPasses++;
auto *pass = new FrameGraphPass<Data, Execute>(std::forward<Execute>(exec));
auto &passNode =
_createPassNode(name, std::unique_ptr<FrameGraphPass<Data, Execute>>(pass));
Builder builder{*this, passNode};
std::invoke(setup, builder, pass->data);
+ m_numNestedPasses--;
return pass->data;
}
Thanks!
Hi,
I was exploring the library's usage in my project.
I discovered the hard way that creating nested passes can cause iterator invalidation on
m_passNodesand weird memory corruptions.For example with a stupid naive approach one could have:
fg.addCallbackPass<ParentData>( "Parent", [&](FrameGraph::Builder& builder, auto& data) { // ... fg.addCallbackPass<ChildData>( "Child", [&](FrameGraph::Builder& builder, auto& data) { // ... }, [=](const auto&, const FrameGraphPassResources&, void* ctx) { // ... }); }, [=](const auto&, const FrameGraphPassResources&, void* ctx) { // ... });I believe that the philosophy behind this library is probably not to have nested passes and that the choice behind a vector is for data locality reasons. But maybe that's an overlook and there's no hard rules against nested passes.
In any case, I believe this would be interesting to either fix the situation by using a safer container such as
std::listor add some debug checks to ensure that the caller will never attempt to create nested passes. This would save up some headaches and can be simply implemented with something like that:Thanks!