On Wed, Feb 26, 2014 at 10:56 AM, Philip Martin
<philip.mar...@wandisco.com>wrote:
> Philip Martin <phi...@codematters.co.uk> writes:
>
> > Philip Martin <philip.mar...@wandisco.com> writes:
> >
> >> I'm seeing test failures when building Subversion and APR with pool
> >> debugging:
> >>
> >> FAIL: lt-fs-fs-pack-test 13: upgrade txns to log addressing in shared
> FSFS
> >> FAIL: lt-fs-fs-pack-test 14: upgrade txns started before svnadmin
> upgrade
> >> FAIL: lt-fs-test 33: test dir prop preservation in unordered txns
> >> FAIL: lt-fs-test 43: test merge directory properties
> >>
> >> Valgrind reports are typically:
> >>
> >> $ valgrind -q .libs/lt-fs-test --cleanup 33
> >> ==32655== Invalid read of size 1
> >> ==32655== at 0x402EA79: bcmp (mc_replace_strmem.c:935)
> >> ==32655== by 0x41F6E72: find_entry (apr_hash.c:280)
> >> ==32655== by 0x41F719D: apr_hash_set (apr_hash.c:357)
> >> ==32655== by 0x50430CB: read_dir_entries (cached_data.c:2053)
> >> ==32655== by 0x50432A4: get_dir_contents (cached_data.c:2098)
> >> ==32655== by 0x5043640: svn_fs_fs__rep_contents_dir
> (cached_data.c:2195)
> >> ==32655== by 0x50714B3: write_final_rev (transaction.c:2897)
> >> ==32655== by 0x5074113: commit_body (transaction.c:3839)
> >> ==32655== by 0x504A898: with_some_lock_file (fs_fs.c:185)
> >> ==32655== by 0x504A970: svn_fs_fs__with_write_lock (fs_fs.c:203)
> >> ==32655== by 0x5074BB1: svn_fs_fs__commit (transaction.c:4009)
> >> ==32655== by 0x507983F: svn_fs_fs__commit_txn (tree.c:2228)
> >> ==32655== Address 0x870d120 is 0 bytes inside a block of size 2 free'd
> >> ==32655== at 0x402AF4C: free (vg_replace_malloc.c:468)
> >> ==32655== by 0x42021F9: pool_clear_debug (apr_pools.c:1576)
> >> ==32655== by 0x42022D9: apr_pool_clear_debug (apr_pools.c:1613)
> >> ==32655== by 0x5042DCC: read_dir_entries (cached_data.c:1993)
> >> ==32655== by 0x50432A4: get_dir_contents (cached_data.c:2098)
> >> ==32655== by 0x5043640: svn_fs_fs__rep_contents_dir
> (cached_data.c:2195)
> >> ==32655== by 0x50714B3: write_final_rev (transaction.c:2897)
> >> ==32655== by 0x5074113: commit_body (transaction.c:3839)
> >> ==32655== by 0x504A898: with_some_lock_file (fs_fs.c:185)
> >> ==32655== by 0x504A970: svn_fs_fs__with_write_lock (fs_fs.c:203)
> >> ==32655== by 0x5074BB1: svn_fs_fs__commit (transaction.c:4009)
> >> ==32655== by 0x507983F: svn_fs_fs__commit_txn (tree.c:2228)
> >>
> >> I think this was caused by r1554711, switch directory rep to array
> >> order, but I haven't identified the exact problem yet.
> >
> > I think it might be:
> >
> > Index: sw/subversion/src/subversion/libsvn_fs_fs/cached_data.c
> > ===================================================================
> > --- sw/subversion/src/subversion/libsvn_fs_fs/cached_data.c (revision
> 1571876)
> > +++ sw/subversion/src/subversion/libsvn_fs_fs/cached_data.c (working
> copy)
> > @@ -2050,7 +2050,10 @@
> > /* In incremental mode, update the hash; otherwise, write to the
> > * final array. */
> > if (incremental)
> > - apr_hash_set(hash, entry.key, entry.keylen, dirent);
> > + apr_hash_set(hash,
> > + apr_pstrmemdup(scratch_pool, entry.key,
> entry.keylen),
> > + entry.keylen,
> > + dirent);
> > else
> > APR_ARRAY_PUSH(entries, svn_fs_dirent_t *) = dirent;
> > }
>
It turns out that we already have a suitable key copy
in DIRENT. Fixed in r1572049.
> >
> > that removes the above valgrind warning but now I see:
> >
> > $ valgrind -q .libs/lt-fs-test --cleanup 33
> > ==26848== Invalid read of size 8
> > ==26848== at 0x50513AE: svn_fs_fs__id_txn_used (id.c:151)
> > ==26848== by 0x505CB34: svn_fs_fs__unparse_representation
> (low_level.c:890)
> > ==26848== by 0x505CE8A: svn_fs_fs__write_noderev (low_level.c:939)
> > ==26848== by 0x506B76A: svn_fs_fs__put_node_revision
> (transaction.c:499)
> > ==26848== by 0x50497AC: svn_fs_fs__dag_update_ancestry (dag.c:1346)
> > ==26848== by 0x5079474: merge (tree.c:2070)
> > ==26848== by 0x50791B4: merge (tree.c:2021)
> > ==26848== by 0x50791B4: merge (tree.c:2021)
> > ==26848== by 0x507966A: merge_changes (tree.c:2121)
> > ==26848== by 0x5079800: svn_fs_fs__commit_txn (tree.c:2218)
> > ==26848== by 0x4048248: svn_fs_commit_txn (fs-loader.c:830)
> > ==26848== by 0x402CAB: test_commit_txn (fs-test.c:81)
> > ==26848== Address 0x7775668 is 72 bytes inside a block of size 112
> free'd
> > ==26848== at 0x402AF4C: free (vg_replace_malloc.c:468)
> > ==26848== by 0x42021F9: pool_clear_debug (apr_pools.c:1576)
> > ==26848== by 0x42022D9: apr_pool_clear_debug (apr_pools.c:1613)
> > ==26848== by 0x5078B3A: merge (tree.c:1916)
> > ==26848== by 0x50791B4: merge (tree.c:2021)
> > ==26848== by 0x50791B4: merge (tree.c:2021)
> > ==26848== by 0x507966A: merge_changes (tree.c:2121)
> > ==26848== by 0x5079800: svn_fs_fs__commit_txn (tree.c:2218)
> > ==26848== by 0x4048248: svn_fs_commit_txn (fs-loader.c:830)
> > ==26848== by 0x402CAB: test_commit_txn (fs-test.c:81)
> > ==26848== by 0x418544: unordered_txn_dirprops (fs-test.c:4655)
> > ==26848== by 0x403BD93: do_test_num (svn_test_main.c:400)
>
> I can fix this second warning with this:
>
> Index: subversion/libsvn_fs_fs/tree.c
> ===================================================================
> --- subversion/libsvn_fs_fs/tree.c (revision 1571978)
> +++ subversion/libsvn_fs_fs/tree.c (working copy)
> @@ -1957,7 +1957,7 @@ merge(svn_stringbuf_t *conflict_p,
> s_entry->id,
> s_entry->kind,
> txn_id,
> - iterpool));
> + pool));
> }
> else
> {
>
> Not sure whether either of these fixes is correct.
>
It looks correct to me and makes sense: We implicitly
modify (that's the bit I missed) the TARGET dag node.
The latter being allocated in POOL, we should use
POOL for the modification as well.
Committed in r1572049 as well.
Thanks for the analysis, Philip!
-- Stefan^2.
