Fix dangling tracker references in early-return recursive functions - #1928
Fix dangling tracker references in early-return recursive functions#1928Vedant2005goyal wants to merge 1 commit into
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
e4636ad to
a944db5
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a944db5 to
38e081e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
38e081e to
1148829
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1148829 to
6faf82e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
6faf82e to
213072a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
213072a to
c87809b
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
c87809b to
bc9afde
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
bc9afde to
9e43e4d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| @@ -0,0 +1,54 @@ | |||
| // RUN: %cladclang -std=c++17 -O0 -I%S/../../include/ %s -o %t | |||
There was a problem hiding this comment.
That test does not feel complete. Here is what I think we will need:
--- a/test/Gradient/RecursiveFunctionEarlyReturn.C
+++ b/test/Gradient/RecursiveFunctionEarlyReturn.C
@@
-// RUN: %cladclang -std=c++17 -O0 -I%S/../../include/ %s -o %t
-// RUN: %t
+// RUN: %cladclang -std=c++17 -O0 -I%S/../../include/ %s -o %t 2>&1 | %filecheck %s
+// RUN: %t | %filecheck_exec %s
#include "clad/Differentiator/Differentiator.h"
+#include <cstdio>
#include <vector>
@@
-// CHECK: void func_grad(
+// CHECK: void func_grad_0_2(
// CHECK: clad::restore_tracker _tracker0 = {};
// CHECK: auto _rev0 = [&] {
// CHECK: _tracker0.restore();
@@ (in main, after d_fn.execute)
+ printf("d_A = {%.2f, %.2f, %.2f}\n", d_A[0], d_A[1], d_A[2]);
+ // CHECK-EXEC: d_A = {74.00, 13.00, 2.00}
+ printf("d_v = {%.2f}\n", d_v[0]);
+ // CHECK-EXEC: d_v = {6.00}
@@ (after d_func1.execute)
+ printf("d_x = %.2f\n", d_x); // func1(x,3) = 8x
+ // CHECK-EXEC: d_x = 8.00
-}
\ No newline at end of file
+}| // CHECK-NEXT: clad::restore_tracker _tracker0 = {}; | ||
| // CHECK-NEXT: MyStructWrapper _d_s = {{.*0., 0..*}}; | ||
| // CHECK-NEXT: MyStructWrapper s; | ||
| // CHECK-NEXT: clad::restore_tracker _tracker0 = {}; |
There was a problem hiding this comment.
These tests do not have early returns and yet we change them -- we should sccope the change only to things with early returns.
There was a problem hiding this comment.
Thanks @vgvassilev for the review.
I restored those tests to their original form
| // CHECK-NEXT: float _t3; | ||
| // CHECK-NEXT: float _t4; | ||
| // CHECK-NEXT: float _t3 = 0.F; | ||
| // CHECK-NEXT: float _t4 = 0.F; |
There was a problem hiding this comment.
Why do we need to initialize these?
There was a problem hiding this comment.
They were done so to prevent segmentation fault in CI's since Hessians inherently use early returns for their reverse sweeps.
There was a problem hiding this comment.
That seems like a potentially bigger problem... Why only these two get initialized?
f4d41d5 to
5c00df6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
…rns. Previously, Clad generated reverse-sweep lambdas that captured uninitialized memory, causing a segmentation fault.
5c00df6 to
ab99a4e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
This PR fixes a segmentation fault encountered while differentiating recursive functions with early returns (e.g., backtracking type functions). When differentiating such functions Clad wraps the reverse sweep in a lambda
(auto _rev0 = [&]). Previously,clad::restore_trackerdeclarations and ValueAndAdjoint temporaries generated for operator[] calls were emitted inside nested forward-sweep blocks and it leads to null pointer dereferencing, resulting in a segmentation fault.