Re: [RFC PATCH v3 1/5] Libffi Static Trampolines

[DJ Delorie <dj@redhat.com>]

"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> writes:
>>> +case "$target" in
>>> +     *-linux*)
>>> +       AC_DEFINE(FFI_EXEC_STATIC_TRAMP, 1,
>>> +                 [Define this if you want statically defined trampolines])
>>> +     ;;
>>> +esac
>> 
>> Ok.  Might want to conditional for the arches that are supported too.
>> 
>
> ok. I will add the arches although the code currently handles that by testing
> ffi_tramp_arch == NULL.

/me ponders...

Is the intent here that a Linux client has to check for static
trampolines in configure (i.e. it's on linux) but *also* check if it's
supported at runtime?  If we could minimize the number of checks, that
would help the developer.  If the API is always present (or always
hidden, so that the client need do nothing), that's best.

Ideally, the client would need do nothing regardless, but that only
works if the static trampoline functionality is 100% hidden.

> My intention is to allow applications and libraries that currently use dynamic code
> to use the libffi API to eliminate their dynamic code. The maintainers of these
> apps/libs would rewrite their dynamic code into static code. All they need is a
> way to pass data to their static code (that uses PC-relative access). The libffi
> API will let them do that.

Ok, as long as there's a use case.

Note that adding public APIs is MUCH easier than removing them later...

> My intention is to always have the API functions defined in the library. If
> FFI_EXEC_STATIC_TRAMP is defined, then, the API will supply static trampolines.
> Else, the stub API functions will return an error and the caller can fall back
> on his existing method if any.

Ok.  I guess that answers one of my earlier questions (above) ;-)

>>> @@ -943,12 +966,17 @@ void *
>>>  ffi_data_to_code_pointer (void *data)
>>>  {
>>>    msegmentptr seg = segment_holding (gm, data);
>>> +
>> 
>> Extraneous but ok.
>
> This is needed.

I meant the extra blank line ;-)

>>> +  if (table->nfree == gtramp.ntramp && gtramp.ntables > 1)
>> 
>> You can't compare these.  The first is a count of *trampolines* and the
>> second is a count of *tables*.
>> 
>
> There are two conditions here:
>
> If (table->nfree == gtramp.ntramp) is true, then all of the table's slots are free.

This reads as "If the number of trampolines equals the number of tables"
which are not comparable.  If you're doing some math magic hack here to
get the right answer, it *really* needs a big comment explaining how it
works.

>> tramp_table_del unlinks the table, but doesn't deallocate its
>> resources.  Are these resources lost/leaked?
>> 
>
> The table list contains tables that have free slots. If all slots of a table
> are allocated, the table is removed from the list. Its trampoline resources
> are held inside closures. When those closures are freed, the table will be back
> on the list. When the table gets back all of its trampolines, it will be freed
> along with all of its trampolines.

Ah, ok.