Page MenuHomeWildfire Games

Build: include libexecinfo on musl system
ClosedPublic

Authored by nephele on Mar 26 2020, 2:59 AM.

Details

Summary

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.

Test Plan

I build and ran this on alpine linux, with the fmt review applied aswell.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nephele created this revision.Mar 26 2020, 2:59 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1911/display/redirect

Stan added a reviewer: Itms.Mar 28 2020, 1:38 PM
Stan added a subscriber: Stan.

Itms said this was not the correct way to do, it I'll let him explain why.

Itms requested changes to this revision.Mar 28 2020, 7:09 PM

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.

This revision now requires changes to proceed.Mar 28 2020, 7:09 PM

and waiting for the test compilation would slow down the pre-build process.

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)

Is it an issue if we link libexecinfo all the time?

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
Itms added a comment.Mar 28 2020, 7:34 PM

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.

Would it be easy to detect musl?

No, it wouldn't, and detecting musl does not seem desireable either since the libc is supposed to be useable anyway.
We basically only need to figure out if the symbol is available in the system libc or not, which this test does.

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.

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?
In any case i could do this sure, if it is required.

Itms added a comment.Mar 28 2020, 7:42 PM

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.

Why would it matter?

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. 🙂

Alright, i can move the test to line 92 cache the result and then retest. (with apropriate comments)

nephele updated this revision to Diff 12002.May 23 2020, 8:03 PM
nephele edited the test plan for this revision. (Show Details)

Moved the comment, moved the if bsd conditionals to the variable asignment for readability

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/809/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2219/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/813/display/redirect

Itms accepted this revision.Tue, Jun 23, 4:05 PM

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! 👍🏻

This revision is now accepted and ready to land.Tue, Jun 23, 4:05 PM
Itms added a comment.Tue, Jun 23, 5:00 PM

(Upon committing, I am only adding -o /dev/null to the cc call, in order not to generate an unneeded a.out file)

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Tue, Jun 23, 5:07 PM

Ah, i did not know about -o, otherwise i might have done that myself.

Thanks for reviewing and commiting :)