3

I am porting Lightweight Communications and Marshalling from julia to lisp as it has a better API. I used swig to generate C function calls.

I want to know if this is a safe usage for C pointer or not. here is the create function:

(defun create-lcm (&optional (provider (null-pointer)))
    (let* ((ptr (lcm_create provider))
           (addr (cffi:pointer-address ptr)))
        (tg:finalize ptr (lambda () (lcm_destroy (cffi:make-pointer addr))))
        (if (NULL-POINTER-P ptr)
                (error "lcm creation error"))
        (%create-lcm :pointer ptr :provider provider
                                 :file-descriptor (lcm_get_fileno ptr))))

Question:

  • What is the proper way to finalize C pointer?
  • How to make a test for that?

Any other notes/advices are welcome.

Thanks in advance.

I.Omar
  • 501
  • 7
  • 19
  • 1
    You probably shouldn’t attach a finaliser to a null pointer. It’s bad style to `let` `fd` be `nil` and then `setq` it later. You should just `let` it when you need it. I don’t know enough about what you’re doing to say if it’s otherwise right – Dan Robertson Aug 05 '18 at 16:17
  • 2
    The finalizer function refers to the object it's attached to, which will prevent it from ever being garbage collected. You should wrap the pointer in a suitable box and attach the finalizer to the box (the object created on the last line would probably suffice, assuming the pointer is only ever accessed through it (ie. never assigned to a variable or a slot in another object that might have a longer lifespan)). – jkiiski Aug 05 '18 at 18:52
  • @jkiiski So Is it right now? – I.Omar Aug 06 '18 at 01:47

2 Answers2

3

Here are a few things that were wrong:

  1. Attaching a finaliser to a pointer that might be null
  2. I’m not sure you’re allowed to attach a finaliser to a foreign pointer. Maybe you are.
  3. You need to be careful with finalisers and gc. If the finaliser references the object that it finalises then the object and its finaliser keep each other alive (they can’t be collected at once because a finaliser might store a reference to the object somewhere and then that object would be alive and so shouldn’t have been finalised.

I don’t know if this is right but it is better:

(defun create-lcm (&optional (provider (null-pointer))
  (let ((ptr (lcm_create provider)))
    (when (null-pointer-p ptr)
      (error “lcm creation error”))
    (flet ((finaliser () (lcm_destroy ptr)))
      (let ((result (%create-lcm :pointer ptr :provider provider
                                 :file-descriptor (lcm_get_fileno ptr))))
        (tg:finalize result #'finaliser)
        result))))

Here are some things that are wrong:

  1. If there is an error from %create-lcm or lcm_get_fileno then the finaliser will not run
Dan Robertson
  • 4,315
  • 12
  • 17
  • as @jkiiski pointed out. finalizer function should not attempt to look at object by closing over it because that will prevent it from being garbage collected. so I have to use (cffi:make-pointer addr). – I.Omar Aug 06 '18 at 09:21
  • 1
    The finaliser closes over the `ptr` but it is the finaliser for `result` which it doesn’t close over. – Dan Robertson Aug 06 '18 at 11:48
3

You might want to read about cl-autowrap, which is used notably to wrap the SDL 2 in cl-sdl2. The library provides thin wrappers around pointers that already free memory on finalization.

I think also that the recommended way to use finalizers is only to use them for cleaning up possible leaks, given that you have little control over when and how the cleanup function is executed (e.g. which thread, which dynamic environment).

One way to manage memory is to allocate your structures ahead of time and clean them when you no longer need them (a pool). Or you can define a function or a macro which defines a scope such that memory is allocated when entering it and freed on exit, using unwind-protect:

(defmacro with-lcm ((context &rest options) &body body)
  (let ((internal (gensym))) 
    `(let* ((,internal (create-lcm ,@options))
            (,context ,internal))
       (unwind-protect (progn ,@body)
         (destroy-lcm ,internal)))))
coredump
  • 37,664
  • 5
  • 43
  • 77