thr3ads.net - Libguestfs - [Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values [Jan 2022]

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2022-Jan-11 11:15 UTC

[Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

On 01/10/22 16:36, Richard W.M. Jones wrote:> On Mon, Jan 10, 2022 at 02:26:23PM +0100, Laszlo Ersek wrote:
>> Store_field() is apparently documented to never invalidate its last
>> argument by calling the garbage collector, so the code we currently
have
>> in "libosinfo-c.c" is safe.
>
> Or was!  Store_field is a trivial macro around caml_modify, and
> caml_modify _was_ documented as not calling the GC here:
>
>
https://github.com/ocaml/ocaml/blob/b6163a49ee67acf17235b49c177a5f85b8c944db/runtime/memory.c#L609
>
> But that comment is not present on the trunk.  (I'm unclear when it
> was removed - the git history is complicated to follow.)
Given how difficult the ocaml commit history is to follow (due to the
many merges), I could only narrow it down to this:

  *   c8f58f406d57 (MERGE) Merge commit
'7303ac34cad9046746f9f95a7610740efcfd2121' into multicore-merged
  |\
  | * 7303ac34cad9 (ORIG) fix some of the whitespace problems in the source
  * | d73317491e75 (REMOVED) Bootstrap compiler.
  * | 09c794232adb (REMOVE) Rip out the GC.
  * | 5da7dd6d54d6 (RENAME) Replace caml_modify with caml_modify_field.
  |/
  * bd7fa181cb64 (FORK) Fix two makeblocks incorrectly marked as Immutable.  The
lazy-related one is concretely problematic in presence of stronger constant
propagation.

At FORK, we still had the comment (originally from commit 61bd8ace6bdb
("Passage a la version bootstrappee (franchissement du Rubicon)",
1995-05-04).)

After FORK, on a branch, commits RENAME and REMOVE renamed and removed,
correspondingly, caml_modify(), and the comment we're talking about met
the same fate.

After FORK, on a different branch, the original function name and
comment survived.

Before MERGE, ORIG and REMOVED were the last direct commits on the
separate branches.

At MERGE, the code from REMOVED prevailed.

Since MERGE, we've not had the comment in "trunk". (There have
been a
number of commits since MERGE into "trunk", from descendants of ORIG,
namely a whole lot of

  Merging multicore and 4.02.1 branches, part XXX

but in each of those, the comment was not allowed "back in".)

At some point caml_modify_field() was replaced back with caml_modify()
<https://github.com/ocaml-multicore/ocaml-multicore/pull/500>, but the
comment did not return.
>
>> However, the following warning
>> <https://ocaml.org/manual/intfc.html#ss:c-simple-gc-harmony> does
apply to
>> Store_field():
>>
>>> The first argument of Store_field and Store_double_field must be a
>>> variable declared by CAMLparam* or a parameter declared by
CAMLlocal* to
>>> ensure that a garbage collection triggered by the evaluation of the
>>> other arguments will not invalidate the first argument after it is
>>> computed.
>>
>> meaning that the *first* argument of Store_field() *could* be released
or
>> moved by a garbage collection that is triggered by the evaluation of
the
>> last argument. And, in order to protect the first arg from such a
release
>> (or, to be notified if the first arg is moved), the first arg must be
>> (temporarily) registered as a "root" value (with the
"rootness"
>> substituting for other inward edge(s) from other blocks).
>>
>> Keeping in mind the first and last (block) args' *different*
exposures to
>> garbage collection is difficult. Therefore, for every Store_field()
call
>> where the last arg is not an unboxed integer (in practice: Val_bool()),
>> but a block created with caml_copy_*(), track that block (at least
>> temporarily -- "long enough") as a root value, via a new
local CAML value
>> called "copyv".
>>
>> For consistency, update the one
>>
>>   CAMLreturn (caml_copy_string (id))
>>
>> call we have as well.
>>
>> Suggested-by: Richard W.M. Jones <rjones at redhat.com>
>> Ref:
https://listman.redhat.com/archives/libguestfs/2022-January/msg00037.html
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>  convert/libosinfo-c.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/convert/libosinfo-c.c b/convert/libosinfo-c.c
>> index b8e78bec5c1c..ec7c06d379e3 100644
>> --- a/convert/libosinfo-c.c
>> +++ b/convert/libosinfo-c.c
>> @@ -183,23 +183,26 @@ value
>>  v2v_osinfo_os_get_id (value osv)
>>  {
>>    CAMLparam1 (osv);
>> +  CAMLlocal1 (copyv);
>>    const gchar *id;
>>
>>    id = osinfo_entity_get_id (OSINFO_ENTITY(OsinfoOs_t_val (osv)));
>> -  CAMLreturn (caml_copy_string (id));
>> +  copyv = caml_copy_string (id);
>> +  CAMLreturn (copyv);
>>  }
>>
>>  static value
>>  glist_to_value_list (GList *list)
>>  {
>>    CAMLparam0 ();
>> -  CAMLlocal2 (rv, v);
>> +  CAMLlocal3 (rv, v, copyv);
>>    GList *l;
>>
>>    rv = Val_emptylist;
>>    for (l = list; l != NULL; l = l->next) {
>>      v = caml_alloc (2, 0);
>> -    Store_field (v, 0, caml_copy_string (l->data));
>> +    copyv = caml_copy_string (l->data);
>> +    Store_field (v, 0, copyv);
>>      Store_field (v, 1, rv);
>>      rv = v;
>>    }
>> @@ -211,7 +214,7 @@ value
>>  v2v_osinfo_os_get_device_drivers (value osv)
>>  {
>>    CAMLparam1 (osv);
>> -  CAMLlocal3 (rv, v, vi);
>> +  CAMLlocal4 (rv, v, vi, copyv);
>>    OsinfoDeviceDriverList *list;
>>    gint i, len;
>>
>> @@ -230,9 +233,11 @@ v2v_osinfo_os_get_device_drivers (value osv)
>>
>>      vi = caml_alloc (6, 0);
>>      str = osinfo_device_driver_get_architecture (driver);
>> -    Store_field (vi, 0, caml_copy_string (str));
>> +    copyv = caml_copy_string (str);
>> +    Store_field (vi, 0, copyv);
>>      str = osinfo_device_driver_get_location (driver);
>> -    Store_field (vi, 1, caml_copy_string (str));
>> +    copyv = caml_copy_string (str);
>> +    Store_field (vi, 1, copyv);
>>      b = osinfo_device_driver_get_pre_installable (driver);
>>      Store_field (vi, 2, Val_bool (b));
>>      b = osinfo_device_driver_get_signed (driver);
>> @@ -243,7 +248,8 @@ v2v_osinfo_os_get_device_drivers (value osv)
>>      /* Same as OSINFO_DEVICE_DRIVER_DEFAULT_PRIORITY in libosinfo
1.7.0+. */
>>      i64 = 50;
>>  #endif
>> -    Store_field (vi, 4, caml_copy_int64 (i64));
>> +    copyv = caml_copy_int64 (i64);
>> +    Store_field (vi, 4, copyv);
>>      l = osinfo_device_driver_get_files (driver);
>>      Store_field (vi, 5, glist_to_value_list (l));
>>      g_list_free (l);
>> @@ -286,7 +292,7 @@ value
>>  v2v_osinfo_os_get_all_devices (value osv)
>>  {
>>    CAMLparam1 (osv);
>> -  CAMLlocal3 (retvalv, linkv, propsv);
>> +  CAMLlocal4 (retvalv, linkv, propsv, copyv);
>>    g_autoptr (OsinfoDeviceList) dev_list = NULL;
>>    OsinfoList *ent_list;
>>    gint ent_nr;
>> @@ -310,7 +316,8 @@ v2v_osinfo_os_get_all_devices (value osv)
>>        prop_val = osinfo_entity_get_param_value (ent,
device_prop[prop_nr]);
>>        if (prop_val == NULL)
>>          prop_val = "";
>> -      Store_field (propsv, prop_nr, caml_copy_string (prop_val));
>> +      copyv = caml_copy_string (prop_val);
>> +      Store_field (propsv, prop_nr, copyv);
>>      }
>>
>>      linkv = caml_alloc (2, 0);
>>
>> base-commit: f0cea012d0183edf6f7b769c28d5038593f3fe6a
>
> ACK
Pushed as commit 2e8a991c1916, with the following commit message update:
> 1:  30b09ae48afa ! 1:  2e8a991c1916 convert/libosinfo-c.c: turn
caml_copy_*() return blocks into root values
>     @@ -24,11 +24,21 @@
>          released), and then the new reference inside the parent block
would point
>          to garbage.
>
>     -    Store_field() is apparently documented to never invalidate its
last
>     +    Store_field() used to be documented [*] to never invalidate its
last
>          argument by calling the garbage collector, so the code we
currently have
>     -    in "libosinfo-c.c" is safe.
>     +    in "libosinfo-c.c" should be safe.
>
>     -    However, the following warning
>     +    [*] The OCaml code comment
>     +
>     +        [caml_modify] never calls the GC.
>     +
>     +        disappeared in commits 5da7dd6d54d6 and 09c794232adb, on one
of the
>     +        branches between fork point commit bd7fa181cb64 and merge
commit
>     +        c8f58f406d57, and then the comment removal prevailed in the
merge.
>     +        OCaml has not had the comment reinstated since, with
"trunk" currently
>     +        at 284834d31767.
>     +
>     +    Either way, the following warning
>          <https://ocaml.org/manual/intfc.html#ss:c-simple-gc-harmony>
does apply to
>          Store_field():
>
>     @@ -61,6 +71,10 @@
>          Suggested-by: Richard W.M. Jones <rjones at redhat.com>
>          Ref:
https://listman.redhat.com/archives/libguestfs/2022-January/msg00037.html
>          Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>     +    Message-Id: <20220110132623.9113-1-lersek at redhat.com>
>     +    Acked-by: Richard W.M. Jones <rjones at redhat.com>
>     +    [lersek at redhat.com: extend the commit message with the (rough)
history of
>     +     the caml_modify comment]
>
>      diff --git a/convert/libosinfo-c.c b/convert/libosinfo-c.c
>      --- a/convert/libosinfo-c.c
By the way, we have more "offenders" left:

- three in "bundled/libvirt-ocaml/libvirt_c_common.c":
> value
> Val_virconnectcredential (const virConnectCredentialPtr cred)
> {
> ...
>   Store_field (rv, 1, caml_copy_string (cred->prompt));
> value
> Val_virterror (virErrorPtr err)
> {
> ...
>   Store_field (rv, 7, caml_copy_int32 (err->int1));
>   Store_field (rv, 8, caml_copy_int32 (err->int2));
- two in "bundled/libvirt-ocaml/libvirt_c_oneoffs.c":
> CAMLprim value
> ocaml_libvirt_domain_get_cpu_stats (value domv)
> {
> ...
>         Store_field(typed_param, 0, caml_copy_string(params[pos].field));
> value
> ocaml_libvirt_domain_get_all_domain_stats (value connv,
>                                            value statsv, value flagsv)
> {
> ...
>       Store_field (v2, 0, caml_copy_string
(rstats[i]->params[j].field));
I guess those need to be modified in the libvirt-ocaml project first.

Thanks!
Laszlo

Richard W.M. Jones

2022-Jan-11 11:22 UTC

head link

[Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

On Tue, Jan 11, 2022 at 12:15:13PM +0100, Laszlo Ersek
wrote:> By the way, we have more "offenders" left:
> 
> - three in "bundled/libvirt-ocaml/libvirt_c_common.c":
I'm quite sure this happens in a lot of places.

BTW libvirt-ocaml is a separate project, so fixes should go to:

https://libvirt.org/git/?p=libvirt-ocaml.git;a=summary

(and really we should remove all bundled code - I can't recall why it
was added, but it's not needed now.)
> I guess those need to be modified in the libvirt-ocaml project first.
Yup.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Libguestfs - Jan 2022 - [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

[Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

[Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values