Skip to content

Commit 82308ce

Browse files
committed
Do not splice the original function's AST nodes into derivatives.
A generated derivative must share no Stmt node with the primal it is built from. The primal's AST outlives differentiation, so a node clad reuses from the original exposes the user's own code to any later in-place edit of the derivative. Several forward-mode paths handed the primal node straight through as the "cloned" primal instead of cloning it: the member `this`, non-differentiable decls, __null and __func__, captured lambdas, the function-name string inside a PredefinedExpr, and top-level lambda closures. Clone each of them so the derivative owns a distinct subtree, and add findPrimalSharedNode to keep it that way -- the cross-boundary sibling of findSharedNode. It intersects the Stmt::children() reachability sets of the derivative and its primal (child edges only, so Clang's legitimate type, template-argument and default-argument shares are excluded) and is enforced beside findSharedNode: assert in debug, diagnostic in release.
1 parent b75bba2 commit 82308ce

7 files changed

Lines changed: 134 additions & 34 deletions

File tree

lib/Differentiator/ASTIntegrity.cpp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,55 @@
1313
#include "clang/AST/StmtOpenMP.h"
1414

1515
#include "llvm/ADT/DenseMap.h"
16+
#include "llvm/ADT/DenseSet.h"
1617
#include "llvm/ADT/SmallPtrSet.h"
18+
#include "llvm/ADT/SmallVector.h"
1719

1820
using namespace clang;
1921

2022
namespace clad {
2123

24+
// Collect the Stmt::children() reachability set of Root. Child edges only, NOT
25+
// RecursiveASTVisitor: RAV also follows type and template-argument edges, and
26+
// Clang legitimately shares nodes across those (a template-argument constant,
27+
// an array bound). children() also never descends into a CXXDefaultArgExpr's
28+
// stored default or a type, so those Clang-owned shares are excluded for free.
29+
static void collectChildEdges(const Stmt* Root,
30+
llvm::DenseSet<const Stmt*>& Set) {
31+
if (!Root)
32+
return;
33+
llvm::SmallVector<const Stmt*, 64> Work;
34+
Set.insert(Root);
35+
Work.push_back(Root);
36+
while (!Work.empty()) {
37+
const Stmt* Cur = Work.pop_back_val();
38+
for (const Stmt* Ch : Cur->children())
39+
if (Ch && Set.insert(Ch).second)
40+
Work.push_back(Ch);
41+
}
42+
}
43+
44+
const Stmt* findPrimalSharedNode(const Stmt* Derivative, const Stmt* Primal) {
45+
if (!Derivative || !Primal)
46+
return nullptr;
47+
llvm::DenseSet<const Stmt*> PrimalNodes;
48+
collectChildEdges(Primal, PrimalNodes);
49+
llvm::SmallVector<const Stmt*, 64> Work;
50+
llvm::DenseSet<const Stmt*> Seen;
51+
Seen.insert(Derivative);
52+
Work.push_back(Derivative);
53+
while (!Work.empty()) {
54+
const Stmt* Cur = Work.pop_back_val();
55+
for (const Stmt* Ch : Cur->children())
56+
if (Ch && Seen.insert(Ch).second) {
57+
if (PrimalNodes.count(Ch))
58+
return Ch;
59+
Work.push_back(Ch);
60+
}
61+
}
62+
return nullptr;
63+
}
64+
2265
const Stmt* findSharedNode(const Stmt* Root) {
2366
if (!Root)
2467
return nullptr;

lib/Differentiator/ASTIntegrity.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,16 @@ namespace clad {
2727
/// proper tree.
2828
const clang::Stmt* findSharedNode(const clang::Stmt* Root);
2929

30+
/// Return the first node in \p Derivative that is also reachable from \p Primal
31+
/// through Stmt::children() edges, or nullptr if the derivative splices no node
32+
/// from its primal. The original function's AST outlives differentiation, so a
33+
/// generated derivative sharing one of its nodes would corrupt the user's own
34+
/// code if a later pass edits it. Child edges only: Clang legitimately shares
35+
/// type/template-argument constants and default-argument expressions, which
36+
/// Stmt::children() does not traverse.
37+
const clang::Stmt* findPrimalSharedNode(const clang::Stmt* Derivative,
38+
const clang::Stmt* Primal);
39+
3040
} // namespace clad
3141

3242
#endif // CLAD_AST_INTEGRITY_H

lib/Differentiator/BaseForwardModeVisitor.cpp

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1665,18 +1665,15 @@ StmtDiff BaseForwardModeVisitor::VisitDeclStmt(const DeclStmt* DS) {
16651665
// supported.
16661666
if (typeDecl && (clad::utils::hasNonDifferentiableAttribute(typeDecl) ||
16671667
typeDecl->isLambda())) {
1668-
// When reconstructing the primal inside a pushforward, a lambda copied
1669-
// here would share its operator() body with the primal lambda emitted in
1670-
// the enclosing derivative. Rebuild it with a fresh closure. The
1671-
// top-level primal (forward mode) must keep the original closure so its
1672-
// generated pushforward method still resolves, hence the mode guard.
1673-
const bool inPushforward = m_DiffReq.Mode == DiffMode::pushforward ||
1674-
m_DiffReq.Mode == DiffMode::vector_pushforward;
1668+
// A lambda copied verbatim would splice the primal's closure -- its
1669+
// operator() body -- into the derivative. Rebuild it with a fresh closure
1670+
// instead; the call's pushforward is regenerated for that closure in
1671+
// VisitCallExpr, so it still resolves.
16751672
for (auto* D : DS->decls()) {
16761673
assert(isa<VarDecl>(D) && "Mixed decl types in a single decl stmt is "
16771674
"not standard c++ syntax");
16781675
auto* VDecl = cast<VarDecl>(D);
1679-
if (inPushforward && typeDecl->isLambda() && VDecl->getInit())
1676+
if (typeDecl->isLambda() && VDecl->getInit())
16801677
if (const auto* InnerLE =
16811678
dyn_cast<LambdaExpr>(VDecl->getInit()->IgnoreImplicit())) {
16821679
Expr* ClonedLambda = buildClonedLambda(InnerLE);
@@ -1689,7 +1686,20 @@ StmtDiff BaseForwardModeVisitor::VisitDeclStmt(const DeclStmt* DS) {
16891686
decls.push_back(NewVD);
16901687
continue;
16911688
}
1692-
decls.push_back(VDecl);
1689+
// A lambda without a direct initializer (or any other
1690+
// non-differentiable decl) is cloned so the derivative does not splice
1691+
// the primal's VarDecl and init into itself.
1692+
if (typeDecl->isLambda()) {
1693+
decls.push_back(VDecl);
1694+
} else {
1695+
Expr* clonedInit =
1696+
VDecl->getInit() ? Clone(VDecl->getInit()) : nullptr;
1697+
VarDecl* copyVD =
1698+
BuildVarDecl(VDecl->getType(), VDecl->getNameAsString(),
1699+
clonedInit, VDecl->isDirectInit());
1700+
m_DeclReplacements[VDecl] = copyVD;
1701+
decls.push_back(copyVD);
1702+
}
16931703
}
16941704
Stmt* DSClone = BuildDeclStmt(decls);
16951705
return StmtDiff(DSClone, nullptr);
@@ -1794,16 +1804,16 @@ BaseForwardModeVisitor::VisitCStyleCastExpr(const CStyleCastExpr* CSCE) {
17941804
StmtDiff BaseForwardModeVisitor::VisitGNUNullExpr(const clang::GNUNullExpr* E) {
17951805
auto* Constant0 =
17961806
ConstantFolder::synthesizeLiteral(m_Context.IntTy, m_Context, /*val=*/0);
1797-
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
1798-
return StmtDiff(const_cast<clang::GNUNullExpr*>(E), Constant0);
1807+
// Clone so the derivative owns its copy rather than the primal's node.
1808+
return StmtDiff(CloneNode(E), Constant0);
17991809
}
18001810

18011811
StmtDiff
18021812
BaseForwardModeVisitor::VisitPredefinedExpr(const clang::PredefinedExpr* E) {
18031813
auto* Constant0 =
18041814
ConstantFolder::synthesizeLiteral(m_Context.IntTy, m_Context, /*val=*/0);
1805-
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
1806-
return StmtDiff(const_cast<clang::PredefinedExpr*>(E), Constant0);
1815+
// Clone so the derivative owns its copy rather than the primal's node.
1816+
return StmtDiff(CloneNode(E), Constant0);
18071817
}
18081818

18091819
StmtDiff
@@ -2301,10 +2311,10 @@ StmtDiff BaseForwardModeVisitor::VisitCXXTemporaryObjectExpr(
23012311

23022312
StmtDiff
23032313
BaseForwardModeVisitor::VisitCXXThisExpr(const clang::CXXThisExpr* CTE) {
2304-
// m_ThisExprDerivative is a single cached `_d_this` ref; hand out a fresh
2305-
// clone so distinct uses do not share the same node.
2306-
return StmtDiff(const_cast<CXXThisExpr*>(CTE),
2307-
CloneNode(m_ThisExprDerivative));
2314+
// Clone CTE so the derivative owns its `this` node rather than splicing the
2315+
// primal method's; m_ThisExprDerivative is a single cached `_d_this` ref, so
2316+
// clone it too so distinct uses do not share the same node.
2317+
return StmtDiff(CloneNode(CTE), CloneNode(m_ThisExprDerivative));
23082318
}
23092319

23102320
StmtDiff BaseForwardModeVisitor::VisitCXXNewExpr(const clang::CXXNewExpr* CNE) {

lib/Differentiator/DerivativeBuilder.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,26 @@ static void registerDerivative(Decl* D, Sema& S, const DiffRequest& R) {
664664
"%1; this is a clad bug -- please report it at "
665665
"https://github.com/vgvassilev/clad")
666666
<< Shared->getStmtClassName() << FD;
667+
668+
// A derivative must also not splice a node owned by its primal. The
669+
// original function's AST outlives differentiation, so a shared node
670+
// exposes the user's own code to any later in-place edit of the
671+
// derivative -- the same corruption risk findSharedNode guards against,
672+
// across the primal/derivative boundary it cannot see. Enforce it too.
673+
if (const clang::FunctionDecl* PrimalFD = request.Function)
674+
if (const clang::Stmt* PrimalBody = PrimalFD->getBody()) {
675+
const clang::Stmt* FromPrimal =
676+
findPrimalSharedNode(Body, PrimalBody);
677+
assert(!FromPrimal &&
678+
"clad spliced a primal AST node into a derivative");
679+
if (FromPrimal)
680+
diag(DiagnosticsEngine::Warning, FD->getLocation(),
681+
"clad reused a '%0' AST node from the original function "
682+
"while "
683+
"differentiating %1; this is a clad bug -- please report it "
684+
"at https://github.com/vgvassilev/clad")
685+
<< FromPrimal->getStmtClassName() << FD;
686+
}
667687
}
668688
#endif
669689

lib/Differentiator/ReverseModeVisitor.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3597,16 +3597,16 @@ Expr* ReverseModeVisitor::getStdInitListSizeExpr(const Expr* E) {
35973597
StmtDiff ReverseModeVisitor::VisitGNUNullExpr(const clang::GNUNullExpr* E) {
35983598
auto* Constant0 = ConstantFolder::synthesizeLiteral(m_Context.IntTy,
35993599
m_Context, /*val=*/0);
3600-
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
3601-
return StmtDiff(const_cast<clang::GNUNullExpr*>(E), Constant0);
3600+
// Clone so the derivative owns its copy rather than the primal's node.
3601+
return StmtDiff(CloneNode(E), Constant0);
36023602
}
36033603

36043604
StmtDiff
36053605
ReverseModeVisitor::VisitPredefinedExpr(const clang::PredefinedExpr* E) {
36063606
auto* Constant0 = ConstantFolder::synthesizeLiteral(m_Context.IntTy,
36073607
m_Context, /*val=*/0);
3608-
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
3609-
return StmtDiff(const_cast<clang::PredefinedExpr*>(E), Constant0);
3608+
// Clone so the derivative owns its copy rather than the primal's node.
3609+
return StmtDiff(CloneNode(E), Constant0);
36103610
}
36113611

36123612
StmtDiff ReverseModeVisitor::VisitCXXFunctionalCastExpr(

lib/Differentiator/StmtClone.cpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,17 @@ Stmt* StmtClone::VisitDeclRefExpr(DeclRefExpr *Node) {
8181
DEFINE_CREATE_EXPR(IntegerLiteral,
8282
(Ctx, Node->getValue(), CloneType(Node->getType()),
8383
Node->getLocation()))
84-
DEFINE_CLONE_EXPR_CO(PredefinedExpr,
85-
(Ctx, Node->getLocation(), CloneType(Node->getType()),
86-
Node->getIdentKind()
87-
CLAD_COMPAT_CLANG17_IsTransparent(Node),
88-
Node->getFunctionName()))
84+
Stmt* StmtClone::VisitPredefinedExpr(PredefinedExpr* Node) {
85+
// Clone the inner function-name StringLiteral; passing
86+
// Node->getFunctionName() would share it with the source.
87+
StringLiteral* FN = Node->getFunctionName();
88+
PredefinedExpr* result = PredefinedExpr::Create(
89+
Ctx, Node->getLocation(), CloneType(Node->getType()),
90+
Node->getIdentKind() CLAD_COMPAT_CLANG17_IsTransparent(Node),
91+
FN ? cast<StringLiteral>(Clone(FN)) : nullptr);
92+
clad_compat::ExprSetDeps(result, Node);
93+
return result;
94+
}
8995
DEFINE_CLONE_EXPR(CharacterLiteral,
9096
(Node->getValue(), Node->getKind(),
9197
CloneType(Node->getType()), Node->getLocation()))

lib/Differentiator/VisitorBase.cpp

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -830,26 +830,37 @@ namespace clad {
830830
}
831831

832832
Expr* VisitorBase::buildClonedLambda(const LambdaExpr* LE) {
833-
// A primal copy needs a *fresh* closure type; a plain StmtClone reuses
834-
// the original closure, so two clones end up sharing the operator() body
835-
// -- violating the one-parent-per-node invariant. Captured lambdas would
836-
// need capture rebinding we do not model here; fall back to a plain clone
837-
// (they are not currently a source of node sharing).
833+
// A primal copy needs a *fresh* closure type; a plain StmtClone reuses the
834+
// original closure, so two clones share the operator() body -- both a
835+
// one-parent-per-node violation and a node shared with the primal lambda.
836+
//
837+
// We reproduce only explicit by-copy/by-ref captures of a named variable
838+
// (each re-resolves by name against the derivative's in-scope copy). Fall
839+
// back to a plain clone for the kinds we do not model -- this-capture,
840+
// init-capture, and packs.
838841
#if CLANG_VERSION_MAJOR < 17
839842
// Lambda differentiation is unsupported below clang-17 (Lambdas.C is
840843
// UNSUPPORTED there) and the Sema lambda-introduction entry points used
841844
// below do not exist yet. Never reached; keep the source compilable.
842845
return cast<Expr>(Clone(LE));
843846
#else
844-
if (LE->capture_size() != 0)
845-
return cast<Expr>(Clone(LE));
847+
for (const clang::LambdaCapture& Cap : LE->explicit_captures())
848+
if (!Cap.capturesVariable() ||
849+
(Cap.getCaptureKind() != clang::LCK_ByCopy &&
850+
Cap.getCaptureKind() != clang::LCK_ByRef))
851+
return cast<Expr>(Clone(LE));
846852

847853
const CXXMethodDecl* CallOp = LE->getCallOperator();
848854

849855
// Mirror the lambda-introduction dance performed while parsing a lambda;
850856
// see buildDerivedLambda for the differentiating counterpart.
851857
LambdaIntroducer Intro;
852-
Intro.Default = LCD_None;
858+
Intro.Default = LE->getCaptureDefault();
859+
for (const clang::LambdaCapture& Cap : LE->explicit_captures())
860+
Intro.addCapture(Cap.getCaptureKind(), Cap.getLocation(),
861+
Cap.getCapturedVar()->getIdentifier(), /*EllipsisLoc=*/
862+
noLoc, clang::LambdaCaptureInitKind::NoInit,
863+
clang::ExprResult(), clang::ParsedType(), SourceRange());
853864
Intro.Range.setBegin(LE->getBeginLoc());
854865
Intro.Range.setEnd(LE->getEndLoc());
855866
AttributeFactory AttrFactory;

0 commit comments

Comments
 (0)