-
Notifications
You must be signed in to change notification settings - Fork 200
Enforce that a derivative references only decls it owns. #1894
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,6 +62,52 @@ | |||||||||
| return nullptr; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const ValueDecl* findOriginalRef(const Stmt* Derivative, | ||||||||||
| const FunctionDecl* Original) { | ||||||||||
| if (!Derivative || !Original) | ||||||||||
| return nullptr; | ||||||||||
| // A generated derivative owns fresh clones of every param/local it needs; a | ||||||||||
| // reference still bound to one of Original's own decls means its remap was | ||||||||||
| // forgotten (the primal clone was never registered in m_DeclReplacements). | ||||||||||
| // Walk the finished body and flag the first such reference. | ||||||||||
| struct Finder : RecursiveASTVisitor<Finder> { | ||||||||||
| const FunctionDecl* Original; | ||||||||||
| const ValueDecl* Stray = nullptr; | ||||||||||
| bool shouldVisitImplicitCode() const { return true; } | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: function 'shouldVisitImplicitCode' should be marked [[nodiscard]] [modernize-use-nodiscard]
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: method 'shouldVisitImplicitCode' can be made static [readability-convert-member-functions-to-static]
Suggested change
|
||||||||||
| bool VisitDeclRefExpr(DeclRefExpr* DRE) { | ||||||||||
| const ValueDecl* D = DRE->getDecl(); | ||||||||||
| // Flag only params/locals declared DIRECTLY in Original -- those are what | ||||||||||
| // BuildParams/VisitDeclStmt clone and must remap. A nested lambda's own | ||||||||||
| // parameter (context is the lambda's CXXMethod, not Original) is | ||||||||||
| // referenced by design when the lambda is preserved, not a forgotten | ||||||||||
| // clone, so exact-context match excludes it. | ||||||||||
| const auto* DC = dyn_cast<FunctionDecl>(D->getDeclContext()); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "clang::dyn_cast" is directly included [misc-include-cleaner] lib/Differentiator/ASTIntegrity.cpp:18: + #include <clang/Basic/LLVM.h> |
||||||||||
| if (isa<VarDecl>(D) && DC && | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "clang::isa" is directly included [misc-include-cleaner] if (isa<VarDecl>(D) && DC &&
^ |
||||||||||
| DC->getCanonicalDecl() == Original->getCanonicalDecl()) { | ||||||||||
| Stray = D; | ||||||||||
| return false; // stop at the first offender | ||||||||||
| } | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
| } F; | ||||||||||
| F.Original = Original; | ||||||||||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) | ||||||||||
| F.TraverseStmt(const_cast<Stmt*>(Derivative)); | ||||||||||
| return F.Stray; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| IntegrityReport verifyDerivative(const Stmt* Derivative, | ||||||||||
| const FunctionDecl* Original) { | ||||||||||
| IntegrityReport R; | ||||||||||
| R.SharedNode = findSharedNode(Derivative); | ||||||||||
| if (Original) { | ||||||||||
| if (const Stmt* PrimalBody = Original->getBody()) | ||||||||||
| R.PrimalNode = findPrimalSharedNode(Derivative, PrimalBody); | ||||||||||
| R.StrayRef = findOriginalRef(Derivative, Original); | ||||||||||
| } | ||||||||||
| return R; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const Stmt* findSharedNode(const Stmt* Root) { | ||||||||||
| if (!Root) | ||||||||||
| return nullptr; | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| #include "clad/Differentiator/Timers.h" | ||
| #include "clad/Differentiator/VectorForwardModeVisitor.h" | ||
| #include "clad/Differentiator/VectorPushForwardModeVisitor.h" | ||
| #include "clad/Differentiator/Version.h" | ||
| #include "clad/Differentiator/VisitorBase.h" | ||
|
|
||
| #include "clang/AST/ASTContext.h" | ||
|
|
@@ -586,6 +587,15 @@ | |
| << VD << L; | ||
| } | ||
|
|
||
| #if CLANG_VERSION_MAJOR > 16 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "CLANG_VERSION_MAJOR" is directly included [misc-include-cleaner] lib/Differentiator/DerivativeBuilder.cpp:51: - #include <cstddef>
+ #include <clang/Basic/Version.h>
+ #include <cstddef> |
||
| // Snapshot the diagnostic tally so the integrity check below can tell a | ||
| // clean differentiation from one that hit an unsupported construct (which | ||
| // is cloned wholesale and knowingly keeps un-remapped references). Guarded | ||
| // with the check itself: below clang-17 it is unused (-Werror=unused). | ||
| DiagnosticsEngine& Diags = m_Sema.getDiagnostics(); | ||
| unsigned DiagsBefore = Diags.getNumWarnings() + Diags.getNumErrors(); | ||
| #endif | ||
|
|
||
| DerivativeAndOverload result{}; | ||
| if (request.Mode == DiffMode::forward) { | ||
| BaseForwardModeVisitor V(*this, request); | ||
|
|
@@ -646,44 +656,60 @@ | |
| } | ||
|
|
||
| #if CLANG_VERSION_MAJOR > 16 | ||
| // A generated derivative must be a proper tree in its Stmt child-edge | ||
| // structure: no node is the child (Stmt::children()) of two parents, | ||
| // because a later in-place edit of a shared node leaks into its other | ||
| // users (and a shared aggregate initializer breaks CodeGen). Below | ||
| // A generated derivative must satisfy several structural invariants; below | ||
| // clang-17 buildClonedLambda cannot synthesize a fresh closure, so lambda | ||
| // derivatives legitimately share and this check is unreachable. | ||
| // derivatives legitimately share and these checks are unreachable. | ||
| if (auto* FD = dyn_cast_or_null<clang::FunctionDecl>(result.derivative)) | ||
| if (clang::Stmt* Body = FD->getBody()) { | ||
| const clang::Stmt* Shared = findSharedNode(Body); | ||
| // Debug asserts builds abort here; release builds keep the diagnostic | ||
| // so a sharing regression is not silently shipped. | ||
| assert(!Shared && "clad generated a derivative with a shared AST node"); | ||
| if (Shared) | ||
| // Compute cleanliness before the diagnostics below inflate the tally. | ||
| bool CleanDerivation = | ||
| Diags.getNumWarnings() + Diags.getNumErrors() == DiagsBefore; | ||
| IntegrityReport Report = verifyDerivative(Body, request.Function); | ||
|
|
||
| // A derivative must be a proper tree in its Stmt child-edge structure: | ||
| // no node the child of two parents, because a later in-place edit of a | ||
| // shared node leaks into its other users (and a shared aggregate | ||
| // initializer breaks CodeGen). Debug builds abort here; release builds | ||
| // keep the diagnostic so a regression is not silently shipped. | ||
| assert(!Report.SharedNode && | ||
| "clad generated a derivative with a shared AST node"); | ||
| if (Report.SharedNode) | ||
| diag(DiagnosticsEngine::Warning, FD->getLocation(), | ||
| "clad internally reused a '%0' AST node while differentiating " | ||
| "%1; this is a clad bug -- please report it at " | ||
| "https://github.com/vgvassilev/clad") | ||
| << Shared->getStmtClassName() << FD; | ||
|
|
||
| // A derivative must also not splice a node owned by its primal. The | ||
| // original function's AST outlives differentiation, so a shared node | ||
| // exposes the user's own code to any later in-place edit of the | ||
| // derivative -- the same corruption risk findSharedNode guards against, | ||
| // across the primal/derivative boundary it cannot see. Enforce it too. | ||
| if (const clang::FunctionDecl* PrimalFD = request.Function) | ||
| if (const clang::Stmt* PrimalBody = PrimalFD->getBody()) { | ||
| const clang::Stmt* FromPrimal = | ||
| findPrimalSharedNode(Body, PrimalBody); | ||
| assert(!FromPrimal && | ||
| "clad spliced a primal AST node into a derivative"); | ||
| if (FromPrimal) | ||
| diag(DiagnosticsEngine::Warning, FD->getLocation(), | ||
| "clad reused a '%0' AST node from the original function " | ||
| "while " | ||
| "differentiating %1; this is a clad bug -- please report it " | ||
| "at https://github.com/vgvassilev/clad") | ||
| << FromPrimal->getStmtClassName() << FD; | ||
| } | ||
| "%1; this is a clad bug -- please report it at %2") | ||
| << Report.SharedNode->getStmtClassName() << FD | ||
| << getCladRepositoryURL(); | ||
|
|
||
| // It must also not splice a node owned by its primal: the original | ||
| // function's AST outlives differentiation, so a later in-place edit of | ||
| // a shared node would corrupt the user's own code. | ||
| assert(!Report.PrimalNode && | ||
| "clad spliced a primal AST node into a derivative"); | ||
| if (Report.PrimalNode) | ||
| diag(DiagnosticsEngine::Warning, FD->getLocation(), | ||
| "clad reused a '%0' AST node from the original function while " | ||
| "differentiating %1; this is a clad bug -- please report it at " | ||
| "%2") | ||
| << Report.PrimalNode->getStmtClassName() << FD | ||
| << getCladRepositoryURL(); | ||
|
|
||
| // And it must reference only decls it owns. A DeclRefExpr still bound | ||
| // to one of the original function's own params/locals is a forgotten | ||
| // reference-remap. Only meaningful for a clean derivation: an | ||
| // unsupported construct is cloned wholesale and knowingly keeps such | ||
| // references in a derivative that is not used. | ||
| if (CleanDerivation) { | ||
| assert(!Report.StrayRef && | ||
| "derivative references an un-remapped decl of the original"); | ||
| if (Report.StrayRef) | ||
| diag( | ||
| DiagnosticsEngine::Warning, FD->getLocation(), | ||
| "clad left a reference to '%0' bound to the original function " | ||
| "while differentiating %1; this is a clad bug -- please report " | ||
| "it at %2") | ||
| << Report.StrayRef->getNameAsString() << FD | ||
| << getCladRepositoryURL(); | ||
| } | ||
| } | ||
| #endif | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: constructor does not initialize these fields: Original [cppcoreguidelines-pro-type-member-init]
lib/Differentiator/ASTIntegrity.cpp:73: