On Tue, 26 May 2026 at 18:16, Branko Čibej <[email protected]> wrote:
> On 26. 5. 26 17:47, Ivan Zhakov wrote: > > On Thu, 21 May 2026 at 15:33, Jun Omae <[email protected]> wrote: > >> On 2026/05/21 21:32, Evgeny Kotkov wrote: >> > Jun Omae <[email protected]> writes: >> > >> >> It occurs only with MSVC toolset v143 (--vsnet-version=2022) and Whole >> >> Program Optimization enabled. It doesn't with v142 >> (--vsnet-version=2019). >> >> >> >> svn_fs_fs__delete_revprops_shard() is built by the optimization to >> >> delete the "revprops/0" directory even if `shard` argument is 0. >> >> >> >> Workaround is to disable the optimization. >> > >> > Interesting. Could be a compiler issue or an UB somewhere in the code. >> > >> > Would you mind providing more details on your environment to make it >> easier >> > to reproduce? >> >> It is able to reproduce using cmake with toolset v143. >> >> 1. Pass -vcvars_ver=14.44 to vcvars64.bat >> 2. Pass CMAKE_INTERPROCEDURAL_OPTIMIZATION=ON to cmake configure >> (The option makes Whole Program Optimization enabled in cmake build) >> >> > I can reproduce this issue in my environment and I believe it is indeed a > compiler bug. Here is the behavior across different MSVC versions: > > - MSVC 19.*40*.33821.0: No problem > - MSVC 19.*44*.35227.0: Problem occurs > - MSVC 19.*50*.35730.0: No problem > > In version 19.44.35227.0 with /GL (Whole Program Optimization) enabled, > the `if (shard == 0)` branch seems to be incorrrectly **optimized away**. > > I think this is caused by combination of the do-while loop and the `while > (kind == svn_node_dir && to_cleanup > 0)` condition on the calling side > (details below). > > The code below from subversion/libsvn_fs_fs/pack.c:synced_pack_shard() > becomes a simple call to svn_io_remove_dir2() that recursively deletes the > revprops/0 directory: > [[[ > > do > { > SVN_ERR(svn_fs_fs__delete_revprops_shard(revprops_shard_path, > to_cleanup, > ffd->max_files_per_dir, > pb->cancel_func, > pb->cancel_baton, > pool)); > > /* If the previous shard exists, clean it up as well. > Don't try to clean up shard 0 as it we can't tell quickly whether > it actually needs cleaning up. */ > revprops_shard_path = svn_dirent_join(pb->revsprops_dir, > apr_psprintf(pool, "%" APR_INT64_T_FMT, --to_cleanup), > pool); > SVN_ERR(svn_io_check_path(revprops_shard_path, &kind, pool)); > } > while (kind == svn_node_dir && to_cleanup > 0); > > ]]] > [[[ > > if (shard == 0) > { > apr_pool_t *iterpool = svn_pool_create(scratch_pool); > int i; > > /* delete all files except the one for revision 0 */ > for (i = 1; i < max_files_per_dir; ++i) > { > const char *path; > svn_pool_clear(iterpool); > > path = svn_dirent_join(shard_path, > apr_psprintf(iterpool, "%d", i), > iterpool); > if (cancel_func) > SVN_ERR(cancel_func(cancel_baton)); > > SVN_ERR(svn_io_remove_file2(path, TRUE, iterpool)); > } > > svn_pool_destroy(iterpool); > } > else > SVN_ERR(svn_io_remove_dir2(shard_path, TRUE, > cancel_func, cancel_baton, scratch_pool)); > > ]]] > > I checked that replacing the do-while loop with an equivalent while {} > loop makes the problem go away. My theory is that the compiler > incorrectly assumes to_cleanup is > 0 within the loop block and optimizes > away the inner check. > > > Could there be an even simpler "fix"? Move the --to_cleanup before the > call to svn_dirent_join? > > It was the first thing I have tried :) But it doesn't help. I think that we could move --to_cleanup out of argument evaluation anyway, making the code easier to parse, but it won't fix the problem. That shouldn't change the semantics at all, but it would indicate that the > MSVC data flow analysis somehow misses that the decrement in the function > parameter has externally visible side effects. The code is clearly correct. > > I agree that the code looks correct. I think the only thing we could do in this case is to report to problem to MS. -- Ivan Zhakov
