Re: [PATCH] guile: fix smob exports

[Andrew Burgess <andrew.burgess@embecosm.com>]

* George Barrett via Gdb-patches <gdb-patches@sourceware.org> [2021-05-20 08:27:50 +1000]:

> Before Guile v2.1[1], calls to `scm_make_smob_type' implicitly
> added the created class to the exports list of (oop goops); v2.1+ does
> not implicitly create bindings in any modules. This means that the GDB
> manual subsection documenting exported types is not quite right when
> GDB is linked against Guile <v2.1 (types are exported from (oop
> goops)) instead of (gdb)) and incorrect when linked against Guile
> v2.1+ (types are not bound to any variables at all!).
> 
> This commit makes a small change to GDB's smob registration machinery
> to make sure registered smobs get exported from the current
> module. This will likely cause warnings to the user about conflicting
> exports if they load both (gdb) and (oop goops) from a GDB linked
> against Guile v2.0, but it shouldn't impact functionality (and seemed
> preferable to trying to un-export bindings from (oop goops) if v2.0
> was detected).
> 
> [1]: This changed with Guile commit
>      28d0871b553a3959a6c59e2e4caec1c1509f8595
> 
> gdb/ChangeLog:
> 
> 2021-05-20  George Barrett  <bob@bob131.so>
> 
> 	* guile/scm-gsmob.c (gdbscm_make_smob_type): Export registered
> 	smob type from the current module.
> 
> gdb/testsuite/ChangeLog:
> 
> 2021-05-20  George Barrett  <bob@bob131.so>
> 
> 	* gdb.guile/scm-gsmob.exp (test exports): Add tests to make
> 	sure the smob types currently listed in the GDB manual get
> 	exported from the (gdb) module.
> ---
>  gdb/guile/scm-gsmob.c                 |  9 ++++++++-
>  gdb/testsuite/gdb.guile/scm-gsmob.exp | 28 +++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/guile/scm-gsmob.c b/gdb/guile/scm-gsmob.c
> index c623b07d26c..a8ab63a67b5 100644
> --- a/gdb/guile/scm-gsmob.c
> +++ b/gdb/guile/scm-gsmob.c
> @@ -96,7 +96,8 @@ gdbscm_is_gsmob (SCM scm)
>    return slot != NULL;
>  }
>  
> -/* Call this to register a smob, instead of scm_make_smob_type.  */
> +/* Call this to register a smob, instead of scm_make_smob_type.
> +   Exports the created smob type from the current module.  */
>  
>  scm_t_bits
>  gdbscm_make_smob_type (const char *name, size_t size)
> @@ -104,6 +105,12 @@ gdbscm_make_smob_type (const char *name, size_t size)
>    scm_t_bits result = scm_make_smob_type (name, size);
>  
>    register_gsmob (result);
> +
> +  SCM smob_type = scm_smob_type_class (result);

I don't know if we have a defined earliest supported version of guile,
but I tried building again guile 2.0.14 with this patch applied and
the build failed at the above line as scm_smob_type_class was not
known.  Before this patch 2.0.14 would build fine.

> +  SCM smob_type_name = scm_class_name (smob_type);
> +  scm_define (smob_type_name, smob_type);

There's also scm_c_define which looks like it would do the same thing
without us having to lookup the type name?  We could just pass name
through directly.  NOTE: I'm no guile expert, so if what I just said
is rubbish, please do say so.

> +  scm_module_export (scm_current_module (), scm_list_1 (smob_type_name));
> +
>    return result;
>  }
>  
> diff --git a/gdb/testsuite/gdb.guile/scm-gsmob.exp b/gdb/testsuite/gdb.guile/scm-gsmob.exp
> index 90c32df7dda..d2a77198a23 100644
> --- a/gdb/testsuite/gdb.guile/scm-gsmob.exp
> +++ b/gdb/testsuite/gdb.guile/scm-gsmob.exp
> @@ -66,3 +66,31 @@ set prop_list [lsort $prop_list]
>  verbose -log "prop_list: $prop_list"
>  gdb_test "gu (print (sort (map car (object-properties arch)) (lambda (a b) (string<? (symbol->string a) (symbol->string b)))))" \
>      "= \\($prop_list\\)" "object-properties"
> +
> +# Check that smob classes are exported properly

Missing '.' at the end here.

> +with_test_prefix "test exports" {
> +    # For is-a? and <class>

Could you change this to 'Import (oop goops) for is-a? and <class>.',
this comment really puzzled me for a while.

> +    gdb_scm_test_silent_cmd "gu (use-modules (oop goops))" "import goops"
> +    gdb_test_no_output "gu (define-syntax-rule (gdb-exports-class? x) (is-a? (@ (gdb) x) <class>))"
> +
> +    gdb_test "gu (print (gdb-exports-class? <gdb:arch>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:block>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:block-symbols-iterator>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:breakpoint>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:command>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:exception>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:frame>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:iterator>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:lazy-string>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:objfile>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:parameter>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:pretty-printer>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:pretty-printer-worker>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:progspace>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:symbol>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:symtab>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:sal>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:type>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:field>))" "= #t"
> +    gdb_test "gu (print (gdb-exports-class? <gdb:value>))" "= #t"
> +}
> -- 
> 2.31.1