Musl libc compatibility #4385
Closed
Musl libc compatibility #4385
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| dnl | ||
|
|
||
| AC_DEFUN([ZFS_AC_CONFIG_USER_TIRPC], [ | ||
| AH_TEMPLATE([WITH_TIRPC], |
There was a problem hiding this comment.
AH_TEMPLATE does not appear to be used anywhere in the existing ZoL autotools code. This autotools code is failing the buildbot, so this probably should be made consistent with the rest of it to try to fix that.
|
@clandmeter thanks for refreshing these patches. In order to get them merged you're going to need to address the various issues identified by the buildbot. It looks like the biggest issue comes from 936cd76 so it may make sense to split that patch in to its own PR. That would make it easier for us to review, test and merge the other changes. That way we can start whittling down this patch stack. |
| if (!xdr_control(xdr, XDR_GET_BYTES_AVAIL, &bytesrec)) | ||
| return (EFAULT); | ||
| #else | ||
| xdr_control(xdr, XDR_GET_BYTES_AVAIL, &bytesrec); |
There was a problem hiding this comment.
libtirpc's implementation of xdr_control appears to be a no-op because it checks for a function pointer that is always NULL. That means that bytesrec is uninitialized before it is accessed by NVS_XDR_MAX_LEN.
The XDR struct appears to be standardized, so likely could undefine the libtirpc xdr_control in favor of our current xdr_control code. We could do that by putting this into our rpc/xdr.h:
#include_next <rpc/xdr.h>
/* libtirpc xdr_control is unimplemented as of 0.2.5. */
#ifdef WITH_TIRPC
#ifdef xdr_control
#undef xdr_control
#endif
#endif
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 15, 2016
I noticed during code review of openzfs#4385 that the author had peppered the various Makefile.am files with $(TIRPC_LIBS) when putting it into lib/libspl/Makefile.am would have sufficed. Upon further examination, it seems that he had copied what we do with $(ZLIB), which unfortunately is wrong because it is only needed in the Makefile.am corresponding to libzpool. Autoconf is designed to propagate link dependencies to the binary into which the static object is incorporated and the ELF interpreter will load it for that binary without a problem. If we specify it multiple times, we end up with the situation where unless `LDFLAGS` contains `-Wl,--as-needed`, the ELF interpreter is told to load the same library multiple times, which is serves to load down ELF intialization at best and will lead to unnecessary libraries being loaded at worst. The worst case scenario does not happen with zlib because even with -Wl,--as-needed, it is loaded anyway for libuutil. However, we should clean this up to provide a better example of how the code should be written to future contributors. Otherwise, they will be inclined to make the same mistake (for consistency) and we will end up loading libraries that we do not need. Signed-off-by: Richard Yao <[email protected]>
|
You should try to preserve authorship on commits. |
|
@behlendorf @clandmeter I took the liberty of rewriting the libtirpc support patch. I opened #4422 for it to run it past the buildbot, but someone will need to test it against libtirpc on both glibc and musl libc. |
|
@behlendorf @clandmeter The failures involving pkgconfig appear to occur when pkg-config is missing. These guys had the same problem: |
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 16, 2016
31fc193 removed multiple superfluous DT_NEEDED entries, but left zlib alone. At the time, I only paid attention to libraries that were absent from DT_NEEDED when -Wl,--as-needed was passed and zlib was not one of them (as libuutil linked to it). I noticed during code review of openzfs#4385 that the author had peppered the various Makefile.am files with $(TIRPC_LIBS) when putting it into lib/libspl/Makefile.am would have sufficed. Upon further examination, it seems that he had copied what we do with $(ZLIB). Autoconf is designed to propagate link dependencies to the binary into which the static object is incorporated and the ELF interpreter will load it for that binary without a problem. If we specify it multiple times, we end up with the situation where unless `LDFLAGS` contains `-Wl,--as-needed`, the ELF interpreter is told to load the same library multiple times, which is serves to load down ELF intialization at best and will lead to unnecessary libraries being loaded at worst. We should clean this up to provide a better example of how the code should be written to future contributors. Otherwise, they will be inclined to make the same mistake (for consistency) and we will end up loading libraries that we do not need. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 16, 2016
31fc193 removed multiple superfluous DT_NEEDED entries, but left zlib alone. At the time, I only paid attention to libraries that were absent from `DT_NEEDED` when `-Wl,--as-needed` was passed to CPPFLAGS and zlib was not one of them (as libuutil linked to it). A `DT_NEEDED` entry on zlib from libuutil makes absolutely no sense, but I had assumed that the toolchain would be smart enough to catch all cases. That turned out to be untrue (maybe because `USER_ASM` is used?), so I missed zlib. I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into lib/libspl/Makefile.am would have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. Autoconf is designed to propagate link dependencies to the binary into which the static object is incorporated and the ELF interpreter will load it for that binary without a problem. If we specify it multiple times, we end up with the situation where unless `LDFLAGS` contains `-Wl,--as-needed`, the ELF interpreter is told to load the same library multiple times, which is serves to load down ELF intialization at best and will lead to unnecessary libraries being loaded at worst. We should clean this up to provide a better example of how the code should be written to future contributors. Otherwise, they will be inclined to make the same mistake (for consistency) and we will end up loading libraries that we do not need. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 16, 2016
31fc193 removed multiple superfluous DT_NEEDED entries, but left zlib alone. At the time, I only paid attention to libraries that were absent from `DT_NEEDED` when `-Wl,--as-needed` was passed to CPPFLAGS and zlib was not one of them (as libuutil linked to it). A `DT_NEEDED` entry on zlib from libuutil makes absolutely no sense, but I had assumed that the toolchain would be smart enough to catch all cases. That turned out to be untrue (maybe because `USER_ASM` is used?), so I missed zlib. I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into lib/libspl/Makefile.am would have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. Autoconf is designed to propagate link dependencies to the binary into which the static object is incorporated and the ELF interpreter will load it for that binary without a problem. If we specify it multiple times, we end up with the situation where unless `LDFLAGS` contains `-Wl,--as-needed`, the ELF interpreter is told to load the same library multiple times, which is serves to load down ELF intialization at best and will lead to unnecessary libraries being loaded at worst. We should clean this up to provide a better example of how the code should be written to future contributors. Otherwise, they will be inclined to make the same mistake (for consistency) and we will end up loading libraries that we do not need. There might be a bit more to do here with regard to -lrt and friends. That would require examining the interactions between the code and librt et al, which can be left for a later date. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 16, 2016
31fc193 removed multiple superfluous DT_NEEDED entries, but left zlib alone. At the time, I only paid attention to libraries that were absent from `DT_NEEDED` when `-Wl,--as-needed` was passed to `LDFLAGS` and zlib was not one of them (as libuutil linked to it). A `DT_NEEDED` entry on zlib from libuutil makes absolutely no sense, but I had assumed that the toolchain would be smart enough to catch all cases. That turned out to be untrue (maybe because `USER_ASM` is used?), so I missed zlib. I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into lib/libspl/Makefile.am would have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. Autoconf is designed to propagate link dependencies to the binary into which the static object is incorporated and the ELF interpreter will load it for that binary without a problem. If we specify it multiple times, we end up with the situation where unless `LDFLAGS` contains `-Wl,--as-needed`, the ELF interpreter is told to load the same library multiple times, which is serves to load down ELF intialization at best and will lead to unnecessary libraries being loaded at worst. We should clean this up to provide a better example of how the code should be written to future contributors. Otherwise, they will be inclined to make the same mistake (for consistency) and we will end up loading libraries that we do not need. There might be a bit more to do here with regard to -lrt and friends. That would require examining the interactions between the code and librt et al, which can be left for a later date. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into lib/libspl/Makefile.am would have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. There is a bug in libtool where it uses static link dependencies for dynamic linking, which causes overlinking. There is another bug in the toolchain where passing `-Wl,--as-needed` through `LDFLAGS` causes it to be ignored on libraries due to ld only honoring -Wl,--as-needed on libraries specified after it and libtool emitting the libraries before LDFLAGS: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the `-Wl,--as-needed` not being honored on libraries bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation, but it still nicely cleans up the code and should encourage good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. Unfortunately, what we do is wrong, so lets clean it up so that we encourage future contributors to write patches that specify the dependencies only on the things pulling them into the dependency graph. In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug is in libtool, where it overlinks by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. Unfortunately, what we do is wrong, so lets clean it up so that we encourage future contributors to write patches that specify the dependencies only on the things pulling them into the dependency graph. In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug is in libtool, where it overlinks by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Additionally, we have multiple `-lz` and `-lblkid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html Signed-off-by: Richard Yao <[email protected]>
Closed
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug is in libtool, where it overlinks by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Additionally, we have multiple `-lz` and `-lblkid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. We also have a bit of that with `-ldl` and `-lm` too. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In addition, we have multiple `-lz` and `-lblkid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug is in libtool, where it overlinks by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. We also have a bit of that with `-ldl` and `-lm` too. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In addition, we have multiple `-lz` and `-lblkid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug causes libtool to overlink by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging#libtool_issues The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. We also have a bit of that with `-ldl` and `-lm` too. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In addition, we have multiple `-lz` and `-lblkid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug causes libtool to overlink by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging#libtool_issues The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. We also have a bit of that with `-ldl` too. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In addition, we have multiple `-lz` and `-lblkid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug causes libtool to overlink by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging#libtool_issues The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
ryao
added a commit
to ryao/zfs
that referenced
this pull request
Mar 17, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. We also have a bit of that with `-ldl` too. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In addition, we have multiple `-lz` and `-luuid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html In an ideal world, this would translate into improvements in ELF's DT_NEEDED entries, but that is not the case because of a couple of bugs in libtool. The first bug causes libtool to overlink by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging#libtool_issues The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of DT_NEEDED entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]>
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this pull request
Mar 18, 2016
I noticed during code review of openzfs#4385 that the author of a commit had peppered the various Makefile.am files with `$(TIRPC_LIBS)` when putting it into `lib/libspl/Makefile.am` should have sufficed. Upon further examination, it seems that he had copied what we do with `$(ZLIB)`. We also have a bit of that with `-ldl` too. Unfortunately, what we do is wrong, so lets fix it to set a good example for future contributors. In addition, we have multiple `-lz` and `-luuid` passed to the compiler because each `AC_CHECK_LIB` adds it to `$LIBS`. That is somewhat annoying to see, so we switch to `AC_SEARCH_LIBS` to avoid it. This is consistent with the recommendation to use `AC_SEARCH_LIBS` over `AC_CHECK_LIB` by autotools upstream: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Libraries.html In an ideal world, this would translate into improvements in ELF's `DT_NEEDED` entries, but that is not the case because of a couple of bugs in libtool. The first bug causes libtool to overlink by using static link dependencies for dynamic linking: https://wiki.mageia.org/en/Overlinking_issues_in_packaging#libtool_issues The workaround for this should be to pass `-Wl,--as-needed` in `LDFLAGS`. That leads us to the second bug, where libtool passes `LDFLAGS` after the libraries are specified and `ld` will only honor `--as-needed` on libraries specified before it: https://sigquit.wordpress.com/2011/02/16/why-asneeded-doesnt-work-as-expected-for-your-libraries-on-your-autotools-project/ There are a few possible workarounds for the second bug. One is to either patch the compiler spec file to specify `-Wl,--as-needed` or pass `-Wl,--as-needed` via `CC` like `CC='gcc -Wl,--as-needed'` so that it is specified early. Another is to patch ltmain.sh like Gentoo does: https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/ELT-patches/as-needed Without one of those workarounds, this cleanup provides no benefit in terms of `DT_NEEDED` entry generation. It should still be an improvement because it nicely simplifies the code while encouraging good habits when patching autotools scripts. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#4426
required by O_RDONLY, O_RDWR and O_CREAT
| @@ -123,7 +123,7 @@ | |||
| #include <math.h> | |||
| #include <sys/fs/zfs.h> | |||
| #include <libnvpair.h> | |||
| #ifdef __GNUC__ | |||
| #ifdef __GLIBC__ | |||
There was a problem hiding this comment.
Seems reasonable. But can you add a link to the commit comment explaining why GLIBC should be used instead of GNUC. My guess is that GNUC was used here since that's what recommenced fron by gcc documentation.
There was a problem hiding this comment.
execinfo.h and backtrace() are GNU extensions provided by glibc and not by gcc so the change is correct.
The mentioned gcc documentation does not mention backtrace() or execinfo.h as far I can see.
An alternative way to solve it would be to do a configure check for it and use HAVE_EXECINFO_H and HAVE_BACKTRACE.
execinfo.h and backtrace() are GNU extensions provided by glibc and not by gcc http://www.gnu.org/software/libc/manual/html_mono/libc.html#Backtraces
When compiling with musl libc the return type will be incorrect.
This is needed for musl libc
| typedef long long hrtime_t; | ||
| typedef struct timespec timestruc_t; | ||
| typedef struct timespec timespec_t; | ||
|
|
There was a problem hiding this comment.
@behlendorf ive moved the typedefs as you suggested in previous commit, and i have changed longlong_t to long long as longlong_t is not defined by any standard so musl does not support it.
|
Seems comments are removed when they are commented on specific commits. @behlendorf should i include commit behlendorf@468e0c5 to this PR? |
…g long hrtime_t timestruc_t and timespec_t should have originally be included in sys/time.h so lets move them. longlong_t is not defined by any standard so change it to long long
|
@clandmeter I've opened PR #4449 with the proposed assert/verify cleanup. Let's leave it separate for now but if you could please review and test it for Musc that would be appreciated. Let's also try and cherry pick a few of the non-controversial patches here in to master to reduce the patch stack and see what we're left with. |
This was referenced Mar 25, 2016
|
Closing, in favor of other open PRs. @clandmeter which changes are still required for musl libc compatibility? Just #4449 and some version of #4422? |
|
Yes those should do it. |
|
@clandmeter if you could review and test those proposed changes on a musc libc system we should be able to get them in. |
|
I already verified #4449 on musl. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a followup on #2604 (many thanks to @stef). It has some changes which were added to our alpinelinux tree. The main difference is the removal of --enable-musl as discussed before and is now replaced by --with-tirpc (done by @ncopa). If needed I can split some of these into separate PR's.