[Mono-dev] How To Convince Mono To Allocate Big Arrays
Rodrigo Kumpera
kumpera at gmail.com
Wed May 28 13:53:52 EDT 2008
Luis,
I have committed your patch with some changes or missing parts. Thanks for
the contribuitins. Some of the comments were due to a more detailed review
while merging your code in.
Index: mono/metadata/object.c
===================================================================
--- mono/metadata/object.c (revision 17)
+++ mono/metadata/object.c (working copy)
@@ -3513,12 +3513,23 @@
}
/* helper macros to check for overflow when calculating the size of arrays
*/
-#define MYGUINT32_MAX 4294967295U
+#if (GLIB_SIZEOF_SIZE_T < 4 )
I changed the check to be against MONO_BIG_ARRAYS.
@@ -3548,34 +3559,32 @@
/* A single dimensional array with a 0 lower bound is the same as an
szarray */
....
if (CHECK_ADD_OVERFLOW_UN (byte_len, 3))
- mono_gc_out_of_memory (MYGUINT32_MAX);
+ mono_gc_out_of_memory (MONO_ARRAY_MAX_SIZE);
byte_len = (byte_len + 3) & ~3;
You might need to adjust this alignment check to be aware of your changes.
===================================================================
--- mono/metadata/icall.c (revision 17)
+++ mono/metadata/icall.c (working copy)
@@ -527,11 +527,11 @@
aklass = mono_bounded_array_class_get (mono_class_from_mono_type
(type->type), mono_array_length (lengths), bounded);
- sizes = alloca (aklass->rank * sizeof(guint32) * 2);
+ sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * 2);
for (i = 0; i < aklass->rank; ++i) {
- sizes [i] = mono_array_get (lengths, guint32, i);
+ sizes [i] = mono_array_get (lengths, gint32, i);
if (bounds)
- sizes [i + aklass->rank] = mono_array_get (bounds, guint32, i);
+ sizes [i + aklass->rank] = mono_array_get (bounds, gint32, i);
else
sizes [i + aklass->rank] = 0;
}
Why are you using mono_array_get with gint32? I can't think of a reason for
this change, as it won't matter for mono_array_new_full. Besides that, this
is asking for trouble as you are mixing signed and unsigned numbers.
@@ -560,6 +606,27 @@
mono_raise_exception (mono_get_exception_index_out_of_range ());
if (this->bounds == NULL)
+ length = this->max_length;
+ else
+ length = this->bounds [dimension].length;
+
+ if (length > G_MAXINT32)
+ mono_raise_exception (mono_get_exception_overflow ());
+
+ return length;
+}
I have put the length check inside a #ifdef MONO_BIG_ARRAYS
@@ -541,6 +541,51 @@
return array;
}
+static MonoArray *
+ves_icall_System_Array_CreateInstanceImpl64 (MonoReflectionType *type,
MonoArray *lengths, MonoArray *bounds)
...
+ for (i = 0; i < mono_array_length (lengths); i++)
+ if ((mono_array_get (lengths, gint64, i) < 0) ||
+ (mono_array_get (lengths, gint64, i) > MONO_ARRAY_MAX_INDEX))
+ mono_raise_exception (mono_get_exception_argument_out_of_range
(NULL));
Can't this expression be simplified to "if ((mono_array_get (lengths,
guint64, i) > MONO_ARRAY_MAX_INDEX)",
note we are reading it as an unsigned value.
...
+ sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * 2);
+ for (i = 0; i < aklass->rank; ++i) {
+ sizes [i] = mono_array_get (lengths, gint64, i);
+ if (bounds)
+ sizes [i + aklass->rank] = mono_array_get (bounds, gint64, i);
+ else
+ sizes [i + aklass->rank] = 0;
+ }
Note that sizes element type is an unsigned number, so we must read it
acordingly and cast it to mono_array_size_t to
ajust for the desired size - and avoid warnings as well.
We can use the same code for the bound check I suggested for both
CreateInstanceImpl versions. If we do that we could fix
the ugly aspect of having two 99% identical functions by defining it as a
MACRO and expanding it twice.
On Fri, May 23, 2008 at 7:10 PM, Luis F. Ortiz <LuisOrtiz at verizon.net>
wrote:
> Rodrigo:
> Sorry for the delayed response, things got busy, and I did not notice
> your message.
> Please go ahead and check in the parts that you feel most comfortable with,
> since I do not
> have commit rights. I'll adapt any further submissions against the next
> official cut.
>
> The changes I have made are to be released under the X11 license and I
> vouch that
> I have talked to the appropriate people at my employer (Interactive
> Supercomputing) and
> they agree as well to release this and any further contributions I make to
> Mono while
> under their employ under the X11 license. There, that ought to make
> everybody happy.
>
> Luis F. Ortiz
>
>
> On May 16, 2008, at 10:53 AM, Rodrigo Kumpera wrote:\
>
> Hi Luis,
>
> Most parts of your patch are ok to be commited. As I said before, it's
> better to have separate patches to speed up the review. As you get more
> changes in, it will ease your work on preparing patches and will ease on
> tracking head changes.
>
> Right now I'm ok with the configure, mono_array_t and related changes.
> These were easy to review and don't introduce any change in behavior, so we
> can check-in then right now. The others requires more testing from me and
> would be easier to both of us if done after.
> Do you mind cooking you a patch only with the ok parts.
>
> Remember that you need to either release these changes under the X11
> license or have done some copyright assignment paperwork.
>
> Thanks,
> Rodrigo
>
>
>
> On Thu, May 8, 2008 at 8:21 PM, Luis F. Ortiz <LuisOrtiz at verizon.net>
> wrote:
>
>> On May 8, 2008, at 9:29 AM, Rodrigo Kumpera wrote:
>>
>>> One important thing I forgot. If you break your patch into a few smaller
>>> ones the review process will be a lot easier to every one involved.
>>> The first one can introduce new types and configuration foo; then other
>>> to fix codegen and Array methods and; at last, a bunch of fixes to classlib
>>> issues -like sockets, file i/o and so on.
>>>
>>> Cheers,
>>> Rodrigo
>>>
>>
>> OK, here is another stab at the changes.
>>
>> This modifies the following files:
>> configure.in
>> mono/metadata/object.c
>> mono/metadata/object.h
>> mono/metadata/icall-def.h
>> mono/metadata/icall.c
>> mono/metadata/socket-io.c
>>
>> These files:
>> 1) Add a new configuration option, --enable-big-arrays, which defines
>> conditionally defines MONO_BIG_ARRAYS in config.h,
>> 2) Add in mono/metadata/object.h :
>> A) A new typedef for mono_array_size_t to be either guint32 or guint64
>> B) A new #define for MONO_ARRAY_MAX_INDEX for the biggest valid array
>> index, i.e. G_MAXINTxx
>> C) A new #define for MONO_ARRAY_MAX_SIZE for the biggest valid array
>> allocation, i.e. G_MAXUINTxx
>> D) The above all controlled by MONO_BIG_ARRAYS
>> 3) Modify the definitions of mono_array_new(), mono_array_new_full(), and
>> mono_array_new_specific() to match,
>> 4) Modify the usages of those functions in the metadata directory to
>> match,
>> 5) Add range checking in ves_icall_System_Array_CreateInstanceImpl64 so
>> it works with or without MONO_BIG_ARRAYS,
>> 6) Attempt to address all the mistakes you pointed out.
>>
>> These changes, in addition to the other changes I made, pass "make check"
>> with the exception of Tests.test_0_regress_75832(), as previously confessed.
>>
>>
>>
>>
>>
>> /Ortiz/Luis
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080528/98155c12/attachment.html
More information about the Mono-devel-list
mailing list