[Mono-dev] [PATCH] Add mixed-mode assembly support on Windows (now build with cygwin as well)

[Zoltan Varga]

         Hi,

> > - image.c: mono_cli_rva_image_map is part of our public api, so its
> signature
> >  cannot be changed, you can rename it to mono_cli_rva_image_map_internal,
> and
> >  change the signature of that.
> >
>
>  I had a look at "C:\Program Files\Mono\include" and it doesn't contain any
> declaration. Is this public for sure? If it is I'll do what you suggested.
>

I was wrong, this isn't a public header file after all.

>
>
> > - The usage of 'raw_data' is a bit confusing: sometimes it is used as an
> address
> >  (like in mono_cli_rva_image_map), sometimes it is used as a handle
> (everywhere
> >  else). Which is correct ?
> >
>
>  HMODULE is the actual pointer to the base address (position of the DOS
> header) of the module. Also note that there is no such thing as HINSTANCE it
> is a heritage of the Win16 shared address space and HMODULE and HINSTANCE
> are the same in Win32. In Win32s (Win32 API on Win16 that is not really
> Win32 compatible) HMODULE is definitely not the base address but someting
> else that may be a handle. So raw_data is actually the raw_data but unlike
> the current implementation sections are mapped to the actual virtual
> addresses. Even checks in map functions could be omitted but I didn't remove
> them to ensure that the image is correct.
>
>
>
> > - cil-coff.h: Please don't declare constants in the middle of a a
> >  structure definition.
> >
>
>  OK. I actually tried to follow what seemed to me being the coding
> convention of that file.
>
>
>
> > - gc.c: The comment about mono_thread_attach () might be incorrect, but
> the
> >  thread_started_event stuff is definitely needed to avoid races when
> >  the runtime is
> >  shut down before the finalizer thread can start. So I don't think we
> >  can remove this.
> >
>
>  As I see there can be no race because there only are "mono_thread_current
> () == gc_thread" checks. Even if "mono_thread_current ()" returns NULL (that
> is unlikely I think) nothing will happen on the thread if the finalizer
> thread in not initialized yet. Actual work is only done by
> "finalizer_thread" that is being executed only in the finalizer thread. If
> for some reason "mono_gc_cleanup ()" is called before the finalizer thread
> is initialized "WaitForSingleObjectEx" will wait for two seconds for the
> finalizers and thus for the finalizer thread being initialized. If it fails
> to initialize within two seconds finalizers won't run of course but the same
> is true for finalizers that werent processed in two seconds so I see no big
> difference. If really need "WaitForSingleObjectEx (thread_started_event,
> INFINITE, FALSE);" can be moved just before "WaitForSingleObjectEx
> (shutdown_event, 2000, FALSE)" but I don't see any reason to do so. If I'm
> wrong please let me know.
>

I'm still not sure about this, so if this is really needed on win32,
then lets try keeping
the original code, but putting the Wait inside an #ifndef PLATFORM_WIN32 +
adding a comment about why it is inside the #ifndef.

Other than that, I think the patch is ok to check in. One thing that
is missing is
tests. Since the patch changes so many places in the runtime, if there are no
automated tests, the functionality will probably get broken by changes
since people
have no way of noticing when they broke it.

                       Zoltan

>  Kornél
>