[Mono-dev] CoreCLR patches

[Sebastien Pouliot]

Hello Rodrigo,

Sorry I should have said that the "platform detection" part is still "in
flux". I sent an email earlier to moonlight-list on this subject (but
it's stuck in moderation since I used the wrong email address).

Thanks

On Fri, 2009-03-13 at 11:48 -0300, Rodrigo Kumpera wrote:
> Hi Sebastien,
> 
> I have some comments on parts of your mono changes:
> 
> 
> @@ -16,6 +16,7 @@
>  #include "security-core-clr.h"
>  
>  gboolean mono_security_core_clr_test = FALSE;
> +char* mono_security_core_clr_prefix = NULL;
> 
> Why this field is not static?

it should be

> Why do we even need it? mono_security_enable_core_clr stores on it and
> nobody reads from.

it's not used right now, but I "expect" it to be soon...

> +
> +static char* platform_code_assemblies [] = {
> +    "mscorlib",
> 
> This should be const.
> 
> 
> +/*
> + * mono_security_core_clr_determine_platform_image:
> + *
> + *   Check if this image represent platform code
> + */
> +gboolean
> +mono_security_core_clr_determine_platform_image (MonoImage *image)
> +{
> +    /* FIXME: existing code not safe enough wrt XAP XXX we need to
> better define *platform* code (e.g. location)
> 
> This part of the patch comments out a chunk of code that just doesn't
> fit moon, right?
> Then either remove the dead code or use explicit lines to add comment
> start/end, otherwise it gets pretty tricky to figure it out.

Both the commented (did not work on moon-unit) and un-commented (works
for moon-unit) code are not safe and meant to be replaced before
committing.

> 
> @@ -203,7 +203,7 @@
>  mono_get_exception_class    (void);
>  
>  void
> -mono_security_enable_core_clr (void);
> +mono_security_enable_core_clr (const char *prefix);
> 
> This breaks our ABI, we can't change that function this way.
> I know it's a silly requirement for a function that hardly anyone
> could be using, but 
> breaking our ABI should not be taken lightly.
> Create a new function and leave a comment stating that
> mono_security_enable_core_clr is deprecated
> and will be eventually removed. This is bound to happen once we do a
> release with sgen-gc enabled.

np, once the "platform" stuff is resolved I'll add a separate function
(if still needed) to set the platform code prefix

> @@ -4481,28 +4507,60 @@
>      mono_emit_method_call (cfg, thrower, NULL, NULL);
>  }
>  
> +static MonoMethod*
> +unwrap (MonoMethod *method)
> +{
> 
> 
> Please use a more meaningful name for this function.

sure

>  static gboolean
>  method_is_safe (MonoMethod *method)
>  {
> -    /*
> +    /* FIXME: looks somewhat incomplete
> 
> I think this is just dead code used during the initial development of
> core-clr security. 

yes, the comment was to trigger a definitive answer of that :)

> 
> 
> 2009/3/13 Sebastien Pouliot <sebastien.pouliot at gmail.com>
>         Hello,
>         
>         With the set of attached patches all existing moon-unit tests
>         pass. It
>         also pass the SecurityCriticalTest[1] - which calls from
>         application
>         code (i.e. transparent) every visible SL2 API decorated as
>         Critical.
>         
>         The runtime patch mainly avoid unneeded, repetitive calls so
>         it should
>         help performance - however I made those changes so it was
>         easier to set
>         breakpoints using gdb/xdb, real optimizations are for another
>         day ;-)
>         
>         Next steps are to fix CoreCLR-related ReflectionTest[1] and
>         ReflectionEmitTest[1] unit tests and, of course, more
>         tests/testing.
>         
>         Thanks
>         Sebastien
>         
>         [1] in SVN but ignored at the moment
>         
>         
>         _______________________________________________
>         Mono-devel-list mailing list
>         Mono-devel-list at lists.ximian.com
>         http://lists.ximian.com/mailman/listinfo/mono-devel-list
>         
>