* s390x ffi_closure_helper_SYSV @ 2015-12-17 15:05 Richard Plangger 2015-12-17 15:30 ` Ulrich Weigand 0 siblings, 1 reply; 7+ messages in thread From: Richard Plangger @ 2015-12-17 15:05 UTC (permalink / raw) To: libffi-discuss Hi, I hit an issue with libffi that was included in CPython 2.7.10. Here is the github issue: https://github.com/atgreen/libffi/pull/216 Let me know what you think. Cheers, Richard ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: s390x ffi_closure_helper_SYSV 2015-12-17 15:05 s390x ffi_closure_helper_SYSV Richard Plangger @ 2015-12-17 15:30 ` Ulrich Weigand 2015-12-17 19:32 ` Richard Plangger 2015-12-18 15:09 ` Tom Tromey 0 siblings, 2 replies; 7+ messages in thread From: Ulrich Weigand @ 2015-12-17 15:30 UTC (permalink / raw) To: Richard Plangger; +Cc: libffi-discuss Richard Plangger wrote: > I hit an issue with libffi that was included in CPython 2.7.10. > Here is the github issue: > > https://github.com/atgreen/libffi/pull/216 > > Let me know what you think. I think you're running into the special case with return values that is described here in the libffi docs: In most situations, @samp{libffi} will handle promotion according to the ABI. However, for historical reasons, there is a special case with return values that must be handled by your code. In particular, for integral (not @code{struct}) types that are narrower than the system register size, the return value will be widened by @samp{libffi}. @samp{libffi} provides a type, @code{ffi_arg}, that can be used as the return type. For example, if the CIF was defined with a return type of @code{char}, @samp{libffi} will try to store a full @code{ffi_arg} into the return value. I.e. when a return value is described by any of the small integral type codes, code is expected to actually return a full ffi_arg. While this is not explicitly mentioned in the closure docs, the code makes the assumption that this is handled likewise for closure return values. This is also explicitly checked for in the libffi test suite, e.g. testsuite/libffi.call/cls_sshort.c While this is indeed somewhat surprising, I don't think we can simply change the behavior at this point, as other existing users may depend on current behavior. In any case, this is a cross-platform issue (though probably exacerbated on big-endian platforms). Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: s390x ffi_closure_helper_SYSV 2015-12-17 15:30 ` Ulrich Weigand @ 2015-12-17 19:32 ` Richard Plangger 2015-12-21 18:47 ` Ulrich Weigand 2015-12-18 15:09 ` Tom Tromey 1 sibling, 1 reply; 7+ messages in thread From: Richard Plangger @ 2015-12-17 19:32 UTC (permalink / raw) To: libffi-discuss Hi Ulrich, thx for your reply. I agree that it makes sense to return a full ffi_arg if the integral value is smaller than the machine register. However, I still think there is something not right with the s390 implementation. Please have a look at this program: https://gist.github.com/planrich/3fd72767812754d9104d The program pollutes a fairly large junk of memory below the frame pointer and then calls back to a python function (from c). Because the ret_buffer variable (in ffi_closure_helper_SYSV) is not initialized properly, the returned value is not the same on s390x as it is on e.g. x86. `make` on my laptop (x86) returns without asserting, but it does not on s390x. PPC was recently implemented on PyPy and there we did also not hit this issue. Cheers, Richard On 12/17/2015 04:30 PM, Ulrich Weigand wrote: > Richard Plangger wrote: > >> I hit an issue with libffi that was included in CPython 2.7.10. >> Here is the github issue: >> >> https://github.com/atgreen/libffi/pull/216 >> >> Let me know what you think. > > I think you're running into the special case with return values > that is described here in the libffi docs: > > In most situations, @samp{libffi} will handle promotion according to > the ABI. However, for historical reasons, there is a special case > with return values that must be handled by your code. In particular, > for integral (not @code{struct}) types that are narrower than the > system register size, the return value will be widened by > @samp{libffi}. @samp{libffi} provides a type, @code{ffi_arg}, that > can be used as the return type. For example, if the CIF was defined > with a return type of @code{char}, @samp{libffi} will try to store a > full @code{ffi_arg} into the return value. > > I.e. when a return value is described by any of the small integral > type codes, code is expected to actually return a full ffi_arg. > > While this is not explicitly mentioned in the closure docs, the > code makes the assumption that this is handled likewise for closure > return values. This is also explicitly checked for in the libffi > test suite, e.g. testsuite/libffi.call/cls_sshort.c > > While this is indeed somewhat surprising, I don't think we can > simply change the behavior at this point, as other existing > users may depend on current behavior. > > In any case, this is a cross-platform issue (though probably > exacerbated on big-endian platforms). > > Bye, > Ulrich > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: s390x ffi_closure_helper_SYSV 2015-12-17 19:32 ` Richard Plangger @ 2015-12-21 18:47 ` Ulrich Weigand 2015-12-22 17:03 ` Richard Plangger 2016-01-04 16:54 ` Richard Plangger 0 siblings, 2 replies; 7+ messages in thread From: Ulrich Weigand @ 2015-12-21 18:47 UTC (permalink / raw) To: Richard Plangger; +Cc: libffi-discuss Hi Richard, > thx for your reply. I agree that it makes sense to return a full ffi_arg > if the integral value is smaller than the machine register. It's not a matter of what makes sense, it's a matter of what libffi *expects the user-provided callback to do*. If the *callback* doesn't fill in a full ffi_arg, then libffi will not operate correctly. > The program pollutes a fairly large junk of memory below the frame > pointer and then calls back to a python function (from c). > Because the ret_buffer variable (in ffi_closure_helper_SYSV) is not > initialized properly, the returned value is not the same on s390x as it > is on e.g. x86. The point is that if the user-callback were to fill in a full ffi_arg, then ret_buffer would be completely filled. If ret_buffer isn't fully written, then that's a bug in the callback PyPy provides to libffi. > `make` on my laptop (x86) returns without asserting, but > it does not on s390x. PPC was recently implemented on PyPy and there we > did also not hit this issue. Is this on little-endian or big-endian PowerPC? Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: s390x ffi_closure_helper_SYSV 2015-12-21 18:47 ` Ulrich Weigand @ 2015-12-22 17:03 ` Richard Plangger 2016-01-04 16:54 ` Richard Plangger 1 sibling, 0 replies; 7+ messages in thread From: Richard Plangger @ 2015-12-22 17:03 UTC (permalink / raw) To: libffi-discuss; +Cc: Ulrich Weigand Hi, >> The program pollutes a fairly large junk of memory below the frame >> pointer and then calls back to a python function (from c). >> Because the ret_buffer variable (in ffi_closure_helper_SYSV) is not >> initialized properly, the returned value is not the same on s390x as it >> is on e.g. x86. > > The point is that if the user-callback were to fill in a full ffi_arg, > then ret_buffer would be completely filled. If ret_buffer isn't fully > written, then that's a bug in the callback PyPy provides to libffi. Because the return value (on s390x) contains contents from the stack, it looked malicious to me. >> `make` on my laptop (x86) returns without asserting, but >> it does not on s390x. PPC was recently implemented on PyPy and there we >> did also not hit this issue. > > Is this on little-endian or big-endian PowerPC? PyPy implements both. Cheers, Richard ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: s390x ffi_closure_helper_SYSV 2015-12-21 18:47 ` Ulrich Weigand 2015-12-22 17:03 ` Richard Plangger @ 2016-01-04 16:54 ` Richard Plangger 1 sibling, 0 replies; 7+ messages in thread From: Richard Plangger @ 2016-01-04 16:54 UTC (permalink / raw) To: uweigand; +Cc: libffi-discuss [-- Attachment #1: Type: text/plain, Size: 1854 bytes --] Hi and happy new year, I have further investigated this issue. Sorry to be so persistent, but I still think something is not quite right here. To show my point: here is a comparison between x86_64 (I assume this is most tested platform for libffi) and s390x. In short: on x86_64 the closure helper behaves differently processing the return value. Here are some gdb steps in the x86_64 linux implementation: https://gist.github.com/planrich/fd25c31213ba565116a9 In a nutshell: I broke at the assembly position at https://github.com/python/cpython/blob/master/Modules/_ctypes/libffi/src/x86/unix64.S#L269 of my small ctypes sample program. (https://gist.github.com/planrich/3fd72767812754d9104d) As far as I can tell (on x86_64) ffi_closure_unix64_inner is the equivalent to ffi_closure_helper_SYSV on s390x. If the above is correct then: movzx eax,WORD PTR [rsp-0x18] zero extends the 16 bit value to a full 64bit value. That is what my initial patch is all about, s390x does not do this zero/sign extension just after invoking the user closure. > The point is that if the user-callback were to fill in a full ffi_arg, > then ret_buffer would be completely filled. If ret_buffer isn't fully > written, then that's a bug in the callback PyPy provides to libffi. The closure return value (which is written on the stack location of ret_buffer on s390x) is filled in ctypes here: https://github.com/python/cpython/blob/master/Modules/_ctypes/cfield.c#L551 This would mean that ctypes only writes 16 bits into ret_buffer? I have also debugged it, and it does only store 16 bits. If I'm wrong, could someone please point out the issue with my sample program? Cheers, Richard P.S. I have looked at the PPC asm implementation as well, there it is also zero/sign extended to the machine register size. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: s390x ffi_closure_helper_SYSV 2015-12-17 15:30 ` Ulrich Weigand 2015-12-17 19:32 ` Richard Plangger @ 2015-12-18 15:09 ` Tom Tromey 1 sibling, 0 replies; 7+ messages in thread From: Tom Tromey @ 2015-12-18 15:09 UTC (permalink / raw) To: libffi-discuss >>>>> "Ulrich" == Ulrich Weigand <uweigand-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org> writes: Ulrich> While this is not explicitly mentioned in the closure docs, the Ulrich> code makes the assumption that this is handled likewise for closure Ulrich> return values. This is also explicitly checked for in the libffi Ulrich> test suite, e.g. testsuite/libffi.call/cls_sshort.c Actually the closure docs are actively wrong right now. I didn't realize this same widening behavior applied to closures (in fact I thought I asked about this and got the answer I documented, but I can't find the email, so it was probably just my error). I sent a github PR to correct the closure documentation. Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-04 16:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-17 15:05 s390x ffi_closure_helper_SYSV Richard Plangger 2015-12-17 15:30 ` Ulrich Weigand 2015-12-17 19:32 ` Richard Plangger 2015-12-21 18:47 ` Ulrich Weigand 2015-12-22 17:03 ` Richard Plangger 2016-01-04 16:54 ` Richard Plangger 2015-12-18 15:09 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).