Lisp object that refers to a C struct

Lisp object that refers to a C struct

[Eli Zaretskii]

What is the recommended method of getting a Lisp object that
represents a C struct?

It seems like a pseudo-vector is used for this, but in my case it
looks like an overkill: my struct has no Lisp members.  All I need is
a Lisp object that will serve as descriptor for the struct, and that
can be used to extract a pointer to the struct, for C code to access
that struct.

There's Lisp_Save_Value, which looks very promising, but why does its
documentation say it's for record_unwind_protect?  Is there anything
in it which would preclude its use outside unwinding?

Re: Lisp object that refers to a C struct

[Stefan Monnier]

> It seems like a pseudo-vector is used for this, but in my case it
> looks like an overkill: my struct has no Lisp members.

You can use either a pseudo-vector, or s Lisp_Misc.

> There's Lisp_Save_Value, which looks very promising,

Lisp_Save_Value is a Lisp_Misc.

> but why does its documentation say it's for record_unwind_protect?
> Is there anything in it which would preclude its use
> outside unwinding?

It doesn't look very pretty when printed, so it's generally better not
to expose it to users too much.  If your value will usually not be
visible to Elisp code, then Lisp_Save_Value is perfectly fine.
Otherwise, better define your own type so that its printed
representation can be more helpful, indicating what it's used for (I
presume it's for a file-watch-descriptor).


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Mon, 15 Oct 2012 21:07:21 -0400
> 
> > There's Lisp_Save_Value, which looks very promising,
> 
> Lisp_Save_Value is a Lisp_Misc.
> 
> > but why does its documentation say it's for record_unwind_protect?
> > Is there anything in it which would preclude its use
> > outside unwinding?
> 
> It doesn't look very pretty when printed, so it's generally better not
> to expose it to users too much.  If your value will usually not be
> visible to Elisp code, then Lisp_Save_Value is perfectly fine.

OK.

> Otherwise, better define your own type so that its printed
> representation can be more helpful, indicating what it's used for

How do I define my own type?  In particular, what to do with gcmarkbit
in Lisp_Misc?

> (I presume it's for a file-watch-descriptor).

Yes.

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> Otherwise, better define your own type so that its printed
>> representation can be more helpful, indicating what it's used for
> How do I define my own type?

Either by adding one more lisp_misc or one more pseudovector.
For either of them, you need to add the corresponding new tag to their
enumeration (Lisp_Misc_Type or pvec_type).  For lisp_misc you
additionally need to add your entry to the union type (but make sure
the first word has the same structure as the others, starting with
a 16bit type and a 1bit markbit) and make sure the overall size of the
union type is not increased.

Then you need to add the corresponding switch branches in print.c and in
alloc.c.

> In particular, what to do with gcmarkbit in Lisp_Misc?

The markbit represents whether or not the object you created was found
by the tracing GC, so just set it like we do for the other types.

The other question is "when should we free the C struct to which this
new lisp_misc points"?  And that depends.  You could have the GC free it
when it finds your lisp_misc can be freed, or you could have instead
a notion of "deleted file-watcher" (like we have for
buffers/windows/...) which is when the underlying C struct has been
freed (but of course, this state needs to be represented, e.g. by
setting the pointer to NULL, which means that you need to be able to
enumerate all the file-watcher objects (or the only one, if you enforce
there can only be one) corresponding to a particular C struct).


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Tue, 16 Oct 2012 09:11:25 -0400
> 
> >> Otherwise, better define your own type so that its printed
> >> representation can be more helpful, indicating what it's used for
> > How do I define my own type?
> 
> Either by adding one more lisp_misc or one more pseudovector.
> For either of them, you need to add the corresponding new tag to their
> enumeration (Lisp_Misc_Type or pvec_type).  For lisp_misc you
> additionally need to add your entry to the union type (but make sure
> the first word has the same structure as the others, starting with
> a 16bit type and a 1bit markbit) and make sure the overall size of the
> union type is not increased.
> 
> Then you need to add the corresponding switch branches in print.c and in
> alloc.c.

Can I just use PVEC_OTHER instead?

In any case, which one, pseudovector or misc, is better suited for
this particular job, in your opinion?  What would you use?

> > In particular, what to do with gcmarkbit in Lisp_Misc?
> 
> The markbit represents whether or not the object you created was found
> by the tracing GC, so just set it like we do for the other types.
> 
> The other question is "when should we free the C struct to which this
> new lisp_misc points"?  And that depends.  You could have the GC free it
> when it finds your lisp_misc can be freed, or you could have instead
> a notion of "deleted file-watcher" (like we have for
> buffers/windows/...) which is when the underlying C struct has been
> freed (but of course, this state needs to be represented, e.g. by
> setting the pointer to NULL, which means that you need to be able to
> enumerate all the file-watcher objects (or the only one, if you enforce
> there can only be one) corresponding to a particular C struct).

This sounds dangerous in my case.  The struct holds crucial data used
by a separate thread which watches the changes and receives
notifications.  It is unthinkable to let any code outside of the one
specifically written for this to free that struct, because the
associated thread will go down in flames and take Emacs with it.
There's special code (already written and tested) that takes care of
shutting down the watcher thread and freeing the associated resources
in an orderly way.  That code must be run whenever the object is
GC'ed, at the very least.  It would be even better not to leave this
to GC at all, but instead delete the object whenever the watch is
canceled, since otherwise we leave behind a thread that does nothing
except wasting system resources, for a time interval whose length
cannot be predicted or controlled.

Is there a clean way of doing that?

P.S.  I must say that the more I learn about the "make your own
object" path the less I like it.  In the case in point, it sounds like
pure overhead, with no benefits at all.  To do everything I need with
the watcher struct, all I need is a pointer to it, which can easily be
disguised as a Lisp integer.  (This is how the code actually works
now.)  Such a "handle" will be barely visible to Lisp, its only use is
to cancel an existing watch, or examine its associated file name and
the details of the watch request (which _are_ Lisp objects).  There's
no need to manage the struct itself, as it already manages itself.
But look what I need to do just to produce a first-class Lisp object
from it.  And I still don't see what would be the benefits.  E.g., a
Lisp integer prints quite nicely; something like "#<watch 12345678>"
doesn't seem significantly prettier.  What am I missing?

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> Then you need to add the corresponding switch branches in print.c and in
>> alloc.c.
> Can I just use PVEC_OTHER instead?

No, for the same reason as Lisp_Save_Value: you can't give a nice
printed representation.

> In any case, which one, pseudovector or misc, is better suited for
> this particular job, in your opinion?  What would you use?

I guess I'd go with the Lisp_Misc.

>> The other question is "when should we free the C struct to which this
>> new lisp_misc points"?  And that depends.  You could have the GC free it
>> when it finds your lisp_misc can be freed, or you could have instead
>> a notion of "deleted file-watcher" (like we have for
>> buffers/windows/...) which is when the underlying C struct has been
>> freed (but of course, this state needs to be represented, e.g. by
>> setting the pointer to NULL, which means that you need to be able to
>> enumerate all the file-watcher objects (or the only one, if you enforce
>> there can only be one) corresponding to a particular C struct).

> This sounds dangerous in my case.

Of course, but it's unavoidable: you want to export to Elisp a pointer to
a C struct.  So you have to deal with the two risks: failing to free the
object, and having Elisp code use the object after it was freed.

> notifications.  It is unthinkable to let any code outside of the one
> specifically written for this to free that struct, because the
> associated thread will go down in flames and take Emacs with it.

Of course.  The code to free the C struct will be the one you write
specifically for it, nobody else knows how to free it.

> That code must be run whenever the object is GC'ed, at the very least.
> It would be even better not to leave this to GC at all, but instead
> delete the object whenever the watch is canceled, since otherwise we
> leave behind a thread that does nothing except wasting system
> resources, for a time interval whose length cannot be predicted
> or controlled.

If you make sure there's at most 1 file-watcher object per C struct,
then you can easily zero-out its pointer-field after freeing the
C struct, so you can make sure you protect yourself from
dangling pointers.  And if you make sure the GC calls you back when
freeing the file-watcher, then you can make sure that you don't leak the
C structs.

> Is there a clean way of doing that?

AFAIK this *is* the clean way.

> pure overhead, with no benefits at all.  To do everything I need with
> the watcher struct, all I need is a pointer to it, which can easily be
> disguised as a Lisp integer.  (This is how the code actually works
> now.)

What happens if someone passes you this same integer some time after
you've freed the C struct?


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Tue, 16 Oct 2012 17:43:26 -0400
> 
> > pure overhead, with no benefits at all.  To do everything I need with
> > the watcher struct, all I need is a pointer to it, which can easily be
> > disguised as a Lisp integer.  (This is how the code actually works
> > now.)
> 
> What happens if someone passes you this same integer some time after
> you've freed the C struct?

It won't be found in the list of watches, so the command to remove
that watch will say "Invalid watch descriptor", and Lisp-level code
will not find the corresponding Lisp data structure.

Re: Lisp object that refers to a C struct

[Stephen J. Turnbull]

Eli Zaretskii writes:

 > > What happens if someone passes you this same integer some time after
 > > you've freed the C struct?

More interesting, what happens if somebody chooses "I feel lucky" and
passes you a random integer that happens to be a "live" watch?

ISTM it fails safe (ie, doesn't crash).  But it's not good.  So really
you want this to be entirely internal somehow.  The only way to do
that is to have an opaque type that you can only get a pointer to from
the watch constructor.

All-I-need-to-know-about-ABCs-I-learned-in-kindergarten-ly y'rs,

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> What happens if someone passes you this same integer some time after
>> you've freed the C struct?
> It won't be found in the list of watches, so the command to remove
> that watch will say "Invalid watch descriptor", and Lisp-level code
> will not find the corresponding Lisp data structure.

So the integer doesn't encode the pointer, instead it's an index into
a table of C structs.  Like POSIX file-descriptors.
That would work, of course.


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: "Stephen J. Turnbull" <stephen@xemacs.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>     emacs-devel@gnu.org
> Date: Wed, 17 Oct 2012 15:21:40 +0900
> 
> Eli Zaretskii writes:
> 
>  > > What happens if someone passes you this same integer some time after
>  > > you've freed the C struct?
> 
> More interesting, what happens if somebody chooses "I feel lucky" and
> passes you a random integer that happens to be a "live" watch?

I don't follow: what do you mean "somebody passes me a random
integer"?

There are only 2 APIs that expose this integer to Lisp: one that
starts watching for file changes, and another that cancels an existing
watch.  The former returns the integer we are talking about, the
latter accepts that integer and uses it to access the underlying C
struct, shut down the thread that was doing the watch, and delete the
struct and all the other resources the watch was using.

So if they pass me the integer that happens to reference a live watch,
they can only do that through the "remove" API, in which case the
watch will stop.

Or did you mean something else?

> ISTM it fails safe (ie, doesn't crash).

It's fail-safe, yes (well, barring bugs ;-).

> But it's not good.

Why not?  If a luser asked to shut down a watch, and just "happened"
to specify a live one, she gets what she asked for.  It's not like
Emacs _needs_ those watches for some of its internal workings.

> So really you want this to be entirely internal somehow.  The only
> way to do that is to have an opaque type that you can only get a
> pointer to from the watch constructor.

I don't think we have opaque data types in Emacs.

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Tue, 16 Oct 2012 17:43:26 -0400
> 
> > In any case, which one, pseudovector or misc, is better suited for
> > this particular job, in your opinion?  What would you use?
> 
> I guess I'd go with the Lisp_Misc.

OK, thanks.

> The code to free the C struct will be the one you write
> specifically for it, nobody else knows how to free it.

Then the code that GC's this new Lisp object will have to be entirely
specific to the layout of the struct I'm using, right?  It won't be
suitable even for the similar-but-different inotify code, right?  If
so, it's too bad: I hoped I could come up with some Lisp object that
could be a wrapper for an arbitrary C struct, so that any similar
feature that will need to pass opaque objects to Lisp could do that,
instead of adding yet another object type.

If it's possible to do this in some generic way, could you please
explain how?

> > That code must be run whenever the object is GC'ed, at the very least.
> > It would be even better not to leave this to GC at all, but instead
> > delete the object whenever the watch is canceled, since otherwise we
> > leave behind a thread that does nothing except wasting system
> > resources, for a time interval whose length cannot be predicted
> > or controlled.
> 
> If you make sure there's at most 1 file-watcher object per C struct,
> then you can easily zero-out its pointer-field after freeing the
> C struct, so you can make sure you protect yourself from
> dangling pointers.  And if you make sure the GC calls you back when
> freeing the file-watcher, then you can make sure that you don't leak the
> C structs.

You mean, have the free_my_object function (to be called by GC) call
my own code?  Or is there a more generic way?

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Wed, 17 Oct 2012 09:34:40 -0400
> 
> >> What happens if someone passes you this same integer some time after
> >> you've freed the C struct?
> > It won't be found in the list of watches, so the command to remove
> > that watch will say "Invalid watch descriptor", and Lisp-level code
> > will not find the corresponding Lisp data structure.
> 
> So the integer doesn't encode the pointer, instead it's an index into
> a table of C structs.

No, I have no table right now.  I could add one, of course.

Right now, the struct is allocated and its pointer is put into an
alist as a Lisp integer, and also returned, as a Lisp integer, to the
caller of the "start-watching" API.  The caller is supposed to manage
her watches, and pass the right integer when she wants to cancel a
watch.

Re: Lisp object that refers to a C struct

[Davis Herring]

> Then the code that GC's this new Lisp object will have to be entirely
> specific to the layout of the struct I'm using, right?  It won't be
> suitable even for the similar-but-different inotify code, right?  If
> so, it's too bad: I hoped I could come up with some Lisp object that
> could be a wrapper for an arbitrary C struct, so that any similar
> feature that will need to pass opaque objects to Lisp could do that,
> instead of adding yet another object type.
> 
> If it's possible to do this in some generic way, could you please
> explain how?

Python addresses this with the CObject API.  (It is being supplanted by
a newer Capsule API, but it's nice and simple as a comparison point.)
http://docs.python.org/c-api/cobject.html

Basically, you make your single new Lisp type contain a pair of
pointers: the first points to your struct, and the other points to a
function that frees the struct and does whatever other cleanup.  This
latter can also be used as a type tag (so that you can't accidentally
use this to cast a eli_file_watcher_struct* to a
stefan_smie_cache_struct* and turn a Lisp bug into a crash in C),
although that might involve creating several wrappers for free() in the
trivial case.  (The CObjects use a third pointer that can be used to
disambiguate; either way works.)

The advantage of this approach is that the pointer marshaling code (and
the object-printing code and so forth) need only be written once, and
only one true Lisp type is used.  The disadvantage is of course that two
layers of type checking must be performed: the usual CONSP variety, and
then you check the function pointer (or other tag) once you know that
it's a struct-wrapper of some sort.

>> If you make sure there's at most 1 file-watcher object per C struct,
>> then you can easily zero-out its pointer-field after freeing the
>> C struct, so you can make sure you protect yourself from
>> dangling pointers.  And if you make sure the GC calls you back when
>> freeing the file-watcher, then you can make sure that you don't leak the
>> C structs.
> 
> You mean, have the free_my_object function (to be called by GC) call
> my own code?  Or is there a more generic way?

Yet another approach is to make sure that your code keeps a reference to
the struct-wrapper Lisp object and frees that reference only when it
destroys the underlying thread.  Then if the thread is alive, the struct
is; you can have a field in the struct (again like the killed buffer)
that means "this has no thread, so you can't use it from Lisp anymore".
 Then when the Lisp client gives up and drops their reference, the
struct is freed.

(Of course, you could do it the other way around: have GC destroy the
thread when the last reference to the struct-wrapper dies.  Then client
Lisp merely drops their references to the wrapper when they're done,
although there is some nondeterminism about when GC will happen.)

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Lisp object that refers to a C struct

[Stephen J. Turnbull]

Eli Zaretskii writes:

 > I don't follow: what do you mean "somebody passes me a random
 > integer"?

   [...]

 > So if they pass me the integer that happens to reference a live watch,
 > they can only do that through the "remove" API, in which case the
 > watch will stop.

You got it.

 > Why not?  If a luser asked to shut down a watch, and just "happened"
 > to specify a live one, she gets what she asked for.  It's not like
 > Emacs _needs_ those watches for some of its internal workings.

It doesn't now, no.

 > > So really you want this to be entirely internal somehow.  The only
 > > way to do that is to have an opaque type that you can only get a
 > > pointer to from the watch constructor.
 > 
 > I don't think we have opaque data types in Emacs.

Of course we do.  Starting with `cons'.

Re: Lisp object that refers to a C struct

[Stefan Monnier]

> Right now, the struct is allocated and its pointer is put into an
> alist as a Lisp integer,

How do you convert the integer into the C struct pointer?


        Stefan

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> The code to free the C struct will be the one you write
>> specifically for it, nobody else knows how to free it.
> Then the code that GC's this new Lisp object will have to be entirely
> specific to the layout of the struct I'm using, right?

I said "free" not "GC's".  The GC decide what to free, and takes care of
freeing most of the objects in the heap.  The code that frees
the Lisp_Misc file-watcher will be the generic GC code.  And that
generic code will have to trigger your specific code that frees the
C struct.

> It won't be suitable even for the similar-but-different inotify code,
> right?

Depends, you can make it generic by having the Lisp_Misc object have
2 slots: one containing the pointer to the C struct and another pointing
to the matching freeing function.


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 17 Oct 2012 16:23:19 -0400
> 
> > Right now, the struct is allocated and its pointer is put into an
> > alist as a Lisp integer,
> 
> How do you convert the integer into the C struct pointer?

  struct foo *pwatch = (struct foo *)XLI (watch_descriptor);

where watch_descriptor is a Lisp integer.

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 17 Oct 2012 16:27:39 -0400
> 
> >> The code to free the C struct will be the one you write
> >> specifically for it, nobody else knows how to free it.
> > Then the code that GC's this new Lisp object will have to be entirely
> > specific to the layout of the struct I'm using, right?
> 
> I said "free" not "GC's".  The GC decide what to free, and takes care of
> freeing most of the objects in the heap.  The code that frees
> the Lisp_Misc file-watcher will be the generic GC code.  And that
> generic code will have to trigger your specific code that frees the
> C struct.
> 
> > It won't be suitable even for the similar-but-different inotify code,
> > right?
> 
> Depends, you can make it generic by having the Lisp_Misc object have
> 2 slots: one containing the pointer to the C struct and another pointing
> to the matching freeing function.

OK, but who will call this freeing function?  E.g., I don't see a call
to free_misc in gc_sweep, it just puts every Lisp_Misc on the free
list.  Do I add the call to the freeing function in gc_sweep, similar
to the call to unchain_marker for markers?

Re: Lisp object that refers to a C struct

[Paul Eggert]

On 10/17/2012 01:46 PM, Eli Zaretskii wrote:
>> How do you convert the integer into the C struct pointer?
>   struct foo *pwatch = (struct foo *)XLI (watch_descriptor);
> 
> where watch_descriptor is a Lisp integer.

Won't this have problems on platforms where
(EMACS_INT) pwatch < MOST_NEGATIVE_FIXNUM,
or where MOST_POSITIVE_FIXNUM < (EMACS_INT) pwatch?

Also, suppose someone makes up a random integer
and then passes it as the watch descriptor --
wouldn't Emacs dump core?

Re: Lisp object that refers to a C struct

[Stefan Monnier]

> Also, suppose someone makes up a random integer
> and then passes it as the watch descriptor --
> wouldn't Emacs dump core?

That's what I was getting at.


        Stefan

Re: Lisp object that refers to a C struct

[Stefan Monnier]

> OK, but who will call this freeing function?  E.g., I don't see a call
> to free_misc in gc_sweep, it just puts every Lisp_Misc on the free
> list.  Do I add the call to the freeing function in gc_sweep, similar
> to the call to unchain_marker for markers?

Yes, you need to add the call to gc_sweep.


        Stefan

Re: Lisp object that refers to a C struct

[Stephen J. Turnbull]

Stefan Monnier writes:

 > > Also, suppose someone makes up a random integer
 > > and then passes it as the watch descriptor --
 > > wouldn't Emacs dump core?
 > 
 > That's what I was getting at.

I inferred that Eli is already checking it against a list of active
watches (ie, will "fail safe"); otherwise, you could dump core on a
double free anyway.

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> Date: Wed, 17 Oct 2012 15:08:52 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> On 10/17/2012 01:46 PM, Eli Zaretskii wrote:
> >> How do you convert the integer into the C struct pointer?
> >   struct foo *pwatch = (struct foo *)XLI (watch_descriptor);
> > 
> > where watch_descriptor is a Lisp integer.
> 
> Won't this have problems on platforms where
> (EMACS_INT) pwatch < MOST_NEGATIVE_FIXNUM,
> or where MOST_POSITIVE_FIXNUM < (EMACS_INT) pwatch?

Given what XLI does, I don't see how this could happen, with
watch_descriptor being a Lisp integer.

> Also, suppose someone makes up a random integer
> and then passes it as the watch descriptor --
> wouldn't Emacs dump core?

No, because, like Stephen  says, the descriptor is first validated
against a list of known ones.

Re: Lisp object that refers to a C struct

[Paul Eggert]

On 10/17/2012 09:50 PM, Eli Zaretskii wrote:
>> Won't this have problems on platforms where
>> > (EMACS_INT) pwatch < MOST_NEGATIVE_FIXNUM,
>> > or where MOST_POSITIVE_FIXNUM < (EMACS_INT) pwatch?

> Given what XLI does, I don't see how this could happen

To be honest I haven't followed all the back-and-forth on this,
but in general it's not safe to convert a pointer to
an Emacs fixnum, as this can lose information: the payload
of an Emacs fixnum is typically narrower than a pointer.

Re: Lisp object that refers to a C struct

[Juanma Barranquero]

On Thu, Oct 18, 2012 at 6:50 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> No, because, like Stephen  says, the descriptor is first validated
> against a list of known ones.

In that case, wouldn't it be better to have the integer be an index
into a table, like Stefan suggested? It's faster to check that it is
in range than compare it with an arbitrarily long list of known
pointers.

    Juanma

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> Date: Thu, 18 Oct 2012 00:20:11 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> On 10/17/2012 09:50 PM, Eli Zaretskii wrote:
> >> Won't this have problems on platforms where
> >> > (EMACS_INT) pwatch < MOST_NEGATIVE_FIXNUM,
> >> > or where MOST_POSITIVE_FIXNUM < (EMACS_INT) pwatch?
> 
> > Given what XLI does, I don't see how this could happen
> 
> To be honest I haven't followed all the back-and-forth on this,
> but in general it's not safe to convert a pointer to
> an Emacs fixnum, as this can lose information: the payload
> of an Emacs fixnum is typically narrower than a pointer.

A pointer that is properly aligned has no problem with this.  After
all, that's how we produce every Lisp object out of a pointer.

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Thu, 18 Oct 2012 10:52:21 +0200
> Cc: Paul Eggert <eggert@cs.ucla.edu>, emacs-devel@gnu.org
> 
> On Thu, Oct 18, 2012 at 6:50 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > No, because, like Stephen  says, the descriptor is first validated
> > against a list of known ones.
> 
> In that case, wouldn't it be better to have the integer be an index
> into a table, like Stefan suggested?

It's easy to do that (of course, at a price of some slightly more
complicated memory management), but I think Stefan still prefers the
"new object" solution.

> It's faster to check that it is in range than compare it with an
> arbitrarily long list of known pointers.

That's not what the code does.  It calls Fassoc_quit to find the
descriptor in the list of known live watches (which will still be
needed under the table suggestion), and then validates the pointer
itself, just in case (which is also independent of the table
suggestion).

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> In that case, wouldn't it be better to have the integer be an index
>> into a table, like Stefan suggested?
> It's easy to do that (of course, at a price of some slightly more
> complicated memory management),

I don't understand why the memory management would care about which
integer you use to represent file-watcher descriptors.

> but I think Stefan still prefers the "new object" solution.

I generally like stronger types, so you're probably right, but an "index
into a table" doesn't necessarily sound so terribly bad, if you have
such a table anyway (BTW, is this table usable by Lisp, e.g. can they
get the list of current file-watchers?).

>> It's faster to check that it is in range than compare it with an
>> arbitrarily long list of known pointers.
> That's not what the code does.  It calls Fassoc_quit to find the
> descriptor in the list of known live watches (which will still be
> needed under the table suggestion),

Isn't Fassoc_quit going to "compare it with an arbitrarily long list"?
To me this "list of known live watches" sounds exactly like the "table"
I mention above.

> and then validates the pointer itself, just in case (which is also
> independent of the table suggestion).

What means "validate"?


        Stefan

Re: Lisp object that refers to a C struct

[Paul Eggert]

On 10/18/2012 04:09 AM, Eli Zaretskii wrote:
>> but in general it's not safe to convert a pointer to
>> > an Emacs fixnum, as this can lose information: the payload
>> > of an Emacs fixnum is typically narrower than a pointer.
> A pointer that is properly aligned has no problem with this.

It can have problems on hosts where USE_LSB_TAG is 0,
because the conversion to Emacs fixnum can lose high-order
bits in the pointer.

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Juanma Barranquero <lekktu@gmail.com>,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Thu, 18 Oct 2012 08:33:59 -0400
> 
> >> In that case, wouldn't it be better to have the integer be an index
> >> into a table, like Stefan suggested?
> > It's easy to do that (of course, at a price of some slightly more
> > complicated memory management),
> 
> I don't understand why the memory management would care about which
> integer you use to represent file-watcher descriptors.

It doesn't.  I meant the need to manage the table itself, grow it when
needed, etc.

> > but I think Stefan still prefers the "new object" solution.
> 
> I generally like stronger types, so you're probably right, but an "index
> into a table" doesn't necessarily sound so terribly bad, if you have
> such a table anyway

I don't have a table.  What I have is an association list.  I used the
same method as Rüdiger in his inotify patch.

> (BTW, is this table usable by Lisp, e.g. can they get the list of
> current file-watchers?).

Yes, it's an alist.  But it currently isn't exposed to Lisp, neither
as a variable nor via an API.  It could be, of course.  But since
Rüdiger didn't, and since we didn't discuss in detail how this feature
will be used from Lisp, I didn't want to introduce APIs whose
necessity is not clear.

> >> It's faster to check that it is in range than compare it with an
> >> arbitrarily long list of known pointers.
> > That's not what the code does.  It calls Fassoc_quit to find the

I meant Fassoc, of course.

> > descriptor in the list of known live watches (which will still be
> > needed under the table suggestion),
> 
> Isn't Fassoc_quit going to "compare it with an arbitrarily long list"?
> To me this "list of known live watches" sounds exactly like the "table"
> I mention above.

Yes, but since it's an alist, I get its memory management for free.

> > and then validates the pointer itself, just in case (which is also
> > independent of the table suggestion).
> 
> What means "validate"?

Call w32_valid_pointer_p, and in addition verify that the struct
pointed to by it has the correct magic signature.

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> Date: Thu, 18 Oct 2012 09:42:14 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> On 10/18/2012 04:09 AM, Eli Zaretskii wrote:
> >> but in general it's not safe to convert a pointer to
> >> > an Emacs fixnum, as this can lose information: the payload
> >> > of an Emacs fixnum is typically narrower than a pointer.
> > A pointer that is properly aligned has no problem with this.
> 
> It can have problems on hosts where USE_LSB_TAG is 0,
> because the conversion to Emacs fixnum can lose high-order
> bits in the pointer.

We are talking about Windows-specific code, so I don't think this
danger is real.

Re: Lisp object that refers to a C struct

[Stefan Monnier]

> It doesn't.  I meant the need to manage the table itself, grow it when
> needed, etc.

To me "table" doesn't imply "array".  It's just some kind of
data-structure that keeps the elements at hand.  It can be a list, an
array, a tree, a has-table, you name it.

>> (BTW, is this table usable by Lisp, e.g. can they get the list of
>> current file-watchers?).
> Yes, it's an alist.  But it currently isn't exposed to Lisp, neither
> as a variable nor via an API.  It could be, of course.  But since
> Rüdiger didn't, and since we didn't discuss in detail how this feature
> will be used from Lisp, I didn't want to introduce APIs whose
> necessity is not clear.

Note that if we use integers instead of a new type, we can't free
those structs when the Lisp code loses the last reference to the
file-watcher.

> Call w32_valid_pointer_p, and in addition verify that the struct
> pointed to by it has the correct magic signature.

Why is that needed?

Can you give a precise description of your alist?


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: lekktu@gmail.com,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Thu, 18 Oct 2012 18:09:46 -0400
> 
> > It doesn't.  I meant the need to manage the table itself, grow it when
> > needed, etc.
> 
> To me "table" doesn't imply "array".  It's just some kind of
> data-structure that keeps the elements at hand.  It can be a list, an
> array, a tree, a has-table, you name it.

If we use a Lisp data structure, then the same issue of putting a bare
pointer into that started this thread it pops up again, doesn't it?

Anyway, a Lisp data structure is what I have now.

> >> (BTW, is this table usable by Lisp, e.g. can they get the list of
> >> current file-watchers?).
> > Yes, it's an alist.  But it currently isn't exposed to Lisp, neither
> > as a variable nor via an API.  It could be, of course.  But since
> > Rüdiger didn't, and since we didn't discuss in detail how this feature
> > will be used from Lisp, I didn't want to introduce APIs whose
> > necessity is not clear.
> 
> Note that if we use integers instead of a new type, we can't free
> those structs when the Lisp code loses the last reference to the
> file-watcher.

The list is maintained by 2 primitives, which take care of that.  And
the list itself is statically protected from GC.  If some disaster
strikes and the list is somehow thrashed or released by some evil
magic, then all we have is a bunch of threads watching file-change
notifications that no one can hear, threads which can only be
stopped by shutting down Emacs.  Not too bad, considering the doomsday
scenario under which it can happen.

> > Call w32_valid_pointer_p, and in addition verify that the struct
> > pointed to by it has the correct magic signature.
> 
> Why is that needed?

To make sure we never dereference a pointer that doesn't point to the
watch object.  Since the remove-watch API accepts a Lisp integer, it
could be called with any arbitrary integer value.

IOW, I don't want to crash, even if somehow a bad pointer is found in
the alist described below.

> Can you give a precise description of your alist?

It's an alist of cons cells like this:

   (DESCRIPTOR . CALLBACK)

where DESCRIPTOR is a pointer to the C struct as a Lisp integer, and
CALLBACK is a function to call when the file notification event comes
in.  A single cons cell describes a single file-change notification
request; each such request allocates another C struct (and thus gets
another DESCRIPTOR) and starts another thread.

As I wrote, the reason for this design of the alist was to be 100%
compatible with what Rüdiger did.  Otherwise, I could keep CALLBACK in
the C struct as well, for example.

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> > It doesn't.  I meant the need to manage the table itself, grow it when
>> > needed, etc.
>> To me "table" doesn't imply "array".  It's just some kind of
>> data-structure that keeps the elements at hand.  It can be a list, an
>> array, a tree, a has-table, you name it.
> If we use a Lisp data structure, then the same issue of putting a bare
> pointer into that started this thread it pops up again, doesn't it?
> Anyway, a Lisp data structure is what I have now.

In any case, the code now does keep a table of those C structs, and they
are not reclaimed when the Lisp code loses the last "handle".
Instead they are only reclaimed when the Lisp code calls
remove-watch explicitly.

This design means we don't need a "finalizer" (i.e. code in gc_sweep
that calls us back to free the C struct), but it also means that we may
"leak" those C structs if the Lisp code just forgets to `remove-watch'.
I think that's OK for now.

>> > Call w32_valid_pointer_p, and in addition verify that the struct
>> > pointed to by it has the correct magic signature.
>> Why is that needed?
> To make sure we never dereference a pointer that doesn't point to the
> watch object.  Since the remove-watch API accepts a Lisp integer, it
> could be called with any arbitrary integer value.

But as long as the table is not corrupted, there's no risk of such
a thing happening anyway.

> IOW, I don't want to crash, even if somehow a bad pointer is found in
> the alist described below.

OK, so it's just done out of paranoia.  That's fine, but please make it
clear in the code, e.g. by placing it in an eassert.

> As I wrote, the reason for this design of the alist was to be 100%
> compatible with what Rüdiger did.  Otherwise, I could keep CALLBACK in
> the C struct as well, for example.

I think it's OK.  Encoding C pointers in Lisp integers is pretty ugly,
so we should make sure that no part of the design depends on it (so it
can be changed in the future) and we should also make it very clear in
the doc that Elisp code should not rely on those handles being integers
(treat them as black boxes instead).


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: lekktu@gmail.com, eggert@cs.ucla.edu, emacs-devel@gnu.org
> Date: Fri, 19 Oct 2012 10:44:32 -0400
> 
> >> > It doesn't.  I meant the need to manage the table itself, grow it when
> >> > needed, etc.
> >> To me "table" doesn't imply "array".  It's just some kind of
> >> data-structure that keeps the elements at hand.  It can be a list, an
> >> array, a tree, a has-table, you name it.
> > If we use a Lisp data structure, then the same issue of putting a bare
> > pointer into that started this thread it pops up again, doesn't it?
> > Anyway, a Lisp data structure is what I have now.
> 
> In any case, the code now does keep a table of those C structs, and they
> are not reclaimed when the Lisp code loses the last "handle".
> Instead they are only reclaimed when the Lisp code calls
> remove-watch explicitly.

That's true.  The reason is that the C struct cannot be reclaimed as
long as the watcher thread runs, because that thread constantly
dereferences a pointer to the struct.  The remove-watch API takes care
of orderly shutting down the thread (which is not trivial, because it
is mostly stuck in a system call), and then frees the struct.

> This design means we don't need a "finalizer" (i.e. code in gc_sweep
> that calls us back to free the C struct), but it also means that we may
> "leak" those C structs if the Lisp code just forgets to `remove-watch'.

Correct.

> I think that's OK for now.

I concur.

> >> > Call w32_valid_pointer_p, and in addition verify that the struct
> >> > pointed to by it has the correct magic signature.
> >> Why is that needed?
> > To make sure we never dereference a pointer that doesn't point to the
> > watch object.  Since the remove-watch API accepts a Lisp integer, it
> > could be called with any arbitrary integer value.
> 
> But as long as the table is not corrupted, there's no risk of such
> a thing happening anyway.

Right.

> > IOW, I don't want to crash, even if somehow a bad pointer is found in
> > the alist described below.
> 
> OK, so it's just done out of paranoia.  That's fine, but please make it
> clear in the code, e.g. by placing it in an eassert.

Will do.

> > As I wrote, the reason for this design of the alist was to be 100%
> > compatible with what Rüdiger did.  Otherwise, I could keep CALLBACK in
> > the C struct as well, for example.
> 
> I think it's OK.  Encoding C pointers in Lisp integers is pretty ugly,
> so we should make sure that no part of the design depends on it (so it
> can be changed in the future)

Well, the way to get the pointer to the struct given the descriptor
and vice versa _will_ have to change if the design changes.  Other
than that, nothing depends on that.

> and we should also make it very clear in the doc that Elisp code
> should not rely on those handles being integers (treat them as black
> boxes instead).

They are documented as "descriptors".  I will see if there's a place
to make the point you want to drive home.

Anyway, I guess I'm waiting for Rüdiger to install his changes, is
that right?

Re: Lisp object that refers to a C struct

[Stefan Monnier]

> Anyway, I guess I'm waiting for Rüdiger to install his changes, is
> that right?

I don't think he has access rights yet.  So we're waiting for someone to
install his patch.  But I'm beginning to feel like it's late for 24.3,
because I don't have time to devote to it right now.


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: lekktu@gmail.com,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Fri, 19 Oct 2012 17:35:02 -0400
> 
> I'm beginning to feel like it's late for 24.3, because I don't have
> time to devote to it right now.

It's too bad to hear this only now, when all the work is already done.

Re: Lisp object that refers to a C struct

[Stefan Monnier]

>> I'm beginning to feel like it's late for 24.3, because I don't have
>> time to devote to it right now.
> It's too bad to hear this only now, when all the work is already done.

It's not wasted, AFAIK.  But if you want, you can install the inotify
patch along with yours (assuming you can check that the inotify patch is
OK).  Otherwise, it'll have to wait for after the freeze, which
shouldn't be that far in the future anyway.


        Stefan

Re: Lisp object that refers to a C struct

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: lekktu@gmail.com,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Fri, 19 Oct 2012 21:52:33 -0400
> 
> >> I'm beginning to feel like it's late for 24.3, because I don't have
> >> time to devote to it right now.
> > It's too bad to hear this only now, when all the work is already done.
> 
> It's not wasted, AFAIK.  But if you want, you can install the inotify
> patch along with yours (assuming you can check that the inotify patch is
> OK).

I cannot test inotify, sorry.