2009/8/13 Tomek Grabiec <tgrab...@gmail.com>:
> This is necessary for basic block freeing after compilation to be possible.
>
> Signed-off-by: Tomek Grabiec <tgrab...@gmail.com>

Hi,

I feel very confused by this patch, and I wish there was an
explanation of the changes too.

> diff --git a/include/jit/compilation-unit.h b/include/jit/compilation-unit.h
> index d3366f7..4e56868 100644
> --- a/include/jit/compilation-unit.h
> +++ b/include/jit/compilation-unit.h
> @@ -91,6 +91,12 @@ struct compilation_unit {
>        struct radix_tree *safepoint_map;
>
>        /*
> +        * Contains list of fixup_sites inside this compilation unit's
> +        * code which needs to patched after code emission.
> +        */
> +       struct list_head fixup_site_backpatch_list;
> +
> +       /*
>         * Contains native pointers of exception handlers.  Indices to
>         * this table are the same as for exception table in code
>         * attribute.

Why do we need this now and not before? What sort of fixup sites are
these? We have a couple, IIRC: trampoline fixup, static fixup.

> @@ -446,12 +446,25 @@ void fixup_static(struct vm_class *vmc)
>        list_for_each_entry_safe(this, next,
>                &vmc->static_fixup_site_list, vmc_node)
>        {
> -               struct vm_field *vmf = this->vmf;
> -               void *site_addr = buffer_ptr(this->cu->objcode)
> -                       + this->insn->mach_offset;
> -               void *new_target = vmc->static_values + vmf->offset;
> +               struct vm_field *vmf;
> +               void *new_target;
> +               bool is_compiled;
> +
> +               /*
> +                * We should not fixup sites from methods which are
> +                * not yet compiled.
> +                */
> +               pthread_mutex_lock(&this->cu->mutex);
> +               is_compiled = this->cu->is_compiled;
> +               pthread_mutex_unlock(&this->cu->mutex);
> +
> +               if (!is_compiled)
> +                       continue;
> +
> +               vmf = this->vmf;
> +               new_target = vmc->static_values + vmf->offset;
>
> -               cpu_write_u32(site_addr + 2, (unsigned long) new_target);
> +               cpu_write_u32(this->insn_ptr + 2, (unsigned long) new_target);
>
>                list_del(&this->vmc_node);
>

I don't get this part either. Why would we even try to call
fixup_static() with patching sites that are not compiled yet? And if
we do, how can we make sure that they ARE compiled afterwards? (I
think) this function is only supposed to be called once per class,
when the class is initialized... Hm, I have a feeling that our static
handling is horribly broken anyway, and I think we should try to fix
that before adding more cludges to it.

(For example, jit_magic_trampoline locking is screaming for help and
the locking in fixup_static_at is a bit confused (we should never
reach the end of the function -- better insert a die() there or
something), in any case, what ARE our locking rules wrt nesting of
vmc->mutex and compilation_unit->mutex...)


Vegard

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Jatovm-devel mailing list
Jatovm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jatovm-devel

Reply via email to