You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2016/01/21 15:18:43 UTC
svn commit: r1725957 - /subversion/trunk/subversion/svn/merge-cmd.c
Author: kotkov
Date: Thu Jan 21 14:18:43 2016
New Revision: 1725957
URL: http://svn.apache.org/viewvc?rev=1725957&view=rev
Log:
Allocate the baton for 'svn merge --accept' conflict callback in the pool,
instead of using a pointer to stack.
* subversion/svn/merge-cmd.c
(svn_cl__merge): Allocate struct conflict_func_merge_cmd_baton in the
pool. Otherwise, the ctx->conflict_baton2 pointer becomes dangling
when the stack variable falls out of scope.
Modified:
subversion/trunk/subversion/svn/merge-cmd.c
Modified: subversion/trunk/subversion/svn/merge-cmd.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/merge-cmd.c?rev=1725957&r1=1725956&r2=1725957&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/merge-cmd.c (original)
+++ subversion/trunk/subversion/svn/merge-cmd.c Thu Jan 21 14:18:43 2016
@@ -513,14 +513,14 @@ svn_cl__merge(apr_getopt_t *os,
* See the docstring at conflict_func_merge_cmd() for details */
if (opt_state->accept_which != svn_cl__accept_unspecified)
{
- struct conflict_func_merge_cmd_baton b;
+ struct conflict_func_merge_cmd_baton *b = apr_pcalloc(pool, sizeof(*b));
- b.accept_which = opt_state->accept_which;
- SVN_ERR(svn_dirent_get_absolute(&b.path_prefix, "", pool));
- b.conflict_stats = conflict_stats;
+ b->accept_which = opt_state->accept_which;
+ SVN_ERR(svn_dirent_get_absolute(&b->path_prefix, "", pool));
+ b->conflict_stats = conflict_stats;
ctx->conflict_func2 = conflict_func_merge_cmd;
- ctx->conflict_baton2 = &b;
+ ctx->conflict_baton2 = b;
}
merge_err = run_merge(two_sources_specified,
Re: svn commit: r1725957 - /subversion/trunk/subversion/svn/merge-cmd.c
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
>> The scope does matter: the C standard specifies that the lifetime of an
>> object "extends from entry into the block with which it is associated
>> until execution of that block ends" [6.2.4]. An optimising compiler is
>> free to reuse stack slots once out of scope, either for other stack
>> variables or for stack space to call a function.
>
> Interesting. I stand corrected, thanks!
gcc implements this optimisation and has -fstack-reuse to control it:
https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/Code-Gen-Options.html
--
Philip Martin
WANdisco
Re: svn commit: r1725957 -
/subversion/trunk/subversion/svn/merge-cmd.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jan 21, 2016 at 05:46:20PM +0000, Philip Martin wrote:
> Stefan Sperling <st...@elego.de> writes:
>
> > Not objecting to this change, but I believe we do have a few
> > batons on the stack in several places and I don't think this
> > is a problem in general.
>
> It's perfectly acceptable to use a stack variable for a baton but it has
> to be in the same scope as function call. In this case moving the stack
> variable outside the if() block would have been an alternative solution.
>
> > The stack memory will be available until the function returns,
> > which in this case is svn_cl__merge() i.e. the main entry point
> > to the subcommand.
> >
> > The scope shouldn't matter since there's no garbage collection in C.
>
> The scope does matter: the C standard specifies that the lifetime of an
> object "extends from entry into the block with which it is associated
> until execution of that block ends" [6.2.4]. An optimising compiler is
> free to reuse stack slots once out of scope, either for other stack
> variables or for stack space to call a function.
Interesting. I stand corrected, thanks!
Re: svn commit: r1725957 - /subversion/trunk/subversion/svn/merge-cmd.c
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> Not objecting to this change, but I believe we do have a few
> batons on the stack in several places and I don't think this
> is a problem in general.
It's perfectly acceptable to use a stack variable for a baton but it has
to be in the same scope as function call. In this case moving the stack
variable outside the if() block would have been an alternative solution.
> The stack memory will be available until the function returns,
> which in this case is svn_cl__merge() i.e. the main entry point
> to the subcommand.
>
> The scope shouldn't matter since there's no garbage collection in C.
The scope does matter: the C standard specifies that the lifetime of an
object "extends from entry into the block with which it is associated
until execution of that block ends" [6.2.4]. An optimising compiler is
free to reuse stack slots once out of scope, either for other stack
variables or for stack space to call a function.
--
Philip Martin
WANdisco
Re: svn commit: r1725957 -
/subversion/trunk/subversion/svn/merge-cmd.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jan 21, 2016 at 02:18:43PM -0000, kotkov@apache.org wrote:
> Author: kotkov
> Date: Thu Jan 21 14:18:43 2016
> New Revision: 1725957
>
> URL: http://svn.apache.org/viewvc?rev=1725957&view=rev
> Log:
> Allocate the baton for 'svn merge --accept' conflict callback in the pool,
> instead of using a pointer to stack.
>
> * subversion/svn/merge-cmd.c
> (svn_cl__merge): Allocate struct conflict_func_merge_cmd_baton in the
> pool. Otherwise, the ctx->conflict_baton2 pointer becomes dangling
> when the stack variable falls out of scope.
>
> Modified:
> subversion/trunk/subversion/svn/merge-cmd.c
Not objecting to this change, but I believe we do have a few
batons on the stack in several places and I don't think this
is a problem in general.
The stack memory will be available until the function returns,
which in this case is svn_cl__merge() i.e. the main entry point
to the subcommand.
The scope shouldn't matter since there's no garbage collection in C.
>
> Modified: subversion/trunk/subversion/svn/merge-cmd.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/merge-cmd.c?rev=1725957&r1=1725956&r2=1725957&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svn/merge-cmd.c (original)
> +++ subversion/trunk/subversion/svn/merge-cmd.c Thu Jan 21 14:18:43 2016
> @@ -513,14 +513,14 @@ svn_cl__merge(apr_getopt_t *os,
> * See the docstring at conflict_func_merge_cmd() for details */
> if (opt_state->accept_which != svn_cl__accept_unspecified)
> {
> - struct conflict_func_merge_cmd_baton b;
> + struct conflict_func_merge_cmd_baton *b = apr_pcalloc(pool, sizeof(*b));
>
> - b.accept_which = opt_state->accept_which;
> - SVN_ERR(svn_dirent_get_absolute(&b.path_prefix, "", pool));
> - b.conflict_stats = conflict_stats;
> + b->accept_which = opt_state->accept_which;
> + SVN_ERR(svn_dirent_get_absolute(&b->path_prefix, "", pool));
> + b->conflict_stats = conflict_stats;
>
> ctx->conflict_func2 = conflict_func_merge_cmd;
> - ctx->conflict_baton2 = &b;
> + ctx->conflict_baton2 = b;
> }
>
> merge_err = run_merge(two_sources_specified,
>
>