[Mono-dev] Interpreter?

[Paolo Molaro]

On 07/03/07 Andreas Färber wrote:
> First, it includes changes to eglib/configure.in to make it compile  
> on OS X and adds VPATH support to facilitate compiling for ppc and  
> ppc64.

I suggest sending the eglib changes separately (note that the vararg
changes need to be inside a #ifdef conditional).

> Next, it reorganizes the ppc-codegen.h (header still needs some  
> changes Miguel requested) - instructions are now listed  

I'm sorry you have been led to do this kind of change: it is not
acceptable to do extensive cleanups together with code changes:
please send us only the code changes so we can actually review them
(this will also remove one third from the patch length).

> Then, the mini and arch ppc files were updated with #ifdefs for ppc64  
> doing all kinds of necessary changes. I have some FIXMEs/CHECKMEs in  
> there, especially some numeric offsets and alignments are unclear to  
> me. This is where I suspect the most issues.
> Finally, all occurrences of ppc in the sources were checked and adapted.

I gave a very quick read of the changes, here are a few suggestions:
*) you shold minimize the number of inserted #ifdefs. A patch that does:

+#ifdef __ppc64__
+               offset += 8;
+#else
                offset += 4;
+#endif

is not acceptable, while if it did:

-               offset += 4;
+               offset += sizeof (gpointer);

it would be applied right away.
Similar issues are with changes like:
+#ifdef __ppc64__
+       __asm__ volatile("ld    %0,0(r1)" : "=r" (sframe));
+#else
        __asm__ volatile("lwz   %0,0(r1)" : "=r" (sframe));
+#endif

Just add to the header something like:
#ifdef __ppc64__
#define PPC_LOAD_REG_INS "ld"
#else
#define PPC_LOAD_REG_INS "lwz"
#endif

and replace the asm statements with:
        __asm__ volatile (PPC_LOAD_REG_INS "   %0,0(r1)" : "=r" (sframe));

Something similar should be done also for the native code emitting
macros. Instead of:

+#ifdef __ppc64__
+               ppc_ld  (buf, ppc_r0, (guint16)G_STRUCT_OFFSET(MonoLMF, previous_lmf) >> 2, ppc_r3);
+               ppc_std (buf, ppc_r0, (guint16)G_STRUCT_OFFSET(MonoLMF, previous_lmf) >> 2, ppc_r11);
+#else
                ppc_lwz (buf, ppc_r0, G_STRUCT_OFFSET(MonoLMF, previous_lmf), ppc_r3);
                ppc_stw (buf, ppc_r0, G_STRUCT_OFFSET(MonoLMF, previous_lmf), ppc_r11);
+#endif

define in ppc_codegen:

#ifdef __ppc64__
#define ppc_load_reg(a,b,c,d) ppc_ld(a,b,(((c) >> 2) & 0xffff),d)
#define ppc_store_reg(a,b,c,d) ppc_std(a,b,(((c) >> 2) & 0xffff),d)
#else
#define ppc_load_reg(a,b,c,d) ppc_lwz(a,b,c,d)
#define ppc_store_reg(a,b,c,d) ppc_stw(a,b,c,d)
#endif

This will make the changes so much cleaner that it will be possible to
more easily review them and find the issues (and it will also allow
us to commit them incrementally to svn).

*) cpu-g5.md should become cpu-ppc64.md

*) the ppc_load changes are not acceptable: add a ppc_load_sequence
or something like that that will emit lis/ori on 32 and the full
sequence on 64 bit and use that where it is required.

*) mono/io-layer/atomic.h: the changes here can't be skipped, they are
used in many parts of the runtime and without the 64 bit support there
can be memory corruption.

*) the emit_tls_access optimizations should likely be removed unless
they have been tested explicitly (they look a bit bogus).


> compilation of System.dll using the new mono (or on execution of a  
> hello world assembly) it aborts with empty stacktrace:
> 
> MONO_PATH="../../class/lib/basic:$MONO_PATH" /Users/andreas/Mono/mono- 
> ppc64/mono/runtime/mono-wrapper  ../../class/lib/basic/mcs.exe / 
> codepage:65001   -d:NET_1_1 -d:ONLY_1_1 -d:BOOTSTRAP_WITH_OLDLIB - 
> debug /noconfig -define:XML_DEP -r:System.Xml.dll -target:library - 
> out:System.dll  @System.dll.sources
> Stacktrace:
[...]
> (gdb) run ../../../testassembly.exe
> Starting program: /Users/andreas/Mono/mono-ppc64/mono/mono/mini/ 
> mono ../../../testassembly.exe
> Reading symbols for shared libraries .++ done
> 
> Program received signal SIGBUS, Bus error.
> 0x0000000000000000 in ?? ()

This looks like the result of a jump to NULL.
What are the results of running make rcheck in mini/? That is the first
thing that needs checking. If it fails in the same way before starting
the tests it is likely related to something earlier on in the
initialization. The first thing that comes to mind is: doesn't ppc64 on OSX
use function descriptors like all the other ppc64 platforms? If it does
use them this would be the first thing to fix as any indirect call would
pretty much fail.

> I haven't been able to locate the error(s) for several months now and  

Well, I've been here and I don't remember seeing any jit-related request
for help:) Follow the above suggestions for now and we can both try to
get the changes in svn and get the bugs fixed.

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better