This patch will check if linking against the symbol backtrace from the libc suceeds, and link in execinfo if it does not.
This is needed because musl does not implement nor plan to implement backtrace support.
Differential D2671
Build: include libexecinfo on musl system nephele on Mar 26 2020, 2:59 AM. Authored by
Details This patch will check if linking against the symbol backtrace from the libc suceeds, and link in execinfo if it does not. This is needed because musl does not implement nor plan to implement backtrace support. I build and ran this on alpine linux, with the fmt review applied aswell.
Diff Detail
Event TimelineComment Actions Successful build - Chance fights ever on the side of the prudent. Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1911/display/redirect Comment Actions Hi! Thanks for your contribution. I think this is not a good way to test whether libexecinfo is needed: the test compilation you propose could fail for other reasons, and waiting for the test compilation would slow down the pre-build process. Is it an issue if we link libexecinfo all the time? It would make the fix very simple. If glibc doesn't have libexecinfo, we could test with os.findlib("execinfo") (https://github.com/premake/premake-core/wiki/os.findlib) and link if it is found. Let me know if I am missing something, as I don't know a lot about musl libc caveats. Comment Actions
Atleast for my (by now low-to-medium end) system it's a negligable ammount of time. This is roughly the way configure scripts check for availability of symbols, which is what we should determine before linking it in. (It would be nice if there was a more "proper" way to determine symbol avaiilibility, but this nicely determines whether the compiler is satisfied with their existance anyhow)
The symbols execinfo provides are present in glibc, I suppose that linking against it could be problematic if the application gets linked to a mix of glibc and execinfo. (In any case, that doesn't seem desireable to me) It is true that the compile could fail for other reasons, but realistically the only other problem i see is a broken C compiler, in which case it is irrelevant what this test outputs since the compliation will fail either way. $ time gcc execinfo.c /usr/lib/gcc/x86_64-alpine-linux-musl/9.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: /tmp/ccGgELji.o: in function `main': execinfo.c:(.text+0xf): undefined reference to `backtrace' collect2: error: ld returned 1 exit status 0m00.08s real 0m00.05s user 0m00.02s system $ time gcc execinfo.c -lexecinfo 0m00.12s real 0m00.05s user 0m00.03s system Comment Actions I see. In that case I suppose this is OK (I find it rather ugly but that's personal taste). Ideally we would have a musl detection at the beginning (for instance at line 92 after detecting the compiler and the architecture). Or we could even add a premake option with-musl-libc that has to be passed manually, which could be used for instance by maintainers and users of Alpine. Not really user-friendly but far easier for us. Would it be easy to detect musl? Else at the very least I'd prefer to test the need for execinfo once at the beginning, so that we don't have calls to the compiler in the middle of our (already complex) prebuild logic. Comment Actions
No, it wouldn't, and detecting musl does not seem desireable either since the libc is supposed to be useable anyway.
Why would it matter? it's needed in that conditional, then the result can be discarded again, would storing it improve the state of afairs? Comment Actions Just for code clarity. Speaking of which, it would be good to add a comment in the code explaining the need for this test, as you did here. ? Comment Actions Alright, i can move the test to line 92 cache the result and then retest. (with apropriate comments) Comment Actions Moved the comment, moved the if bsd conditionals to the variable asignment for readability Comment Actions Build failure - The Moirai have given mortals hearts that can endure. Link to build: https://jenkins.wildfiregames.com/job/macos-differential/809/display/redirect Comment Actions Successful build - Chance fights ever on the side of the prudent. Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2219/display/redirect Comment Actions Build failure - The Moirai have given mortals hearts that can endure. Link to build: https://jenkins.wildfiregames.com/job/macos-differential/813/display/redirect Comment Actions Hi nephele, sorry for the delay in reacting. I was away finishing writing my PhD thesis. I am going to commit this fix. Thanks a lot for your work! ?? Comment Actions (Upon committing, I am only adding -o /dev/null to the cc call, in order not to generate an unneeded a.out file) Comment Actions Ah, i did not know about -o, otherwise i might have done that myself. Thanks for reviewing and commiting :) |