You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2013/05/24 15:10:54 UTC
Re: svn commit: r1485848 -
/subversion/trunk/subversion/libsvn_subr/subst.c
On Thu, May 23, 2013 at 08:55:32PM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Thu May 23 20:55:32 2013
> New Revision: 1485848
>
> URL: http://svn.apache.org/r1485848
> Log:
> Double the speed translate_chunk in case that keyword substitution has
> not been enabled.
I believe this revision is causing crashes in my client built
from trunk. The trunk client is also corrupting working copies I use.
I'll investigate a bit further, but if my testing shows that this
change is reponsible I am going to revert it ASAP to prevent the
damage from spreading.
'svn diff' coredumps with:
(gdb)
#0 0x000011bd8dc946bd in svn_eol__find_eol_start (buf=0x11bd84838ffa "",
len=12309) at subversion/libsvn_subr/eol.c:56
56 apr_uintptr_t chunk = *(const apr_uintptr_t *)buf;
(gdb)
Bottom (i.e., innermost) frame selected; you cannot go down.
(gdb) p buf
$1 = 0x11bd84838ffa ""
(gdb) p chunk
$2 = 0
(gdb) p * buf
$3 = 0 '\0'
Some working files contain binary garbage after the crash.
The working copy needs to be checked out again with a working client.
>
> * subversion/libsvn_subr/subst.c
> (translate_chunk): use a fast scanner if we only care about newlines
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/subst.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/subst.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/subst.c?rev=1485848&r1=1485847&r2=1485848&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/subst.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu May 23 20:55:32 2013
> @@ -50,6 +50,7 @@
> #include "svn_private_config.h"
>
> #include "private/svn_string_private.h"
> +#include "private/svn_eol_private.h"
>
> /**
> * The textual elements of a detranslated special file. One of these
> @@ -1116,28 +1117,39 @@ translate_chunk(svn_stream_t *dst,
> /* skip current EOL */
> len += b->eol_str_len;
>
> - /* Check 4 bytes at once to allow for efficient pipelining
> - and to reduce loop condition overhead. */
> - while ((p + len + 4) <= end)
> + if (b->keywords)
> {
> - if (interesting[(unsigned char)p[len]]
> - || interesting[(unsigned char)p[len+1]]
> - || interesting[(unsigned char)p[len+2]]
> - || interesting[(unsigned char)p[len+3]])
> - break;
> -
> - len += 4;
> + /* Check 4 bytes at once to allow for efficient pipelining
> + and to reduce loop condition overhead. */
> + while ((p + len + 4) <= end)
> + {
> + if (interesting[(unsigned char)p[len]]
> + || interesting[(unsigned char)p[len+1]]
> + || interesting[(unsigned char)p[len+2]]
> + || interesting[(unsigned char)p[len+3]])
> + break;
> +
> + len += 4;
> + }
> +
> + /* Found an interesting char or EOF in the next 4 bytes.
> + Find its exact position. */
> + while ((p + len) < end
> + && !interesting[(unsigned char)p[len]])
> + ++len;
> + }
> + else
> + {
> + /* use our optimized sub-routine to find the next EOL */
> + const char *eol
> + = svn_eol__find_eol_start((char *)p + len, end - p);
> + len += (eol ? eol : end) - (p + len);
> }
> -
> - /* Found an interesting char or EOF in the next 4 bytes.
> - Find its exact position. */
> - while ((p + len) < end && !interesting[(unsigned char)p[len]])
> - ++len;
> }
> while (b->nl_translation_skippable ==
> svn_tristate_true && /* can potentially skip EOLs */
> p + len + 2 < end && /* not too close to EOF */
> - eol_unchanged (b, p + len)); /* EOL format already ok */
> + eol_unchanged(b, p + len)); /* EOL format already ok */
>
> while ((p + len) < end && !interesting[(unsigned char)p[len]])
> len++;
>
Re: svn commit: r1485848 - /subversion/trunk/subversion/libsvn_subr/subst.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, May 24, 2013 at 3:25 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 24.05.2013 15:10, Stefan Sperling wrote:
> > On Thu, May 23, 2013 at 08:55:32PM -0000, stefan2@apache.org wrote:
> >> Author: stefan2
> >> Date: Thu May 23 20:55:32 2013
> >> New Revision: 1485848
> >>
> >> URL: http://svn.apache.org/r1485848
> >> Log:
> >> Double the speed translate_chunk in case that keyword substitution has
> >> not been enabled.
> > I believe this revision is causing crashes in my client built
> > from trunk. The trunk client is also corrupting working copies I use.
> >
> > I'll investigate a bit further, but if my testing shows that this
> > change is reponsible I am going to revert it ASAP to prevent the
> > damage from spreading.
> >
> > 'svn diff' coredumps with:
> >
> > (gdb)
> > #0 0x000011bd8dc946bd in svn_eol__find_eol_start (buf=0x11bd84838ffa "",
> > len=12309) at subversion/libsvn_subr/eol.c:56
> > 56 apr_uintptr_t chunk = *(const apr_uintptr_t *)buf;
> > (gdb)
> > Bottom (i.e., innermost) frame selected; you cannot go down.
> > (gdb) p buf
> > $1 = 0x11bd84838ffa ""
> > (gdb) p chunk
> > $2 = 0
> > (gdb) p * buf
> > $3 = 0 '\0'
> >
> > Some working files contain binary garbage after the crash.
> > The working copy needs to be checked out again with a working client.
> >
> >> * subversion/libsvn_subr/subst.c
> >> (translate_chunk): use a fast scanner if we only care about newlines
> >>
> >> Modified:
> >> subversion/trunk/subversion/libsvn_subr/subst.c
> >>
> >> Modified: subversion/trunk/subversion/libsvn_subr/subst.c
> >> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/subst.c?rev=1485848&r1=1485847&r2=1485848&view=diff
> >>
> ==============================================================================
> >> --- subversion/trunk/subversion/libsvn_subr/subst.c (original)
> >> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu May 23 20:55:32
> 2013
> >> @@ -50,6 +50,7 @@
> >> #include "svn_private_config.h"
> >>
> >> #include "private/svn_string_private.h"
> >> +#include "private/svn_eol_private.h"
> >>
> >> /**
> >> * The textual elements of a detranslated special file. One of these
> >> @@ -1116,28 +1117,39 @@ translate_chunk(svn_stream_t *dst,
> >> /* skip current EOL */
> >> len += b->eol_str_len;
> >>
> >> - /* Check 4 bytes at once to allow for efficient
> pipelining
> >> - and to reduce loop condition overhead. */
> >> - while ((p + len + 4) <= end)
>
> If the above condition is correct ...
> [...]
>
> >> + else
> >> + {
> >> + /* use our optimized sub-routine to find the next
> EOL */
> >> + const char *eol
> >> + = svn_eol__find_eol_start((char *)p + len, end -
> p);
>
> ... then the second parameter here should be (end - p + 1), not (end - p).
>
Nope, end - (p + len). And that was the problem.
>
> >> + len += (eol ? eol : end) - (p + len);
>
> And I don't understand this statement at all.
>
If there is no EOL, svn_eol__find_eol_start will return NULL.
-- Stefan^2.
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
Re: svn commit: r1485848 - /subversion/trunk/subversion/libsvn_subr/subst.c
Posted by Branko Čibej <br...@wandisco.com>.
On 24.05.2013 15:10, Stefan Sperling wrote:
> On Thu, May 23, 2013 at 08:55:32PM -0000, stefan2@apache.org wrote:
>> Author: stefan2
>> Date: Thu May 23 20:55:32 2013
>> New Revision: 1485848
>>
>> URL: http://svn.apache.org/r1485848
>> Log:
>> Double the speed translate_chunk in case that keyword substitution has
>> not been enabled.
> I believe this revision is causing crashes in my client built
> from trunk. The trunk client is also corrupting working copies I use.
>
> I'll investigate a bit further, but if my testing shows that this
> change is reponsible I am going to revert it ASAP to prevent the
> damage from spreading.
>
> 'svn diff' coredumps with:
>
> (gdb)
> #0 0x000011bd8dc946bd in svn_eol__find_eol_start (buf=0x11bd84838ffa "",
> len=12309) at subversion/libsvn_subr/eol.c:56
> 56 apr_uintptr_t chunk = *(const apr_uintptr_t *)buf;
> (gdb)
> Bottom (i.e., innermost) frame selected; you cannot go down.
> (gdb) p buf
> $1 = 0x11bd84838ffa ""
> (gdb) p chunk
> $2 = 0
> (gdb) p * buf
> $3 = 0 '\0'
>
> Some working files contain binary garbage after the crash.
> The working copy needs to be checked out again with a working client.
>
>> * subversion/libsvn_subr/subst.c
>> (translate_chunk): use a fast scanner if we only care about newlines
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_subr/subst.c
>>
>> Modified: subversion/trunk/subversion/libsvn_subr/subst.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/subst.c?rev=1485848&r1=1485847&r2=1485848&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_subr/subst.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu May 23 20:55:32 2013
>> @@ -50,6 +50,7 @@
>> #include "svn_private_config.h"
>>
>> #include "private/svn_string_private.h"
>> +#include "private/svn_eol_private.h"
>>
>> /**
>> * The textual elements of a detranslated special file. One of these
>> @@ -1116,28 +1117,39 @@ translate_chunk(svn_stream_t *dst,
>> /* skip current EOL */
>> len += b->eol_str_len;
>>
>> - /* Check 4 bytes at once to allow for efficient pipelining
>> - and to reduce loop condition overhead. */
>> - while ((p + len + 4) <= end)
If the above condition is correct ...
[...]
>> + else
>> + {
>> + /* use our optimized sub-routine to find the next EOL */
>> + const char *eol
>> + = svn_eol__find_eol_start((char *)p + len, end - p);
... then the second parameter here should be (end - p + 1), not (end - p).
>> + len += (eol ? eol : end) - (p + len);
And I don't understand this statement at all.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1485848 - /subversion/trunk/subversion/libsvn_subr/subst.c
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> I'll investigate a bit further, but if my testing shows that this
> change is reponsible I am going to revert it ASAP to prevent the
> damage from spreading.
>
> 'svn diff' coredumps with:
>
> (gdb)
> #0 0x000011bd8dc946bd in svn_eol__find_eol_start (buf=0x11bd84838ffa "",
> len=12309) at subversion/libsvn_subr/eol.c:56
> 56 apr_uintptr_t chunk = *(const apr_uintptr_t *)buf;
> (gdb)
> Bottom (i.e., innermost) frame selected; you cannot go down.
> (gdb) p buf
> $1 = 0x11bd84838ffa ""
> (gdb) p chunk
> $2 = 0
> (gdb) p * buf
> $3 = 0 '\0'
I see the same:
==5285== Invalid read of size 8
==5285== at 0x5A3EF29: svn_eol__find_eol_start (eol.c:56)
==5285== by 0x5A6D41B: translate_chunk (subst.c:1144)
==5285== by 0x5A6DB9D: translated_stream_write (subst.c:1323)
==5285== by 0x5A657C9: svn_stream_write (stream.c:161)
==5285== by 0x5A662E2: svn_stream_copy3 (stream.c:501)
==5285== by 0x5A6EC6C: svn_subst_copy_and_translate4 (subst.c:1793)
==5285== by 0x512028C: svn_wc__internal_translated_file (translate.c:226)
==5285== by 0x50F0F0B: svn_wc__diff_base_working_diff (diff_editor.c:526)
==5285== by 0x50F8064: diff_status_callback (diff_local.c:362)
==5285== by 0x511B731: send_status_structure (status.c:930)
==5285== by 0x511BC7A: one_child_status (status.c:1206)
==5285== by 0x511C4F4: get_dir_status (status.c:1441)
==5285== Address 0x40feffb is not stack'd, malloc'd or (recently) free'd
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1485848 -
/subversion/trunk/subversion/libsvn_subr/subst.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, May 27, 2013 at 10:30:31PM +0400, Konstantin Kolinko wrote:
> There is a typo in the commit message:
Fixed, thanks.
Re: svn commit: r1485848 - /subversion/trunk/subversion/libsvn_subr/subst.c
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/5/24 Stefan Fuhrmann <st...@wandisco.com>:
>
> On Fri, May 24, 2013 at 3:10 PM, Stefan Sperling <st...@elego.de> wrote:
>>
>> On Thu, May 23, 2013 at 08:55:32PM -0000, stefan2@apache.org wrote:
>> > Author: stefan2
>> > Date: Thu May 23 20:55:32 2013
>> > New Revision: 1485848
>> >
>> > URL: http://svn.apache.org/r1485848
>> > Log:
>> > Double the speed translate_chunk in case that keyword substitution has
>> > not been enabled.
>>
>> I believe this revision is causing crashes in my client built
>> from trunk. The trunk client is also corrupting working copies I use.
>>
>> I'll investigate a bit further, but if my testing shows that this
>> change is reponsible I am going to revert it ASAP to prevent the
>> damage from spreading.
>
>
> If that patch causes problems, feel free to revert it.
> It's not been an important improvement.
>
> I had just stumbled across this function when I was
> investigating why 'svn merge' is so slow.
>
> -- Stefan^2.
>
There is a typo in the commit message:
http://svn.apache.org/viewvc?view=revision&revision=r1485848
says:
[[[
*** This revision was reverted in r1486046 ***
*** and re-applied with bugfix in r1486058. ***
]]] ...
It was reverted not in ..046, but in ..044:
http://svn.apache.org/viewvc?view=revision&revision=1486044
Best regards,
Konstantin Kolinko
Re: svn commit: r1485848 - /subversion/trunk/subversion/libsvn_subr/subst.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, May 24, 2013 at 3:10 PM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, May 23, 2013 at 08:55:32PM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Thu May 23 20:55:32 2013
> > New Revision: 1485848
> >
> > URL: http://svn.apache.org/r1485848
> > Log:
> > Double the speed translate_chunk in case that keyword substitution has
> > not been enabled.
>
> I believe this revision is causing crashes in my client built
> from trunk. The trunk client is also corrupting working copies I use.
>
> I'll investigate a bit further, but if my testing shows that this
> change is reponsible I am going to revert it ASAP to prevent the
> damage from spreading.
>
If that patch causes problems, feel free to revert it.
It's not been an important improvement.
I had just stumbled across this function when I was
investigating why 'svn merge' is so slow.
-- Stefan^2.
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*