[Mono-list] RE: [Mono-devel-list] REGRESSION: StringBuilder
Iain McCoy
iain at mccoy.id.au
Tue Jan 13 10:16:48 EST 2004
I get the same problem, on a freshly checked-out-from-anoncvs-and-built
mcs and mono. I did some digging (just because I could) and I suspect
the problem is that StringBuilder uses String.InternalStrcpy to move
everything to the right of the insert point across. From my reading of
the code, that would go to the method
ves_icall_System_String_InternalStrcpy_StrN in
mono/metadata/string-icalls.c. That function's basically a call to
memcpy, which can't be used on overlapping memory areas (and breaking
that rule is what causes the regression). I think there's a few options
to fix it:
1 Add a String.InternalStrmov that uses memmove instead of memcpy
2 Change String.InternalStrcpy so that it uses memmove
Both of these options fix the problem. I suspect Option 2 is detrimental
to performance because it would mean that overlapping buffers were being
dealt with even when they weren't actually there.
I'm attaching a little patch I wrote that does Option 1. I make no
claims about this patch, except that it works for me. I didn't look over
StringBuilder particularly thoroughly, but I think I caught all of the
cases that needed to be changed.
I imagine I have broken 5 rules and 10 guidelines with this patch, but
oh well - I hope it is useful.
Iain
On Tue, 2004-01-13 at 23:55, Jaroslaw Kowalski wrote:
> I've just rebuilt mcs and mono and I still get the same error.
>
> BTW. I'm using:
>
> :pserver:anonymous at us-anoncvs.go-mono.com:/mono
>
> Jarek
> ----- Original Message -----
> From: Torstensson, Patrik
> To: Torstensson, Patrik ; Jaroslaw Kowalski ;
> mono-list at lists.ximian.com ; Mono Development
> Sent: Tuesday, January 13, 2004 1:12 PM
> Subject: [Mono-list] RE: [Mono-devel-list] REGRESSION:
> StringBuilder
>
> Have you rebuilt everything? I just did the same ression test
> here and that returns correct value.
>
> Can anyone else test this also with the latest CVS?
>
> Cheers,
> Patrik
>
> ______________________________________________________________
> From: mono-devel-list-admin at lists.ximian.com
> [mailto:mono-devel-list-admin at lists.ximian.com] On Behalf Of
> Torstensson, Patrik
> Sent: den 13 januari 2004 13:02
> To: Jaroslaw Kowalski; mono-list at lists.ximian.com; Mono
> Development
> Subject: RE: [Mono-devel-list] REGRESSION: StringBuilder
>
>
> Looking into it right now, it's strange because SB passed all
> the test.
>
> Be back in 2..
>
> Sorry for the problems caused!
>
> Cheers,
> Patrik
>
> ______________________________________________________________
> From: mono-devel-list-admin at lists.ximian.com
> [mailto:mono-devel-list-admin at lists.ximian.com] On Behalf Of
> Jaroslaw Kowalski
> Sent: den 13 januari 2004 12:33
> To: mono-list at lists.ximian.com; Mono Development
> Subject: [Mono-devel-list] REGRESSION: StringBuilder
> Importance: Low
>
>
> Looks like there's been a regression introduced in mono
> yesterday.
>
> This snippet:
>
> StringBuilder sb = new StringBuilder();
>
> sb.Append("testtesttest");
> sb.Insert(0, '^');
> Console.WriteLine("sb: {0}", sb);
>
> produces:
>
> sb: ^teetteetteet
>
> It obviously should be:
>
> sb: ^testtesttest
>
> This is kind of critical since NAnt no longer runs on
> mono/Linux.
>
> Jarek
--
Iain McCoy <iain at mccoy.id.au>
-------------- next part --------------
? mono/autom4te.cache
? mono/mint.pc
? mono/mono/interpreter/interp.lo
? mono/mono/interpreter/libmint.la
? mono/scripts/soapsuds
? mcs/class/Mono.Posix/Mono.Posix/Mono.Posix.dll
? mcs/class/Mono.Posix/Mono.Posix/map.c
? mcs/class/Mono.Posix/Mono.Posix/map.h
Index: mono/mono/metadata/icall.c
===================================================================
RCS file: /mono/mono/mono/metadata/icall.c,v
retrieving revision 1.395
diff -u -r1.395 icall.c
--- mono/mono/metadata/icall.c 12 Jan 2004 07:42:17 -0000 1.395
+++ mono/mono/metadata/icall.c 13 Jan 2004 16:19:43 -0000
@@ -4452,6 +4452,8 @@
"System.String::InternalAllocateStr", ves_icall_System_String_InternalAllocateStr,
"System.String::InternalStrcpy(string,int,string)", ves_icall_System_String_InternalStrcpy_Str,
"System.String::InternalStrcpy(string,int,string,int,int)", ves_icall_System_String_InternalStrcpy_StrN,
+ "System.String::InternalStrmove(string,int,string)", ves_icall_System_String_InternalStrmove_Str,
+ "System.String::InternalStrmove(string,int,string,int,int)", ves_icall_System_String_InternalStrmove_StrN,
"System.String::InternalIntern", ves_icall_System_String_InternalIntern,
"System.String::InternalIsInterned", ves_icall_System_String_InternalIsInterned,
"System.String::GetHashCode", ves_icall_System_String_GetHashCode,
Index: mono/mono/metadata/string-icalls.c
===================================================================
RCS file: /mono/mono/mono/metadata/string-icalls.c,v
retrieving revision 1.26
diff -u -r1.26 string-icalls.c
--- mono/mono/metadata/string-icalls.c 10 Oct 2003 01:37:27 -0000 1.26
+++ mono/mono/metadata/string-icalls.c 13 Jan 2004 16:19:44 -0000
@@ -790,6 +790,36 @@
memcpy(destptr + destPos, srcptr + startPos, count * sizeof(gunichar2));
}
+
+void
+ves_icall_System_String_InternalStrmove_Str (MonoString *dest, gint32 destPos, MonoString *src)
+{
+ gunichar2 *srcptr;
+ gunichar2 *destptr;
+
+ MONO_ARCH_SAVE_REGS;
+
+ srcptr = mono_string_chars (src);
+ destptr = mono_string_chars (dest);
+
+ memmove(destptr + destPos, srcptr, mono_string_length(src) * sizeof(gunichar2));
+}
+
+void
+ves_icall_System_String_InternalStrmove_StrN (MonoString *dest, gint32 destPos, MonoString *src, gint32 startPos, gint32 count)
+{
+ gunichar2 *srcptr;
+ gunichar2 *destptr;
+
+ MONO_ARCH_SAVE_REGS;
+
+ srcptr = mono_string_chars (src);
+ destptr = mono_string_chars (dest);
+ memmove(destptr + destPos, srcptr + startPos, count * sizeof(gunichar2));
+}
+
+
+
MonoString *
ves_icall_System_String_InternalIntern (MonoString *str)
{
Index: mono/mono/metadata/string-icalls.h
===================================================================
RCS file: /mono/mono/mono/metadata/string-icalls.h,v
retrieving revision 1.8
diff -u -r1.8 string-icalls.h
--- mono/mono/metadata/string-icalls.h 12 Jan 2003 01:35:35 -0000 1.8
+++ mono/mono/metadata/string-icalls.h 13 Jan 2004 16:19:45 -0000
@@ -100,6 +100,12 @@
void
ves_icall_System_String_InternalStrcpy_StrN (MonoString *dest, gint32 destPos, MonoString *src, gint32 startPos, gint32 count);
+void
+ves_icall_System_String_InternalStrmove_Str (MonoString *dest, gint32 destPos, MonoString *src);
+
+void
+ves_icall_System_String_InternalStrmove_StrN (MonoString *dest, gint32 destPos, MonoString *src, gint32 startPos, gint32 count);
+
MonoString *
ves_icall_System_String_InternalIntern (MonoString *str);
Index: mcs/class/corlib/System/String.cs
===================================================================
RCS file: /mono/mcs/class/corlib/System/String.cs,v
retrieving revision 1.101
diff -u -r1.101 String.cs
--- mcs/class/corlib/System/String.cs 12 Jan 2004 22:06:53 -0000 1.101
+++ mcs/class/corlib/System/String.cs 13 Jan 2004 16:20:17 -0000
@@ -1311,6 +1311,12 @@
internal extern static void InternalStrcpy(String dest, int destPos, String src, int startPos, int count);
[MethodImplAttribute(MethodImplOptions.InternalCall)]
+ internal extern static void InternalStrmove(String dest, int destPos, String src);
+
+ [MethodImplAttribute(MethodImplOptions.InternalCall)]
+ internal extern static void InternalStrmove(String dest, int destPos, String src, int startPos, int count);
+
+ [MethodImplAttribute(MethodImplOptions.InternalCall)]
private extern static string InternalIntern(string str);
[MethodImplAttribute(MethodImplOptions.InternalCall)]
Index: mcs/class/corlib/System.Text/StringBuilder.cs
===================================================================
RCS file: /mono/mcs/class/corlib/System.Text/StringBuilder.cs,v
retrieving revision 1.27
diff -u -r1.27 StringBuilder.cs
--- mcs/class/corlib/System.Text/StringBuilder.cs 12 Jan 2004 22:07:38 -0000 1.27
+++ mcs/class/corlib/System.Text/StringBuilder.cs 13 Jan 2004 16:20:22 -0000
@@ -209,7 +209,7 @@
// of the removed part and truncate the sLength
if (_length - (startIndex + length) > 0)
- String.InternalStrcpy (_str, startIndex + length, _str, startIndex, _length - (startIndex + length));
+ String.InternalStrmove (_str, startIndex + length, _str, startIndex, _length - (startIndex + length));
_length -= length;
@@ -455,7 +455,7 @@
InternalEnsureCapacity (_length + value.Length);
// Move everything to the right of the insert point across
- String.InternalStrcpy (_str, index + value.Length, _str, index, _length - index);
+ String.InternalStrmove (_str, index + value.Length, _str, index, _length - index);
// Copy in stuff from the insert buffer
String.InternalStrcpy (_str, index, value.ToString());
@@ -476,7 +476,7 @@
InternalEnsureCapacity (_length + value.Length);
// Move everything to the right of the insert point across
- String.InternalStrcpy (_str, index + value.Length, _str, index, _length - index);
+ String.InternalStrmove (_str, index + value.Length, _str, index, _length - index);
// Copy in stuff from the insert buffer
String.InternalStrcpy (_str, index, value);
@@ -502,7 +502,7 @@
InternalEnsureCapacity (_length + 1);
// Move everything to the right of the insert point across
- String.InternalStrcpy (_str, index + 1, _str, index, _length - index);
+ String.InternalStrmove (_str, index + 1, _str, index, _length - index);
_str.InternalSetChar (index, value);
_length++;
More information about the Mono-devel-list
mailing list