Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix translation of struct-typedefs #152

Merged
merged 1 commit into from Feb 26, 2020
Merged

Conversation

enometh
Copy link

@enometh enometh commented Feb 21, 2020

CFFI now always translates aggregate struct values to lisp property
lists. Earlier struct values which were defined by types introduced
by defctype were translated as foreign pointers.

One consequence of this change is that defcvar to a foreign structure
will always produce an immutable lisp list. Users will have to resort
to FOREIGN-SYMBOL-VALUE in order to access structs to modify them -
and use the (:POINTER VAR) syntax of WITH-FOREIGN-SLOTS to access and
modify fields within the struct.

  • src/early-types.lisp: (defctype): use translatable-foreign-type
    instead of enhanced-foreign-type.

  • tests/structs.lisp: New tests: (misnamed) STRUCT-VALUES.FSBV.1
    STRUCT-VALUES.FSBV.1 Updated tests: STRUCT.ALIGNMENT.1
    STRUCT.ALIGNMENT.2 STRUCT.ALIGNMENT.3 STRUCT.ALIGNMENT.4
    STRUCT.ALIGNMENT.5 STRUCT.ALIGNMENT.6 STRUCT.ALIGNMENT.7
    STRUCT.NESTED-SETF STRUCT.ALIGNMENT.8

Copy link
Member

@luismbo luismbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a minor a suggestion regarding the defcvar -> defvar substitution.

Could you check whether the manual needs to be updated to reflect this change, either in the descriptions or in the examples?

@@ -118,7 +118,7 @@

(defctype s-s-ch (:struct s-s-ch))

(defcvar "the_s_s_ch" s-s-ch)
(defvar *the-s-s-ch* (foreign-symbol-pointer "the_s_s_ch"))
Copy link
Member

@luismbo luismbo Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea.

This has the drawback of not working with saved cores since it'll permanently store that pointer. Not a big deal for the test suite, but since this code might be used as an example, maybe change defvar to define-symbol-macro.

Alternatives:

  1. Keep the defcvar and sprinkle get-var-ptr throughout.
  2. Change the defctypes to something that doesn't translate structs, either with a custom type or by adding some option to the :struct type that skips translation. Dubious. Probably too much work.

I'm fine with your proposed solution (with defvar changed to define-symbol-macro).

@@ -141,10 +141,10 @@

(defctype s-s-short (:struct s-s-short))

(defcvar "the_s_s_short" s-s-short)
(defvar *the-s-s-short* (foreign-symbol-pointer "the_s_s_short"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space there between defvar and the variable name. 🕵️‍♂️

CFFI now always translates aggregate struct values to lisp property
lists.  Earlier struct values which were defined by types introduced
by defctype were translated as foreign pointers.

One consequence of this change is that defcvar to a foreign struct
will always produce an immutable lisp list.  To modify the struct the
user first has to get at the pointer to the struct via
FORIEGN-SYMBOL-VALUE or GET-VAR-POINTER.  To modify nested structs the
user has to get at the pointer to the field via FORIEGN-SLOT-POINTER,
- or use the the (:POINTER VAR) syntax of WITH-FOREIGN-SLOTS.

* src/early-types.lisp: (defctype): use translatable-foreign-type
instead of enhanced-foreign-type.

* tests/structs.lisp: New tests: (misnamed) STRUCT-VALUES.FSBV.1
STRUCT-VALUES.FSBV.1 Updated tests: STRUCT.ALIGNMENT.1
STRUCT.ALIGNMENT.2 STRUCT.ALIGNMENT.3 STRUCT.ALIGNMENT.4
STRUCT.ALIGNMENT.5 STRUCT.ALIGNMENT.6 STRUCT.ALIGNMENT.7
STRUCT.NESTED-SETF STRUCT.ALIGNMENT.8
@luismbo luismbo merged commit c5d140f into cffi:master Feb 26, 2020
@luismbo
Copy link
Member

luismbo commented Feb 26, 2020

Thanks!

@enometh enometh deleted the fix-defctype branch March 5, 2020 04:05
@luismbo
Copy link
Member

luismbo commented May 12, 2020

I'll have to revert this as it's causing issues with magicl:

quil-lang/magicl#106

defcfun expansion before: https://pastebin.com/tjeyKh39 (see lines labeled ; *****)
defcfun expansion after: https://pastebin.com/X6eGV4ga

test case: https://pastebin.com/7pLMVcFA

@luismbo
Copy link
Member

luismbo commented Apr 16, 2021

@enometh
Copy link
Author

enometh commented Apr 17, 2021

Luis, check your mail - on June 20 2020 I sent you the fix to cffi libffi which would would have made this pull request work - correctly

The original problem is with old code in cffi libffi and not a bug in this patch which was reverted.

I wrote:

I've pushed the fix for review on my personal branch as
https://github.com/enometh/cffi/commit/74a15af823e3bfec7256e9a572c67962ed985f60.patch
Again mentioned on cffi-devel on Nov 06 2020 in a reply to Steve Nunez.

Unfortunately there was no feedback from him

I had hoped you would test this patch against the launchpad problems noted above - i believe it would indeed solve them

vert could be reverted.

translate-objects-ret-fix-translatable-foreign.patch.txt

@luismbo
Copy link
Member

luismbo commented Apr 17, 2021

I see, sorry. Emails have this nasty habit of piling up and disappearing. I've pushed the stuff you sent me by email to this pull request: #181 Did I miss anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants