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

Build fails with musl because of wrong strerror_r() variant #1047

Closed
joshuakraemer opened this issue Jun 2, 2021 · 5 comments · Fixed by #1049
Closed

Build fails with musl because of wrong strerror_r() variant #1047

joshuakraemer opened this issue Jun 2, 2021 · 5 comments · Fixed by #1049
Labels
bug Something isn't working

Comments

@joshuakraemer
Copy link
Contributor

The function strerror_r() exists in a POSIX and a GNU variant. In https://github.com/dosbox-staging/dosbox-staging/blob/master/src/misc/support.cpp#L316, the GNU variant is chosen if _GNU_SOURCE is defined. However, the musl library only provides the POSIX variant, even if _GNU_SOURCE is defined. Thus, the wrong variant is chosen with musl, making the build fail:

../src/misc/support.cpp:327:19: error: could not convert 'strerror_r(err, ((char*)(& buf)), (static_if_array_then_zero<char [128]>() + (sizeof (buf) / sizeof (buf[0]))))' from 'int' to 'std::string' {aka 'std::__cxx11::basic_string<char>'}

As strerror_r() is deprecated anyway, I suggest to change to strerror_l(). See https://www.austingroupbugs.net/view.php?id=655:

Applications should use strerror_l() rather than strerror() or strerror_r() to avoid thread safety and possible alternative (non-conforming) versions of these functions in some implementations.

Alternatively, C++ function overloading could be used instead of the macro test, see e.g. https://github.com/chriskohlhoff/asio/blob/master/asio/include/asio/impl/error_code.ipp#L189

Environment (please complete the following information):
Void Linux, x86_64, musl-1.1.24

Version of dosbox-staging:
649a428

@joshuakraemer joshuakraemer added the bug Something isn't working label Jun 2, 2021
@joshuakraemer joshuakraemer changed the title Build failure with musl because of wrong strerror_r() variant Build fails with musl because of wrong strerror_r() variant Jun 2, 2021
@kcgen
Copy link
Member

kcgen commented Jun 2, 2021

Thanks @joshuakraemer ,
If you try either of the recommendations in the source, does it build and run?
You can also submit either of those changes as a PR here, so we can see if it builds under all supported platforms.

@ericonr
Copy link

ericonr commented Jun 2, 2021

_GNU_SOURCE is defined by GCC by default when compiling C++ code, so the current version will always pick the GNU strerror_r variant when using GCC.

@ericonr
Copy link

ericonr commented Jun 2, 2021

Using function overloading is probably more portable than switching to strerror_l, though I haven't actually checked.

@kcgen
Copy link
Member

kcgen commented Jun 2, 2021

Musl's documentation (https://www.musl-libc.org/doc/1.1.24/manual.html) says:


_GNU_SOURCE (or _ALL_SOURCE)

Adds everything above, plus interfaces modeled after GNU libc extensions and interfaces for making use of Linux-specific features.


From reading that, it looks like you've found a gap between what Musl says it provides versus reality: they are not providing GNU's libc extension of strerror_r. So it seems like we're trying to work around a Musl bug.

@joshuakraemer
Copy link
Contributor Author

The issue was discussed on the musl mailing list in 2013, and it seems that it was not considered to be a bug. This is what one of the musl developers said about it (https://www.openwall.com/lists/musl/2013/02/08/5):

musl provides the posix api when requested

musl provides many gnu specific apis when _GNU_SOURCE is set

but when posix and gnu collides it's always the posix api,
musl never provides broken gnu apis

at least this was the policy so far

In #1048, I have implemented an overloaded helper function to handle the different strerror_r() variants. This is probably the most portable solution (I think strerror_l() is missing in macOS).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants