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 On Wed, May 7, 2008 at 11:50 PM, Rodrigo Kumpera <[EMAIL PROTECTED]> wrote: > Hi Luis, > > To have your patch integrated, some changes are needed. First, we want to > default to 32bits sized arrays on 64bits machines, so your changes must be > conditionally compiled. To help with that some changed to your patch are > due. Next are some comments about it: > > Instead of replacing guint32 for gsize, it's better to create a new type, > let's say array_size_t. This would reduce conditional compilation to fewer > places. > > /* helper macros to check for overflow when calculating the size of arrays > */ > -#define MYGUINT32_MAX 4294967295U > +#if (GLIB_SIZEOF_SIZE_T < 4 ) > +#define MYGUINT32_MAX 0xFFFFFFFFUL > +#define MYGUINT_MAX MYGUINT32_MAX > > This #if seens bogus, don't you mean "if ((GLIB_SIZEOF_SIZE_T == 4 )" as > mono never supported 16bits machines. > The macros can be unified by using MYGUINT_MAX and the 'array_size_t' type > I talked before. The definition of MYGUINT_MAX > should be put together in the same place we define 'array_size_t'. And we > could go with a meaningful name, don't you think? > > > - if (CHECK_MUL_OVERFLOW_UN (n, elem_size)) > - mono_gc_out_of_memory (MYGUINT32_MAX); > + if (CHECK_MUL_OVERFLOW_UN (n, elem_size)) { > + g_print("CHECK_MUL_OVERFLOW_UN(%zd,%zd) failed\n",n, elem_size); > + mono_gc_out_of_memory (MYGUINT_MAX); > + } > > If you find that keeping such debug code is really important, you should > follow the same pattern of the rest of the project. Take a look at how > DEBUG_IMT is used on object.c. > > > @@ -3548,34 +3559,30 @@ > /* A single dimensional array with a 0 lower bound is the same as an > szarray */ > if (array_class->rank == 1 && ((array_class->byval_arg.type == > MONO_TYPE_SZARRAY) || (lower_bounds && lower_bounds [0] == 0))) { > len = lengths [0]; > - if ((int) len < 0) > - arith_overflow (); > > > Why are you removing overflow checks here? > > > @@ -562,6 +607,26 @@ > if (this->bounds == NULL) > return this->max_length; > > + length = this->bounds [dimension].length; > + if (length > G_MAXINT32) > + mono_raise_exception (mono_get_exception_overflow ()); > + > + return length; > +} > > Why throwing an exception here? I'm not sure this is the way to go, > unfortunately this is an area underspecified on ecma. Not that truncating is > a good option either. > > Changes to amd64 code I'll leave to Zoltan. > > > Cheers, > Rodrigo > > > 2008/5/5 Luis F. Ortiz <[EMAIL PROTECTED]>: > >> Back in September ("Big Arrays, Many Changes --- Request for Advice") I >> asked folks for advice on how to go about adding the capability to Mono to >> allocate arrays with more than Int32.MaxValue elements. After playing >> around with it for a few months, I'm at the point where I have an >> implementation that mostly works, with a couple of warts which could >> probably be quickly fixed by someone who knows more than I do about Mono >> internals. I spoke with Miguel about these patches, and he encouraged me to >> post them to mono-dev as soon as I got them to pass the "make check" test >> suite. So here I am a week later. >> I want to start by going over the changes themselves, what alternatives >> there might be to what I had done and what flaws I know to exist in the >> implementation. >> >> First off, though Microsoft chose for some reason NOT to implement large >> array allocation, the necessary APIs are in the .NET specification. For >> example, in the System.Array class, we find: >> >> public long >> GetLongLength<http://msdn.microsoft.com/en-us/library/system.array.getlonglength.aspx>(int >> dimension); >> public long >> LongLength<http://msdn.microsoft.com/en-us/library/system.array.longlength.aspx> >> { >> get; } >> public static Array >> CreateInstance<http://msdn.microsoft.com/en-us/library/1z8w3at5.aspx>(Type >> elementType, params long[] lengths); >> public Object >> GetValue<http://msdn.microsoft.com/en-us/library/2zexc3z9.aspx>(long >> index); >> public void >> SetValue<http://msdn.microsoft.com/en-us/library/czx562xz.aspx>(Object >> value, long index); >> ... (other overloads omitted, but there) >> and we find that the >> Newarr<http://msdn.microsoft.com/en-us/library/system.reflection.emit.opcodes.newarr%28VS.71%29.aspx> >> opcode >> takes a natural int or an Int32 as the length, so the bytecode level is >> ready too. >> >> Mono as of 1.2.6 already implemented most (all?) of the necessary >> interfaces in mcs/class/corlib/System/Array.cs, but they all cast down their >> long arguments down to integers as the underlying implementation was not >> there. >> >> So the first set of changes were to: >> * **mono/metadata/object.c* >> * **mono/metadata/object.h* >> * **mono/metadata/icall-def.h* >> * **mono/metadata/icall.c* >> * **mono/metadata/socket-io.c* >> >> In object.h I made three changes: >> >> 1) Changed MonoArrayBounds to use gsize instead of guint32 as the type for >> length and lower_bound, >> 2) Changed MonoArray to use gsize instead of guint32 as the type of >> max_length, >> 3) Changed the prototypes for mono_array_new(), mono_array_new_full(), and >> mono_array_new_specific() to >> take gsize's instead of guint32's for their size and bounds parameters. >> I.e.: >> MonoArray* >> -mono_array_new (MonoDomain *domain, MonoClass *eclass, guint32 n); >> +mono_array_new (MonoDomain *domain, MonoClass *eclass, gsize n); >> >> MonoArray* >> mono_array_new_full (MonoDomain *domain, MonoClass *array_class, >> - guint32 *lengths, guint32 *lower_bounds); >> + gsize *lengths, gsize *lower_bounds); >> >> MonoArray * >> -mono_array_new_specific (MonoVTable *vtable, guint32 n); >> +mono_array_new_specific (MonoVTable *vtable, gsize n); >> >> This is the first place an alternative shows up. ¿Which type is better: >> gsize or gssize? The unsigned type gsize better matches the type memory >> allocation functions use (size_t or some variant) and the existing guint32, >> but the signed type better matches the interface presented at the top level >> (i.e. CreateInstance). I chose the unsigned alternative, but an argument >> could be made for the signed type. Another alternative would be to create >> 64 bit versions of the mono_array_new(), mono_array_new_full(), and >> mono_array_new_specific() functions, but that seemed to be too much work. >> >> The changes in *object.c* add the implementations of the >> modified mono_array_new(), mono_array_new_full(), and >> mono_array_new_specific() functions. There was some confusing #defines >> around MYGUINT32_MAX that I did not like, but rather than replace that >> cruft, I extended it. >> >> The changes in *icall-def.h* add two new method calls to the array >> object, CreateInstanceImpl64() and GetLongLength(). It might be possible to >> avoid the CreateInstanceImpl64() definition and make it an overload >> of CreateInstanceImpl() with long parameters, if I was sure on how to do >> that. >> >> The changes to *icall.c* tweak the implementation >> of ves_icall_System_Array_CreateInstanceImpl() to change the type of the >> sizes array and add the implementations >> of ves_icall_System_Array_CreateInstanceImpl64() >> and ves_icall_System_Array_GetLongLength(). >> >> The change to *socket-io.c* was to tweak its usage of mono_array_new_full >> to use gssize instead of int for for the array of lengths. >> >> So all these changes get us to the point where the basic foundation >> is laid down, but there is still the JIT to contend with. It requires a few >> more files to be changed: >> * **mono/mini/mini.c* >> * **mono/mini/jit-icalls.c* >> * **mono/mini/exceptions.cs* >> >> The changes in *mini.c* change the signature of mono_array_new >> and mono_array_new_specific to take "int" instead of "int32". >> >> The changes in *jit-calls.c* do the boring change of guint32's into >> gsize's. >> >> The changes in *exceptions.cs* split the test case for test_0_array_size >> into a 32 and 64 bit variant, because an allocation of Int32.MaxValue can >> succeed after these changes are applied. >> >> There was only one touch-up needed in the the C# compiler: the GetLength >> property code special-inlined code-generation needed to be tweaked since it >> is now possible to get an array length that will not fit into an I4. >> Changing *mcs/mcs/ecore.cs* and *mcs/mcs/expression.cs* to use * >> OpCodes.Conv_Ovf_I4* after *OpCodes.Ldlen* instead of *OpCodes.Conv_I4* >> fixed >> that. >> >> Oh, yeah, and ALL the long method calls in * >> mcs/class/corlib/System/Array.cs* needed to be converted over to >> use CreateInstanceImpl64() and GetLongLength(). The SetValue() and >> GetValue() implementations still need work, but since there are no unit >> tests for those methods, I put them off. >> >> That gets us to the point where we can allocate a large array, but it does >> not let us index a large array. I changed the following files to start the >> process of converting the indexing operations to do bounds checking against >> the now 32/64 bit length of arrays and to index using a 64/32 bit index: >> * **mono/mini/inssel-amd64.brg* >> * **mono/mini/mini-amd64.c* >> * **mono/mini/mini-ops.h* >> * **mono/mini/cpu-amd64.md* >> >> In *inssel-amd64.brg*, I changed the >> macro MONO_EMIT_NEW_AMD64_ICOMPARE_MEMBASE_REG to use a new >> opcode OP_AMD64_COMPARE_MEMBASE_REG_I8 instead >> of OP_AMD64_ICOMPARE_MEMBASE_REG and hacked CEE_LDELEMA to no longer use >> the faster OP_X86_LEA because I was not sure how to generalize it. >> Perhaps MONO_EMIT_NEW_AMD64_ICOMPARE_MEMBASE_REG should be retired and >> explicit I4 and I8 versions substituted where appropriate. >> >> In *mini-amd64.c* I added the OP_AMD64_COMPARE_MEMBASE_REG_I8 opcode as >> being the same as OP_AMD64_ICOMPARE_MEMBASE_REG, except with an operand size >> of 8 bytes instead of 4. >> >> In *mini-ops.h* and *cpu-amd64.md* I added an entries >> for OP_AMD64_COMPARE_MEMBASE_REG_I8 and amd64_compare_membase_reg_i8. >> >> These changes seem to get 64 bit indexing working, and passed all the >> regression tests in 1.2.6, but in 1.9.1 a regression test >> called test_0_regress_75832() breaks. It could be that the changes I made >> in mono/mini are incorrect. I am sure the changes are incomplete, and I >> have not considered what to do to other architectures. >> >> Advice or assistance is greatly appreciated. >> >> Luis F. Ortiz >> Official Mono Modifier >> Interactive Supercomputing, Inc. >> >> PS: Here are the changes proper: >> >> >> >> >> >> _______________________________________________ >> Mono-devel-list mailing list >> Mono-devel-list@lists.ximian.com >> http://lists.ximian.com/mailman/listinfo/mono-devel-list >> >> >
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list