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.

Sign up for GitHub

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

Jump to bottom

Add compiler macro foreign-type-size #343

Closed
wants to merge 2 commits into from
Closed

Add compiler macro foreign-type-size #343

wants to merge 2 commits into from

Conversation

samuel-hunter
Copy link
Contributor

@samuel-hunter samuel-hunter commented Nov 7, 2022 •
edited

Fixes #340. Depends on #345

The compiler macro is a first-pass that should fix most use cases without breakage. It checks to see if the foreign type is constant, and if so, expands into its foreign type size, falling back to the old form if an error is signaled.

Before the compiler macro:

* (disassemble '(lambda () (cffi:foreign-type-size '(:pointer :uint32))))
; disassembly for (LAMBDA ())
; Size: 32 bytes. Origin: #x5365FC5C                          ; (LAMBDA ())
; 5C:       498B4510         MOV RAX, [R13+16]                ; thread.binding-stack-pointer
; 60:       488945F8         MOV [RBP-8], RAX
; 64:       488B15BDFFFFFF   MOV RDX, [RIP-67]                ; '(:POINTER
                                                              ;   :UINT32)
; 6B:       B902000000       MOV ECX, 2
; 70:       FF7508           PUSH QWORD PTR [RBP+8]
; 73:       B8A29D3F50       MOV EAX, #x503F9DA2              ; #<FDEFN CFFI:FOREIGN-TYPE-SIZE>
; 78:       FFE0             JMP RAX
; 7A:       CC10             INT3 16                          ; Invalid argument count trap
NIL

After (on SBCL 2.2.5 64-bit)

* (disassemble (lambda () (cffi:foreign-type-size '(:pointer :uint32))))
; disassembly for (LAMBDA ())
; Size: 21 bytes. Origin: #x5365FECC                          ; (LAMBDA ())
; CC:       498B4510         MOV RAX, [R13+16]                ; thread.binding-stack-pointer
; D0:       488945F8         MOV [RBP-8], RAX
; D4:       BA10000000       MOV EDX, 16
; D9:       488BE5           MOV RSP, RBP
; DC:       F8               CLC
; DD:       5D               POP RBP
; DE:       C3               RET
; DF:       CC10             INT3 16                          ; Invalid argument count trap
NIL

Tested on SBCL and CCL with no additional test errors.

@phoe
Copy link
Contributor

phoe commented Nov 7, 2022

Fixes #340

phoe
phoe reviewed Nov 7, 2022
View reviewed changes
src/early-types.lisp Outdated Show resolved Hide resolved
src/early-types.lisp Outdated Show resolved Hide resolved
src/early-types.lisp
@@ -164,6 +164,14 @@ Signals an error if FOREIGN-TYPE is undefined."))
(:documentation
"Return the size in bytes of a foreign type."))

(define-compiler-macro foreign-type-size (&whole form foreign-type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any tests that should be performed for this compiler macro alone? I assume that you could call the compiler macro function yourself to ensure that it provides correct results on some test cases.

@sionescu
Copy link
Member

sionescu commented Nov 7, 2022

@samuel-hunter Copy QUOTEDP, CONSTANTP, and EVAL-CONSTANT from https://github.com/sionescu/static-vectors/blob/master/src/constantp.lisp and use them.
We'll figure out later how to share the code.

@samuel-hunter
Copy link
Contributor Author

samuel-hunter commented Nov 7, 2022 •
edited

@samuel-hunter Copy QUOTEDP, CONSTANTP, and EVAL-CONSTANT from https://github.com/sionescu/static-vectors/blob/master/src/constantp.lisp and use them. We'll figure out later how to share the code.

Preexisting compiler macros ought to benefit from this if grafted into CFFI. I think this may be best split into its own PR

@sionescu
Copy link
Member

sionescu commented Nov 7, 2022

Why a new PR ?

@samuel-hunter
Copy link
Contributor Author

My thinking, at least, is that it separates two units of work: Adding a compiler macro for foreign-type-size, and then augmenting all compiler macros to use QUOTEDP, CONSTANTP, and EVAL-CONSTANT (many, many compiler macros I see here use cl:constantp and cl:eval).

@samuel-hunter
Copy link
Contributor Author

I'll make a new branch/PR at least for now

@sionescu
Copy link
Member

sionescu commented Nov 7, 2022

@samuel-hunter Please rebase to master. I added the utils in d4216a3.

@sionescu
Copy link
Member

sionescu commented Nov 7, 2022

I opened #344 for converting existing compiler macros.

sionescu
sionescu reviewed Nov 7, 2022
View reviewed changes
src/early-types.lisp Outdated Show resolved Hide resolved
sionescu
sionescu requested changes Nov 7, 2022
View reviewed changes
src/early-types.lisp Outdated
;; executed in a null lexical environment.
(if (constantp (macroexpand foreign-type))
`(load-time-value (locally (declare (notinline foreign-type-size)) ,form))
(if (constant-form-p (macroexpand foreign-type))
Copy link
Member

Choose a reason for hiding this comment

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

Let's do the conversion in another PR. You can cherry-pick this commit to another branch.

@samuel-hunter samuel-hunter mentioned this pull request Nov 8, 2022
Open
sionescu
sionescu requested changes Nov 8, 2022
View reviewed changes
src/early-types.lisp Outdated Show resolved Hide resolved
@samuel-hunter
Add compiler macro foreign-type-size
a978e49 
The compiler macro is a first-pass that should fix most use cases
without breakage. It checks to see if the foreign type is constant, and
if so, expands into its foreign type size, falling back to the old form
if an error is signaled.
@sionescu sionescu closed this in af33f6e Nov 8, 2022
@phoe phoe mentioned this pull request Nov 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phoe phoe phoe left review comments

@sionescu sionescu sionescu requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expanding foreign-type-size for standard types
3 participants
@samuel-hunter @phoe @sionescu