update vendored copy of libbacktrace #59
Conversation
motivation: catch up to fixes and improvments in underlying library changes: * vendor latest version of libbacktrace * update vendoring script to exclude some new files that are not needed
tomerd
added
the
patch-version-bump-only
ianpartridge
left a comment
ianpartridge
left a comment
There was a problem hiding this comment.
LGTM.
Long time no speak @tomerd ! Hope you are doing well.
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
Haven't reviewed all the code but updating this sounds like a good idea!
| break; | ||
| case 1: | ||
| filename = "/proc/self/exe"; | ||
| filename = getexecname (); |
There was a problem hiding this comment.
It might be worth pinging the original author @ianlancetaylor. @ianlancetaylor if you have time, I'd be much appreciated if you'd let us know what you think about the changes (see explanation on the PR) in #36.
There was a problem hiding this comment.
Thanks for the ping. I'm puzzled by #36. That pull request says it is for Linux, but on Linux getexecname is not in libc as far as I know. The only system I know with getexecname is Solaris. And the libbacktrace configure script will only arrange to call getexecname on Solaris. In all other cases getexecname is #define'd to return NULL, so we will move on to the next case. Can you provide any more details on the failing case? Thanks.
There was a problem hiding this comment.
@ianlancetaylor Thank you for looking into this. I apologise, the description of the PR w.r.t. getexecname() is indeed misleading if not wrong, let me run you through my thought process at the time:
- The actual fix for the underlying issue was this: https://github.com/swift-server/swift-backtrace/pull/36/files#diff-3b807e3382126cd3e623b416fa0ac4e79619f79db52cec4cba395daec969ac68R24 . So the crucial fix was to not start looking into
argv[0] (== CommandLine.arguments[0]). - Then I looked up what
getexecname()does (which was unknown to me -- which makes sense because it's not available on Linux) and the manpage says that it essentially returnsargv[0]so I concluded that this would give us worse information than/proc/self/exeand moved it down
Now with your new information that getexecname() only exists on Solaris (which in turn won't have /proc/*) I do agree that we indeed do not need to apply my changes to libbacktrace as long as we don't lose the actual fix. But that's in the Swift codebase so there's no risk we'll lose it.
Another way of phrasing this would be: On Linux, we'll have /proc/* but not getexecname(). On Solaris we'll have getexecname() but not /proc/* so their relative order doesn't matter a bit. I just didn't know that no Linuxes would have getexecname() and just did the change because it was easy.
@tomerd tl;dr your PR looks fine as is.
5 participants
motivation: catch up to fixes and improvments in underlying library
changes: