2009/8/14 Vegard Nossum <vegard.nos...@gmail.com>:
> 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.
We have fixup sites (call fixup) and static field fixup sites. The
list .fixup_site_backpatch_list
contains fixup sites (call fixup). I agree that since we have
different kinds of fixup sites
we should change the naming to avoid confusion. We need this list now
to set ->instr_ptr of all
call fixup sites after code emission. We didn't need this before,
because we used pointer to struct insn, from which we obtained machine
offset. Now, we want to free all compilation structures after method
compilation, so we can no longer
use pointers to struct insn in fixup sites because fixup sites are
used after method is compiled.
>
>> @@ -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?
We attach static fixup sites to the class during instruction selection,
> (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
>
--
Tomek Grabiec
------------------------------------------------------------------------------
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
