FOREIGN-SLOT-SET compiler macro generates invalid code

[Luís Oliveira]

On Wed, Aug 5, 2009 at 5:07 PM, Nikodemus Siivola <email address hidden> wrote:
> 2009/8/5 Vitaly Mayatskikh <email address hidden>:
>
>> Gives me:
>> ; in: LAMBDA NIL
>> ;     (CFFI::FOREIGN-SLOT-SET #:STORE651
>> ;                             (SB-PCL::ACCESSOR-SLOT-VALUE #:TMP650 'RAW)
>> ;                             'MY-STRUCT 'FOO)
>> ; --> SETF LET* MULTIPLE-VALUE-BIND LET PROGN CFFI::MEM-SET CFFI-SYS:%MEM-SET
>> ; --> LET SETF SB-KERNEL:%SET-SAP-REF-8
>> ; ==>
>> ;   (LET #:TMP11
>> ;     #:TMP10
>> ;     #:TMP9)
>> ;
>> ; caught ERROR:
>> ;   Malformed LET bindings: #:TMP11.
>>
>> and several others errors
>>
>> Clisp loads the same code ok. Is it a bug?
>
> Yes, it's a but -- but I believe a bug in CFFI. Explanation follows.
> (Example demonstrating an alternative way to trigger comes right at
> the end.)
>
> In the expansion, this is the problematic form:
>
> (DEFMETHOD (SETF MY-STRUCT-FOO) ((INST MY-STRUCT) NEW)
>  (SETF (CFFI:FOREIGN-SLOT-VALUE (SLOT-VALUE INST 'RAW) 'MY-STRUCT
>                                 'FOO)
>        NEW))
>
> first off, this probably does not do what you think it does: the new value is
> the first argument, not the last. Anyways, digging deeper, the trouble
> arises with
> the result of the SETF expansion here:
>
> (CFFI::FOREIGN-SLOT-SET #:STORE968 (SB-PCL::ACCESSOR-SLOT-VALUE #:TMP967 'RAW)
>                        'MY-STRUCT 'FOO)
>
> Note that PCL has replaced the SLOT-VALUE with ACCESSOR-SLOT-VALUE. Had it
> been a (SETF SLOT-VALUE) it would now be ACCESSOR-SET-SLOT-VALUE.
>
> CFFI has a compiler macro for FOREIGN-SET-SLOT,
>
> (SETF (CFFI:MEM-REF (SB-PCL::ACCESSOR-SLOT-VALUE #:TMP10 'RAW) ':UCHAR 0)
>        #:STORE11)
>
> which in turn has a SETF-expander, producing
>
> (LET* ((#:TMP972
>        ((SB-PCL::.IGNORE.
>          (LOAD-TIME-VALUE
>           (SB-PCL::ENSURE-ACCESSOR 'SB-PCL::READER
>                                    '(SB-PCL::SLOT-ACCESSOR :GLOBAL RAW
>                                      SB-PCL::READER)
>                                    'RAW)))))
>       (#:TMP971 (DECLARE (IGNORE SB-PCL::.IGNORE.)))
>       (#:TMP970
>        (SB-EXT:TRULY-THE (VALUES T &OPTIONAL)
>                          (FUNCALL
>                           (LOCALLY
>                               (DECLARE (SB-EXT:MUFFLE-CONDITIONS
> STYLE-WARNING))
>                             #'(SB-PCL::SLOT-ACCESSOR :GLOBAL RAW
>                                                      SB-PCL::READER))
>                           #:TMP10))))
>  (MULTIPLE-VALUE-BIND
>        (#:STORE973)
>      #:STORE11
>    (PROGN
>      (CFFI::MEM-SET #:STORE973
>                     (LET #:TMP972
>                       #:TMP971
>                       #:TMP970)
>                     ':UCHAR 0)
>      #:STORE973)))
>
> where the malformed LET appears. This is because ACCESSOR-SLOT-VALUE is not a
> settable place, but rather a macro that expands into a LET form.
>
> I'm inclined to blame the compiler-macro for FOREIGN-SLOT-SET, which puts the
> original SLOT-VALE form in a context where it becomes a (SETF SLOT-VALUE.)
> While arguably the code walker should take expansion of compiler-macros into
> account, compiler macros are not allowed the change the semantics of their
> arguments -- which this one does.
>
> Notice how the same error can be provoked without the PCL code walker:
>
> (defun my-bad (x z)
>  (setf (cffi:foreign-slot-value (let ((y (x-y x)))
>                                   (y-foreign y))
>                                 'my-struct
>                                 'foo)
>        z))
>
> Unless the contract of (SETF FOREIGN-SLOT-VALUE) is that the first
> (second) argument
> must be a settable place itself, the compiler-macro is broken.
>
> Cheers,
>
>  -- Nikodemus

[Mark Cox (markcox80)]

Thank you to |3b| on #sbcl for linking me to this report.

The setf expander defined for FOREIGN-SLOT-VALUE calls GET-SETF-EXPANDER on the PTR form. The setf expanders for MEM-REF and MEM-AREF do the same.

I am unsure on why the PTR argument needs to be a place in any of the above.