Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept both Cvoid and Ptr{Void} return type in ccall(:memcpy) #31464

Merged
merged 1 commit into from
May 10, 2019
Merged

Conversation

Keno
Copy link
Member

@Keno Keno commented Mar 24, 2019

POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
void). Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073

@vtjnash
Copy link
Member

vtjnash commented Mar 26, 2019

I'm not sure how much we want to commit to guaranteeing that this precise input to ccall is allowed to be incorrect with respect to the C function that is being called. If we keep the code to permit return of void, then we may need to also document that it has this one special case. (although likely on most platforms, return of a pointer vs. return void probably doesn't affect the caller ABI, so it'll likely work even though wrong)

@Keno
Copy link
Member Author

Keno commented Mar 29, 2019

Ok, I don't have any particularly strong preference either way. However, if we fixup the call site and switch this around, we'll probably have to do a different backport, since otherwise we'd be causing performance problems in the backports if anybody relied on the old signature. Up to you.

This was referenced Apr 15, 2019
@KristofferC
Copy link
Member

Bump

@KristofferC KristofferC mentioned this pull request May 9, 2019
58 tasks
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073
@JeffBezanson
Copy link
Member

I think we should merge this. Might as well tolerate as much stuff as possible :)

@Keno Keno merged commit 8d2727b into master May 10, 2019
@KristofferC KristofferC deleted the kf/memcpyrt branch May 10, 2019 13:51
KristofferC pushed a commit that referenced this pull request May 13, 2019
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073

(cherry picked from commit 8d2727b)
@KristofferC KristofferC mentioned this pull request Aug 26, 2019
55 tasks
@KristofferC KristofferC mentioned this pull request Dec 3, 2019
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Illegal instruction with ccall to :memcpy
5 participants