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

[Kornél Pál]

Hi,

Thank you very much for the review and your comments.

> From: Zoltan Varga
> To: Kornél Pál
> Cc: mono-devel
> Sent: Friday, April 25, 2008 4:45 PM
> Subject: Re: [Mono-dev] [PATCH] Add mixed-mode assembly support on Windows 
> (now build with cygwin as well)


> - appdomain.h: This is a public header file, so the new functions
>   should go somewhere
>   else, like domain-internals.h
> - domain.c: The code here could be put into a function in corree.c.
> - Put mono_module_handle and coree_handle into some header file like
>   coree.h, and
>   there definitons into corree.c for example.

> - corree.h: Please include <config.h>, since that contains the
>   definition of PLATFORM_WIN32.

OK.

> - 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.

> - 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.

Kornél