Skip to content

Commit 17ff9da

Browse files
Eric Prud'hommeauxclaude
Eric Prud'hommeaux
and
claude
committed
~ work through docs/code-review.md: all suites green, CI gates on everything
P1 fixes: - BGPSimplifier double-ownership (the test_QueryMap heisenbug): tableDisjunction now captures the outer conjunction once, processes each disjunct in a fresh context, and joins the pending conjunction as a whole; flushConjunction no longer re-adds a BGP that defaultGraphPattern/namedGraphPattern already added. Diagnosed with a new SWOBJ_DEBUG_DOUBLE_DELETE build (TableOperation live-set + tree-overlap check in ChainingMapper::map; kept, compiled out by default). 0/30 failures in release and instrumented stress runs (was 8/25). - ShEx semantic actions fire only from the accepted matching: step-6 candidate filtering runs with the handler suppressed, the accepted matching re-validates its chosen pairs with the handler on, and every rejection path rolls back to a per-matching mark. - SPARQL hash builtins (MD5/SHA1/SHA256/SHA384/SHA512) implemented in self-contained lib/SWCrypto.hpp, verified against system md5/shasum; replaces the dead CRYPT_LIB path (whose RAND() fallback returned a constant - now <random>). test_SPARQL11 fully green. - CSV <foo>-as-IRI branch re-enabled in ResultSet.cpp (conformant CSV results serialize IRIs bare, so only literal <...> cells change); function_library passes, csv-tsv results tests unaffected. P2 leaks/ownership: - ShExCParser.ypp %destructor declarations (interned TTerms and the emptyShape sentinel excluded); fixed two action-ordering bugs they exposed (addFacet duplicate-facet path, start-already-defined path). - ~SPARQLDriver frees stranded curFilter/curOp; ~ShExDriver frees stranded pendingRestricts. - getRDFLiteral frees a stray langtag on typed-literal returns. - ParserDriver namespace map: unique_ptr + observer, freeNamespaces bool gone. test_ShEx caches and GraphIndex use unique_ptr / a real (s,p)->o map. P6 performance: - ShEx neighborhood extraction uses the BGP SP/PO indexes (getTripleIterator); fixed a latent empty-range end-iterator dereference in triple_iterator's range constructor. - Matchings is now a pruning DFS bounded per-TC by SorbeExpr::maxCounts (product of enclosing cardinality maxima). Items 10/13/14/15 resolved with rationale in docs/code-review.md (bison %union + emptyShape sentinel make unique_ptr containers wrong for the ShEx AST; the closed spec AST doesn't warrant visitor conversion; grammar actions are already minimal; cardinality arithmetic already unified). Verification: all 21 ctest suites pass locally (first time); LSan (ubuntu:24.04): test_DAWG, test_ShEx incl. negativeSyntax, and the ShExMap CLI are leak-free; test_QueryMap retains ~2.8KB of pre-existing success-path leaks, catalogued in the doc. CI now gates on the full suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent c99b208 commit 17ff9da

13 files changed

Lines changed: 861 additions & 330 deletions

‎.github/workflows/ci.yml‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -38,20 +38,9 @@ jobs:
3838
- name: Build
3939
run: cmake --build build -j4
4040

41+
# All suites gate the build: the md5/sha* builtins, the
42+
# function_library CSV IRI branch, and the query-rewriter
43+
# double-delete that made these three non-blocking are fixed.
4144
- name: Run test suites
4245
run: >
4346
ctest --test-dir build --output-on-failure --timeout 600
44-
-E 'test_(SPARQL|SPARQL11|QueryMap)$'
45-
46-
# These suites carry known pre-existing failures:
47-
# - test_SPARQL11: md5/sha* builtins are unimplemented (8 cases)
48-
# - test_SPARQL: function_library expects CSV <foo> parsed as an IRI,
49-
# a csv-regex branch deliberately disabled upstream (~2013)
50-
# - test_QueryMap: heap-layout-dependent nondeterminism in the query
51-
# rewriter makes single cases flaky
52-
# Run them for visibility without gating the build.
53-
- name: Run suites with known pre-existing failures (non-blocking)
54-
continue-on-error: true
55-
run: >
56-
ctest --test-dir build --output-on-failure --timeout 600
57-
-R 'test_(SPARQL|SPARQL11|QueryMap)$'

‎docs/code-review.md‎

Lines changed: 170 additions & 142 deletions
Large diffs are not rendered by default.

‎lib/ChainingMapper.hpp‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,48 @@ namespace w3c_sw {
839839
rules.push_back(r);
840840
}
841841

842+
#ifdef SWOBJ_DEBUG_DOUBLE_DELETE
843+
/** Collect every TableOperation pointer in a tree, flagging nodes
844+
* reachable twice. */
845+
struct OpCollector : public RecursiveExpressor {
846+
std::set<const TableOperation*> seen;
847+
std::vector<const TableOperation*> dups;
848+
void note (const TableOperation* op) {
849+
if (!seen.insert(op).second)
850+
dups.push_back(op);
851+
}
852+
virtual void base (const Base* const, std::string) { }
853+
virtual void namedGraphPattern (const NamedGraphPattern* const self, const TTerm* a, bool b, const ProductionVector<const TriplePattern*>* c) { note(self); RecursiveExpressor::namedGraphPattern(self, a, b, c); }
854+
virtual void defaultGraphPattern (const DefaultGraphPattern* const self, bool a, const ProductionVector<const TriplePattern*>* b) { note(self); RecursiveExpressor::defaultGraphPattern(self, a, b); }
855+
virtual void tableConjunction (const TableConjunction* const self, const ProductionVector<const TableOperation*>* a) { note(self); RecursiveExpressor::tableConjunction(self, a); }
856+
virtual void tableDisjunction (const TableDisjunction* const self, const ProductionVector<const TableOperation*>* a) { note(self); RecursiveExpressor::tableDisjunction(self, a); }
857+
virtual void filter (const Filter* const self, const TableOperation* a, const ProductionVector<const Expression*>* b) { note(self); RecursiveExpressor::filter(self, a, b); }
858+
virtual void bind (const Bind* const self, const TableOperation* a, const Expression* b, const Variable* c) { note(self); RecursiveExpressor::bind(self, a, b, c); }
859+
virtual void optionalGraphPattern (const OptionalGraphPattern* const self, const TableOperation* a, const ProductionVector<const Expression*>* b) { note(self); RecursiveExpressor::optionalGraphPattern(self, a, b); }
860+
virtual void minusGraphPattern (const MinusGraphPattern* const self, const TableOperation* a) { note(self); RecursiveExpressor::minusGraphPattern(self, a); }
861+
virtual void graphGraphPattern (const GraphGraphPattern* const self, const TTerm* a, const TableOperation* b) { note(self); RecursiveExpressor::graphGraphPattern(self, a, b); }
862+
virtual void serviceGraphPattern (const ServiceGraphPattern* const self, const TTerm* a, const TableOperation* b, e_Silence c, AtomFactory* d, bool e) { note(self); RecursiveExpressor::serviceGraphPattern(self, a, b, c, d, e); }
863+
};
864+
865+
/** Print tree structure with node pointers. */
866+
struct OpPrinter : public OpCollector {
867+
int depth = 0;
868+
void note (const TableOperation* op) {
869+
fprintf(stderr, "%*s%p %s\n", depth * 2, "", (void*)op, typeid(*op).name());
870+
}
871+
virtual void namedGraphPattern (const NamedGraphPattern* const self, const TTerm* a, bool b, const ProductionVector<const TriplePattern*>* c) { note(self); ++depth; RecursiveExpressor::namedGraphPattern(self, a, b, c); --depth; }
872+
virtual void defaultGraphPattern (const DefaultGraphPattern* const self, bool a, const ProductionVector<const TriplePattern*>* b) { note(self); ++depth; RecursiveExpressor::defaultGraphPattern(self, a, b); --depth; }
873+
virtual void tableConjunction (const TableConjunction* const self, const ProductionVector<const TableOperation*>* a) { note(self); ++depth; RecursiveExpressor::tableConjunction(self, a); --depth; }
874+
virtual void tableDisjunction (const TableDisjunction* const self, const ProductionVector<const TableOperation*>* a) { note(self); ++depth; RecursiveExpressor::tableDisjunction(self, a); --depth; }
875+
virtual void filter (const Filter* const self, const TableOperation* a, const ProductionVector<const Expression*>* b) { note(self); ++depth; RecursiveExpressor::filter(self, a, b); --depth; }
876+
virtual void bind (const Bind* const self, const TableOperation* a, const Expression* b, const Variable* c) { note(self); ++depth; RecursiveExpressor::bind(self, a, b, c); --depth; }
877+
virtual void optionalGraphPattern (const OptionalGraphPattern* const self, const TableOperation* a, const ProductionVector<const Expression*>* b) { note(self); ++depth; RecursiveExpressor::optionalGraphPattern(self, a, b); --depth; }
878+
virtual void minusGraphPattern (const MinusGraphPattern* const self, const TableOperation* a) { note(self); ++depth; RecursiveExpressor::minusGraphPattern(self, a); --depth; }
879+
virtual void graphGraphPattern (const GraphGraphPattern* const self, const TTerm* a, const TableOperation* b) { note(self); ++depth; RecursiveExpressor::graphGraphPattern(self, a, b); --depth; }
880+
virtual void serviceGraphPattern (const ServiceGraphPattern* const self, const TTerm* a, const TableOperation* b, e_Silence c, AtomFactory* d, bool e) { note(self); ++depth; RecursiveExpressor::serviceGraphPattern(self, a, b, c, d, e); --depth; }
881+
};
882+
#endif /* SWOBJ_DEBUG_DOUBLE_DELETE */
883+
842884
/** Map a SPARQL operation over the consequents of #rules to an operation over the antecedents of #rules. */
843885
const Operation* map (const Operation* query, const ResultSet* mappingConstants = NULL) {
844886
QueryWalker walker(rules, atomFactory, sharedVars, nodeShare);
@@ -848,6 +890,29 @@ namespace w3c_sw {
848890
return op;
849891
BGPSimplifier dup(atomFactory); // removing the dup breaks test_QueryMap/healthCare/cabig/bg_hl7
850892
op->express(&dup);
893+
#ifdef SWOBJ_DEBUG_DOUBLE_DELETE
894+
{
895+
OpCollector inTree, outTree;
896+
op->express(&inTree);
897+
dup.last.operation->express(&outTree);
898+
for (std::vector<const TableOperation*>::const_iterator d = inTree.dups.begin();
899+
d != inTree.dups.end(); ++d)
900+
fprintf(stderr, "walker tree aliases %p (%s)\n", (void*)*d, typeid(**d).name());
901+
for (std::vector<const TableOperation*>::const_iterator d = outTree.dups.begin();
902+
d != outTree.dups.end(); ++d)
903+
fprintf(stderr, "simplified tree aliases %p (%s)\n", (void*)*d, typeid(**d).name());
904+
for (std::set<const TableOperation*>::const_iterator s = outTree.seen.begin();
905+
s != outTree.seen.end(); ++s)
906+
if (inTree.seen.find(*s) != inTree.seen.end())
907+
fprintf(stderr, "simplified tree shares %p (%s) with walker tree\n", (void*)*s, typeid(**s).name());
908+
if (!inTree.dups.empty() || !outTree.dups.empty()) {
909+
fprintf(stderr, "---- simplified tree ----\n");
910+
OpPrinter p;
911+
dup.last.operation->express(&p);
912+
_exit(77);
913+
}
914+
}
915+
#endif /* SWOBJ_DEBUG_DOUBLE_DELETE */
851916
delete op;
852917

853918
SWObjectCanonicalizer c(atomFactory);

‎lib/ParserCommon.hpp‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "SWObjects.hpp"
99
#include "location.hh"
1010
#include "utf8.h"
11+
#include <memory>
1112

1213
void stop(size_t line);
1314

@@ -95,22 +96,19 @@ struct ParserExceptions : std::exception {
9596
class ParserDriver {
9697
protected:
9798
std::string baseURI;
98-
NamespaceMap* namespaces;
99-
bool freeNamespaces;
99+
std::unique_ptr<NamespaceMap> ownNamespaces; // engaged unless a map was borrowed
100+
NamespaceMap* namespaces; // always the one in use
100101
TTerm::String2BNode bnodeMap;
101-
102+
102103
ParserDriver ()
103-
: baseURI(""), namespaces(new NamespaceMap()),
104-
freeNamespaces(true) { }
104+
: baseURI(""), ownNamespaces(new NamespaceMap()),
105+
namespaces(ownNamespaces.get()) { }
105106

106107
ParserDriver (std::string baseURI)
107-
: baseURI(baseURI), namespaces(new NamespaceMap()),
108-
freeNamespaces(true) { }
108+
: baseURI(baseURI), ownNamespaces(new NamespaceMap()),
109+
namespaces(ownNamespaces.get()) { }
109110

110-
virtual ~ParserDriver () {
111-
if (freeNamespaces)
112-
delete namespaces;
113-
}
111+
virtual ~ParserDriver () { }
114112

115113
public:
116114

@@ -130,10 +128,8 @@ class ParserDriver {
130128
}
131129

132130
void setNamespaceMap (NamespaceMap* newMap, bool pFreeNamespaces = false) {
133-
if (freeNamespaces)
134-
delete namespaces;
131+
ownNamespaces.reset(pFreeNamespaces ? newMap : NULL);
135132
namespaces = newMap;
136-
freeNamespaces = pFreeNamespaces;
137133
}
138134

139135
NamespaceMap* getNamespaceMap () { return namespaces; }

‎lib/ResultSet.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ namespace w3c_sw {
220220
"|(?:(,)|(^))"
221221
/* \TermText */ "("
222222
/* \Variable ?abc */ "\\?([a-zA-Z0-9_-]+)"
223-
/* \IRI <abc> */ "|([^\r\n]^)" // disabled
223+
/* \IRI <abc> */ "|<([^>]*)>"
224224
/* \BNode _:abc */ "|_:([a-zA-Z0-9_-]+)"
225225
/* \Quoted "abc" */ "|\\\"((?:[^\\\\\"]|\\\\[\"nrtb])*)\\\"(?:\\^\\^<([^>]*)>)?"
226226
/* \Double 1.2e3 */ "|([+-]?[0-9]*\\.[0-9]*[Ee][+\\-]?[0-9]+)"

‎lib/SPARQLParser.ypp‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,18 @@ public:
128128
curExprList(NULL), validate(DefaultValidation), root(NULL), unnestTree(false)
129129
{ }
130130

131-
~SPARQLDriver ()
132-
{ }
131+
~SPARQLDriver () {
132+
// free mid-parse state stranded when a parse error unwinds
133+
if (curFilter != NULL) {
134+
for (std::vector<const Expression*>::const_iterator it = curFilter->m_Expressions.begin();
135+
it != curFilter->m_Expressions.end(); ++it)
136+
delete *it;
137+
delete curFilter;
138+
}
139+
// curBGP is conjoined into curOp as soon as it is created
140+
// (ensureBasicGraphPattern), so deleting curOp covers it
141+
delete curOp;
142+
}
133143

134144
protected:
135145

0 commit comments

Comments
 (0)