wanted: by-value structures in SB-ALIEN

[Nikodemus Siivola]

Passing and returning structures by value is not supported by SB-ALIEN.

As of 07.06.2019, there's a bounty on this feature: https://www.bountysource.com/issues/75202399-wanted-by-value-structures-in-sb-alien

[Ekaterina Vaartis (vaartis)]

There's now a bunty on this: https://www.bountysource.com/issues/75202399-wanted-by-value-structures-in-sb-alien.

[Drew Crampsie (drewc)]

Now? 02 January 2009 is not "now", it's over 10 years ago. Why do you point attention to it now? Has any progress been made over the last 10 years 5 months and 5 days?

If not, does inflation matter? $83.58 would be what my local bank says.

Also, "bunty" is probably not what you meant. :)

[Stas Boukarev (stassats)]

What are you talking about?

[Ekaterina Vaartis (vaartis)]

The bounty has been created just today. I posted this bounty here for anyone who would find this here and would want to claim the bounty.

[Drew Crampsie (drewc)]

That's so very confusing, as the bounty page says :

"wanted: by-value structures in SB-ALIEN
 02 January 2009 Posted by Nikodemus Siivola
Passing and returning structures by value is not supported by SB-ALIEN."

Can you please update it to say that you are offering the money, and this is not 2009 but 2019?

Can you see where people may get confused?

[Ekaterina Vaartis (vaartis)]

Bountysource is a site where you offer money for solving bugs, this would be the only purpose for the bug to be there. The page there reflects the state of the bug on launchpad, i cannot alter neither the bug description, nor the date, as far as i know. The fact remains that the bounty is unclaimed and the feature is not implemented, the date does not mean much in the context, only the fact that it hasn't been implemented for ten years.

[Ekaterina Vaartis (vaartis)]

Actually, i found a way to edit the bug description, so i will add the bounty note to the description.

[Drew Crampsie (drewc)]

Thank you for explaining that to me. Hopefully your editing will make people less confused about who is offering what and when. Many blessings.

[g_o (go0)]

Hey, already started working on this; I got the return value partially working. It seems like a lot of other code on sb-alien is guess-work, should i also guess optimiztions or should i require some flag when compiling?

Thanks in advance

[g_o (go0)]

Update here: I'm almost finished, problem is I need to call a function i made that fixes something on alien-funcall. I made prints but somehow they won't show when i run a test lisp program? i might need some help

[Ekaterina Vaartis (vaartis)]

Did you try printing to a different stream with (print obj :output-stream stream)? Maybe stdout isn't set up there or something. Also, does anything that might show if it works work in there? Something like raising a condition?

[g_o (go0)]

I tried printing to different stream (stderr) to no avail; not sure what you mean by "raising a condition" but i can't think of a simple condition to conclude - i practically have 1 input in my control - the args, tried to do length mismatch but it's caught beforehand; the only hypothesis i have is that it might be optimized by a transform from aliencomp or something.. though i really am not knowledgeable in sbcl's optimizations to be honest.

[g_o (go0)]

so somehow the print shows on some contrib tests ... [visible confusion]

[g_o (go0)]

Update: somehow using defvar to extern-alien fixes the problem. I'll try to get rid of the cause root by the time I send the patch

[g_o (go0)]

Semester started so progress will be a bit slow, though it really is almost done

[g_o (go0)]

so i guess i'll just update that i got back to the issue and it partially works. what i have left is to deal with default alignment (not packing/inner padding), i.e: initial offset of the struct when passed. can anyone give me some pointers as to how to guess that offset on the sbcl side?

[g_o (go0)]

nevermind it seems like the optimization of passing args by registers is a little different for structs; and so i don't think it was really an offset problem. i'll try to fix it this week, hopefully it goes well and i'll finally have a patch

[g_o (go0)]

alright, so it works but now i get back to the problem i discussed with @vaartis where if i use dynamic binding (i.e: defvar/defparameter) for my alien function my changes show up, but if i use lexical (i.e: let) - as if i never changed anything (e.g: if i inserted a print it wouldn't show). changes to aliencomp though still show. i'm still pretty lost here - is this a host/target compilation thing?

[Ekaterina Vaartis (vaartis)]

Could you perhaps upload your version somewhere with a simple test?

[g_o (go0)]

@vaartis i can send you my patch but the problem is that it's incredibly important to fix it because define-alien-routine is using lexical variables so if you want to both pass and return struct you'd have to make an almost clone of it on the user side which is really clunky..

[g_o (go0)]

here's what i have so far.

[g_o (go0)]

these are external tests i have but didn't integrate yet

[g_o (go0)]

anyway what i sent should suffice for pass and return, if you want to do both i can send an example file of how it'd be done under this patch from user standpoint. it's not a good solution though, so i really prefer an explanation as to why alien-funcall problematic for lexical vars and if it's a separate issue - open another bug.

to be clear: it doesn't matter *how* i change alien-funcall as far as i could tell, it just uses whatever i had previously. this last edge case allegedly can be solved simply by changing a let to a defvar in define-alien-routine; but even if we settle on this horrible solution - it crashes compilation itself. i really am not sure what i can do here

[g_o (go0)]

alright i think i fixed most of the problems

[g_o (go0)]

umm @vaartis mind giving some sort of feedback on the latest patch i sent?

[Ekaterina Vaartis (vaartis)]

Sorry for not responding before. I'll try your patch tomorrow with the use case I had in mind initially and get back to you.

[Ekaterina Vaartis (vaartis)]

When building latest SBCL (d0243a9f9961f0afdc09b555821b88edb2488be9 on github) with your patch applied I get the following signal (which pauses compilation):

[314/314] src/code/last-file

; file: /home/vaartis/Code/sbcl/src/code/target-alieneval.lisp
; in: DEFUN SB-ALIEN:ALIEN-FUNCALL
; (T (ERROR "~S is not an alien function." SB-ALIEN:ALIEN))
;
; caught STYLE-WARNING:
; undefined function: COMMON-LISP:T
;
; compilation unit finished
; Undefined function:
; T
; caught 1 STYLE-WARNING condition
; printed 4 notes
While evaluating the form starting at line 23, column 0
  of #P"/home/vaartis/Code/sbcl/make-host-2.lisp":

However, afterwards the rest builds fine.

When trying the code you have and following the way you call the function in the tests, it seems like alien-funcall'ing the same externed function only works once, because afterwards the externed function value is modified to have an additional struct parameter (i'm trying a functon that returns a struct and has no arguments).

The tests do pass, but when I tried writing a function of my own and calling it, the data in the struct was all messed up. Not sure if i'm doing something wrong. The test lisp file is attached to this post. I'll also attach a C file to the following post since it seems like only one attachment is allowed.

[g_o (go0)]

about the compilation pause - i did not experience it so that's weird, i'll see if i can replicate that.
about multiple calls - it should inject arguments to a copy but i probably forgot to change it in the one of the steps, i'll see what's up
about your own test - my first impression is that it's an edge case i didn't notice. i think i know what it is and it shouldn't take too long to fix

stay tuned i guess

[g_o (go0)]

i think this should do the job

[g_o (go0)]

cleaned up a bit and changed something tiny. i think it's done

[Stas Boukarev (stassats)]

(defun alien-void-unparsed-type-p (type)
  (string-equal "(VALUES)" (princ-to-string type)))

doesn't look good...

[Ekaterina Vaartis (vaartis)]

With the latest patch, the first value in the passed truct did work as expected. However, in the test file I attached before there were two values and the second one (y) is now always zero when passed into C. In fact, every second field is equal to zero, and the next field after them has their value. Seems like an issue with offsets, perhaps. When I added a third field, it had the value that the third field was supposed to have. This is all despite unsigned int and unsigned-int (in lisp) both being 32bits. Attached is a modified test lisp file. I will also attach the modifed C test file in the next post.

Another thing i've noticed is when I try to create a boolean field in lisp, i always get

debugger invoked on a TYPE-ERROR in thread
#<THREAD "main thread" RUNNING {1004AE0993}>:
  The value
    T
  is not of type
    CHARACTER
  when binding CHAR

when declaring it

[Ekaterina Vaartis (vaartis)]

> third field was supposed to have
* the second field

[g_o (go0)]

@stassats oops forgot about that "^^, that hack should be easily replaceable though
@vaartis about the boolean - i'm not responsible for the typing system. i think i saw some
         sbcl kludges that said that alien system doesn't really support less than byte types
         (in case you're hoping for bitfield); i'm pretty sure i tested char,
         try using it instead. in any case i think it might be out of the scope of the issue.
         (i made no real distinction between types on my end anyways).
         hmm about 32-bit types on 64 bit machine i'm not sure if there's an elegant way to solve
         this; see, i let sbcl's existing argument packer deal with padding and offsetting.
         i prefer being verbose as a workaround using prefered offsetting/alignment attributes
         on gcc's end or on the lisp alien type end.

         tbh at this point i had my fun with this issue and while it's nice to have i
         don't care all that much about the beer money. so if anyone wanna improve this thing or try
         something different he/she is very welcome, hopefully we'll share both.
         i'm just really out of time for this issue, as another semester is coming my way
         and with things being crazy as they are right now.

[Rongcui Dong (rongcuid)]

I disagree with that view. If refactor breaks things, it means that tests are not good enough. Easing maintenance is a very good benefit: I cite https://www.sbcl.org/history.html for the fact that SBCL is "distinguished from CMUCL by a greater emphasis on maintainability". Of course, doing a refactor like this will require extensive testing and is probably a large task. I hope Github CI would allow such tests to be done.

If you really don't want that, I can skip it, but I personally don't think that's a good idea. The original backend code has a lot of architecture specific code mixed into the common path, and it's quite a lot of work to reverse engineer their mini-DSLs embedded as lists. You can have a look at my draft PR for a comparison. I've only done it for ARM64 and X86 with an exact translation, and I am using the PR only for the CI.

I am going to pause until we reach an agreement of what to do, but the ARM64 patches are complete and can be tested. The patches I sent do not include the refactor.

[Charles (karlosz)]

I agree, it’s absurd to ask for not refactoring anything when it’s directly beneficial for adding a large feature.

The number stack should be used for allocating space for struct return values. You can’t have random numbers on the descriptor stack, especially for precise platforms like arm, where the GC could treat them as Lisp pointers.

It’s okay to use the number stack writing to the stack and incrementing the NSP. The NSP gets reset correctly to the NFP. Just don’t change the NFP and things should work.

[Stas Boukarev (stassats)]

Of course you would disagree, because your refactorings is how I came to that conclusion.

[Christophe Rhodes (csr21-cantab)]

> If refactor breaks things, it means that tests are not good enough.

OK. I'm perfectly happy to go on record to say that our tests are not good enough, in general, but certainly in the specific area of %alien-funcall.

That doesn't mean that refactorings / reworkings / rearrangements / rearchitecting can't happen, but it does raise the burden on the refactorer, and they carry with them an implicit promise to deal with any subsequently-discovered breakage further down the line.

As for how to test: the cfarm compile farm project offers ssh access to hosts on a variety of architectures and operating systems; it should be possible to build and test things there. Github CI is inadequate for this, as it does not cover most supported architectures or most supported operating systems.

I'd strongly advise first auditing the existing tests (and possibly the implicit tests from e.g. loading and testing systems in quicklisp) to get a sense for what documented functionality is tested at all, what is not, and how big a job it is to write tests to cover a reasonable amount of the difference.

[Charles (karlosz)]

Re: testing and CI: there's a cross-build runner you (Rongcui) can use which tests if you break building for other architectures (the parts which only exercise the cross-compiler). You can also push to a local fork if you want github CI test runners to work. Unfortunately, this doesn't actually run the tests on architecures beside x86-64 and arm64. You would have to do that yourself. I can recommend requesting access for the gcc farm machines which are available to free software porters if you want machines; they have machines for every platform available. It's annoying for sure, but I think that's the best you can do to make sure you don't break code on other machines. Of course, the less tested platforms naturally bitrot anyway.

Stas, I appreciate your judgements on my own work, but I don't know how valuable that is to the general discussion. To say "don't refactor" to someone who's clearly spent a lot of work in the code and gotten a big and involved feature working on one platform who has insights on what exactly is making it difficult to write the code to port to other architectures doesn't make much sense in this case. I don't know if I could convince you, maybe this needs to come from someone else.

[Rongcui Dong (rongcuid)]

Would you mind elaborating on my refactors? I am open to API change if you think it's not a good encapsulation. The PR is a draft PR, and is meant to break. I have only two archs done so far, and I am merely using the CI for machines I don't have.

[Stas Boukarev (stassats)]

It's not a judgement. Writing code without bugs is nigh impossible. So refactoring introduces bugs but provides no benefits of faster/smaller code.

[Stas Boukarev (stassats)]

>Would you mind elaborating on my refactors?

Why was multiple values changed to a list?

>and I am merely using the CI for machines I don't have.

The CI can be run in a personal fork, which I do all the time. Pull requests seem to eat up Cirrus CI compute credits. I'll have to investigate how to disable that.

[Rongcui Dong (rongcuid)]

I am not going to touch anything outside ALIEN-FUNCALL, so hopefully I don't need to audit everything else... Also, I will make test cases for anything I add as thoroughly as possible.

If I were to continue, I will still start with Github CI, as it's the simplest thing to use. I'll take the advice of machine farms.

[Rongcui Dong (rongcuid)]

> Why was multiple values changed to a list?

This is because every architecture has a different set of requirements. For instance, only ARM64 currently has a preprocessing step. Only X86 needs to set/reset FPU state. I am using destructuring-bind with keywords to make intention clear. Changing back to multiple-values-bind will not affect functionality, but will mean that someone needs to keep track of a large number of positional arguments.

I have no problem with using values. I start with readable code when refactoring, and should any performance issue arise, I can optimize.

> Pull requests seem to eat up Cirrus CI compute credits.

I see. I can move to personal fork for testing.

[Stas Boukarev (stassats)]

Multiple values can be used with keyword arguments too.

[Rongcui Dong (rongcuid)]

> Multiple values can be used with keyword arguments too.

Can they? I don't see it mentioned in CLHS

[Stas Boukarev (stassats)]

(multiple-value-call (lambda (&key a) a) (values :a 10))

[Rongcui Dong (rongcuid)]

> (multiple-value-call (lambda (&key a) a) (values :a 10))

Ok, I see. That's an easy enough change.

So, do I continue with the refactor?

[Rongcui Dong (rongcuid)]

@karlosz

> The number stack should be used for allocating space for struct return values

Does this actually work? I tried to push the return value by pushing to number stack and decrementing NSP in ALIEN-FUNCALL. It works when I have a C function of form `struct foo return_foo(void);`, but whenever I add an argument SBCL will crash due to memory fault. The PC pointer looks nothing like the disassembly suggests, so I assume that the crash happened after the alien function returns.

Basically, is this correct?

* Lisp function
* Calls alien routine
** Call C function
** Push result to number stack
* ???

That is, if I push the number stack _inside_ the alien function, is that still valid after the function returns?

[Charles (karlosz)]

No, I don't think allocating space on the number stack only inside ALIEN-FUNCALL can work. There needs to be coordination with the caller so that the values can be copied before return.

I think you have to have the caller allocate the space for the return-type as well, which unfortunately means you have to restrict alien-funcall for struct return types to only work when the alien function type is known. This is OK because the majority of alien-funcall uses fall under this case. (For the case where you don't know the type in advance, alien-funcall checks the return type and if it's a struct, it barfs).

Look at the transforms for alien-funcall in aliencomp.lisp. You can analyze the struct return type there and allocate the proper amount of space on the number stack there, which is going to be the real backing sap. The sap to that space can then be passed for alien-funcall to copy the struct it gets from C into the backing storage.

Does that plan make sense?

[Yukari Hafner (shinmera)]

Hey, really excited to finally see this make some headway!

I wanted to talk about an additional use-case that I need, and which not even cffi-libffi can help me with: structs-by-value in callback arguments and returns.

I've so far gotten by without needing this, but I recently started on the Framebuffers [1] project, which I would like to have a backend to Apple's Cocoa library. Apple looooves callbacks, so you have to implement a bunch of those to change and implement certain behaviours. Some of those callbacks look like this:

    struct rect update_rect(struct rect input)

Which in CFFI would be defined something like this:

    (cffi:defcallback update-rect (:struct rect) ((input (:struct rect))) ...)

So I need to be able to return a struct to C somehow.

For the struct-returns of a call, CFFI usually gives you a "translated" version of the struct in the form of a plist. That way lifetime questions are avoided, since it's just a regular lisp object. I'm not sure if sb-alien should take the same approach though.

[1] https://shirakumo.github.io/framebuffers/

[Rongcui Dong (rongcuid)]

There are two transforms:

deftransform alien-funcall ((function &rest args)
                             ((alien (* t)) &rest t) *)

deftransform alien-funcall ((function &rest args) * * :node node)

Which one should I use?

[Charles (karlosz)]

You should use the second one, that tells what the function type is at compile time so you can decide how much space to allocate on the number stack at compile time in the caller. This space can then be passed via a SAP to %alien-funcall which does the copying into that region when it gets the returned struct from C. This is how you extend the lifetime of the stack memory to the duration of the caller frame.

[Rongcui Dong (rongcuid)]

Also, the crash was caused by misaligned stack. My original implementation which pushed NSP from %ALIEN-FUNCALL "worked", but is that just accidental?

[Charles (karlosz)]

I'd have to check again whether on arm64 (or the other platforms where the Lisp stack is not the number stack), function calls tear down the number stack frame. It might be the case that on arm64, its OK. But on x86-64, the C stack is the Lisp stack, so when %ALIEN-FUNCALL returns that memory definitely no longer protected by the stack pointer, so it would be dangerous.

[Rongcui Dong (rongcuid)]

I see. I am still reading the transform function and trying to figure out how to reserve stack space there. Do you have some quick pointers/examples for doing so?

For callbacks, I may look into them later. I would guess that they are a bit easier, but I'll of course need to figure out how to copy struct data from C to Lisp.

[Charles (karlosz)]

Look at the %alien-funcall ir2-convert method to get a feeling of how the number stack is manipulated by %alien-funcall itself. You would have to lift up some machinery from there to get it to work in the caller. You can see that the number stack space is reset there once %alien-funcall returns, so probably how you have it working is only accidental.

For an initial prototype, you can reserve the space in the caller with (%primitive alloc-alien-stack-space ...). Look at the transform for MAKE-LOCAL-ALIEN. Hope this helps.

[Charles (karlosz)]

MAKE-LOCAL-ALIEN is how WITH-ALIEN works, by the way. (which reserves number stack space in the current frame)

[Rongcui Dong (rongcuid)]

> You can see that the number stack space is reset there once %alien-funcall returns, so probably how you have it working is only accidental.

Actually, I am aware that %alien-funcall resets NSP (if you are referring to dealloc-number-stack-space). Currently my structs are pushed to NSP _after_ this, so the decremented NSP stays to the end of %alien-funcall. You can have a look here: https://github.com/rongcuid/sbcl/blob/0fa3218340820ecffe9786cd1bfc5ca22caf3dca/src/compiler/aliencomp.lisp#L756. It's a funcall, but the lambda being called decrements NSP and copies the struct.

I will see what make-local-alien does.

[Rongcui Dong (rongcuid)]

What's the difference between SB-C::VOP and SB-C::%PRIMITIVE? I was doing exactly what make-local-alien does, except with VOP. I also see a FIXME saying that %PRIMITIVE means the same thing as VOP?

[Charles (karlosz)]

I think what you're doing at that line looks fine then, actually. As long as the NSP protects it, the memory is valid. It will get cleaned up when the Lisp function returns (see e.g. compiler/arm64/call.lisp, where the nsp is cleaned up in the call VOPs). So the user just has to make sure to use or copy the alien struct before the Lisp frame returns.

[Charles (karlosz)]

VOP and %PRIMITIVE work at different levels. %PRIMITIVE is for normal CL code that is written, while VOP is used as part of codegen methods (hence it takes IR2 structures as part of its arguments, which %PRIMITIVE doesn't do; those are supplied by the codegen phase of the compiler)

[Rongcui Dong (rongcuid)]

> It will get cleaned up when the Lisp function returns (see e.g. compiler/arm64/call.lisp, where the nsp is cleaned up in the call VOPs).

That's the part I am not 100% certain about: does this happen at the end of an alien call as well? Or is %alien-funcall all there is? Or, are alien functions wrapped inside a lisp function call which I am not seeing in %alien-funcall routine?

[Charles (karlosz)]

The primitive %ALIEN-FUNCALL is not wrapped in its own Lisp function when the function type is known; this is what the transform does (it changes a call to ALIEN-FUNCALL directly into that sequence of steps in the caller via %ALIEN-FUNCALL, which is compiled as a primitive, not as a function call). However, when the function type is not known, the primitive is wrapped by the Lisp function ALIEN-FUNCALL. That's why I mentioned that it's probably only possible to do struct returns when the function type is known to the compiler.

[Rongcui Dong (rongcuid)]

I see. But doesn't the compiler always know the function types? DEFINE-ALIEN-ROUTINE has to provide all type information.

[Charles (karlosz)]

No. You could do something like (defun f (x) (alien-funcall x ...)). So yes, the compiler knows what the types are for a given alien function, but it doesn't necessarily know what alien function is being alien-funcalled. So if you did (f <alien-function-that-returns-a-struct>), it couldn't really work because F doesn't know how much space to allocate for the struct that's returned. But if you had

(defun bar (x)
  (alien-funcall (extern-alien "returns_struct" ...) x))

that gets rewritten by the compiler since it knows the alien function being invoked and hence th return type (and so it can rewrite alien-funcall to the stuff in the transform with %alien-funcall; now it works to allocate space in the caller). Does that make sense?

[Rongcui Dong (rongcuid)]

I see. Thank you very much.

[Rongcui Dong (rongcuid)]

This patch adds the ability to return structure by value on ARM64.

[Rongcui Dong (rongcuid)]

Updated doc strings for return by value

[Charles (karlosz)]

Could you update the manual as well (if you haven't already?) It still says struct pass by value is unsupported.

[Rongcui Dong (rongcuid)]

Maybe more can be explained, but I'll just add a few sentence.

[Rongcui Dong (rongcuid)]

Summer semester starts so my work on this will be slow. My next target is alien callbacks on ARM64 to complete full support for ARM64. Then, I will move on to X86-64.

[Rongcui Dong (rongcuid)]

I no longer have time to work on this issue, so I will stop development for now. I will still provide any help if existing patches need to be merged.