Alternative approach to using strerror_r for reentrant error string conversion#11010
Alternative approach to using strerror_r for reentrant error string conversion#11010xavierleroy merged 3 commits intoocaml:trunkfrom
Conversation
runtime/sys.c
Outdated
| #elif defined(_GNU_SOURCE) | ||
| return strerror_r(errnum, buf, buflen); | ||
| #else |
There was a problem hiding this comment.
I'm not sure this special case is necessary: _GNU_SOURCE is not defined in this file, and the fallback case below would result in a type error if the GNU version of strerror_r as , and if it were the C compiler would error on the use of strerror_r with the wrong type.
But I don't mind keeping the special case, and I checked that the function compiles and runs correctly whether _GNU_SOURCE is defined or not.
There was a problem hiding this comment.
Don't you want to define _GNU_SOURCE in order to get the GNU implementation, though? That would avoid a data copy for each caml_strerror call, since GNU strerror_r usually just returns an immutable pointer to its static array of error messages (whereas the XSI version is forced to copy into the destination buffer). That's why I stuck it in a separate file in the other PR, for header hygiene.
There was a problem hiding this comment.
Ah, now I understand the motivation for the special case: it's not "do the right thing if _GNU_SOURCE happens to be defined", it's "use the GNU version for performance". Sorry for having missed that. I need to think about it some more, but globally I'm not too concerned about the extra copy done by the POSIX strerror_r.
There was a problem hiding this comment.
I went out on a limb and removed support for the GNU version of strerror_r. It feels safer to rely exclusively on the POSIX function. Nothing is performance-critical here. Let me know what you think.
|
This looks good! I used TLS in my PR since I wasn't sure about using so much stack space for some of the invocations in the error path. If that's acceptable, then this is definitely a better approach. I'll try this PR on OpenBSD/FreeBSD later on. |
|
I think the extra stack space usage is reasonable, esp. on OCaml 5 where the C stacks are not used for OCaml stack frames and therefore have a much lower chance of overflowing. Maybe |
Done in 127fbe6 . |
This uses the POSIX strerror_r function on *nix, and strerror on Win32 (which is thread-safe). Co-authored-by: Anil Madhavapeddy <anil@recoil.org> squashme
It's better to do it in a separate "cold" function, to reduce code size and stack usage for the platform functions.
127fbe6 to
a7fd49e
Compare
avsm
left a comment
There was a problem hiding this comment.
I hope to tempt you with the more efficient version for the common case (Linux/glibc/gnu-strerror) in avsm@d35845c which adds onto this branch, but the existing version is fine by me. Tested on FreeBSD/OpenBSD.
|
After much soul-searching, I'd rather stay with a pure POSIX solution. Thanks for your patience! |
This is a variant of #10969 that does not use thread-local storage. Instead,
caml_strerroris given the same interface as the GNU nonstandardstrerror_rfunction: it takes a buffer as argument, which it may use or not, and returns achar *pointing to the error message, which can be in the buffer or not.