[PATCH] guile: fix smob exports

[PATCH] guile: fix smob exports

[George Barrett]

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);
+  SCM smob_type_name = scm_class_name (smob_type);
+  scm_define (smob_type_name, smob_type);
+  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
+with_test_prefix "test exports" {
+    # For is-a? and <class>
+    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

Re: [PATCH] guile: fix smob exports

[Andrew Burgess]

* 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

Re: [PATCH] guile: fix smob exports

[George Barrett]

On Thu, Jun 03, 2021 at 10:22:54AM +0100, Andrew Burgess wrote:
> > +  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.

Ouch, I forgot to even check! Sorry about that. As for an earliest supported
version, trying to build against 1.8 fails but Fedora still links against 2.0.
So 2.0 sounds about right.

This is a bit of a problem, though. Digging though the code/commit log,
options for accessing smob class values from outside libguile seems to break
down as follows:
 - <2.1.0: Export from (oop goops)
 - =2.1.0: N/A (!!!)
 - >2.1.0: scm_smob_type_class

IIUC Guile's odd minor versions are development/preview releases so the lack
of availability in the 2.1.0 case shouldn't have much impact, but it'll make
for a nice ifdef ladder.

> > +  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.

The class name isn't the same as what gets passed to `scm_make_smob_type';
rather, the supplied name is implicitly enclosed in '<>'. Should I add a
comment saying as much?

> > +# Check that smob classes are exported properly
> 
> Missing '.' at the end here.

Ack.

> > +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.

Sure. Reading it back, I'm not sure anyone could blame you for being confused
;)

Thanks