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

WIP: Register JITTed functions with libunwind #12380

Merged
merged 2 commits into from
Feb 6, 2016
Merged

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 30, 2015

I am extremely surprised that we didn't do this before - I guess libunwind
was just pretty good at figuring it out anyway, unlike the OS X unwind
library. However, for better backtraces, register the JITted functions
with libunwind, which also prevents libunwind from trying to access
invalid memory in a good number of cases.

WIP, because libunwind only supports 32 bit offsets between the .text
and .eh_frame sections, but MCJIT spreads the farther apart. I am in
the process of fixing this in libunwind.

return P;
}

#include <iostream>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@Keno
Copy link
Member Author

Keno commented Jul 30, 2015

I just mailed an appropriate patch upstream. Will update here once it's accepted.

@yuyichao
Copy link
Contributor

Do you have a reference for it?

Also, do you have any idea if this could be the cause of #11691 ? I tried to enable the test on top of your branch and it doesn't fixed the problem but maybe it could with the libunwind patch.

@Keno
Copy link
Member Author

Keno commented Jul 30, 2015

Yes, quite possibly. The failure mode is is a ReadOnlyMemoryError. However, note that this is LLVM 3.7 only, because the old JIT did not emit the appropriate unwind info.

@JeffBezanson
Copy link
Member

Yes, could you post a gist of the patch just in case?

@Keno
Copy link
Member Author

Keno commented Jul 30, 2015

@carnaval
Copy link
Contributor

@Keno any news ? libunwind often stops unwinding early with llvm-svn on jited code. I assume that it's because we often emit functions without frame pointers now, although I'm not sure what libunwind fallbacks to when proper unwind info is not provided.

@carnaval
Copy link
Contributor

also if it's less bothersome than libunwind upstream we can probably fix that by overriding the default allocator in the SectionManager thing.

@Keno
Copy link
Member Author

Keno commented Oct 5, 2015

I reminded upstream about the patch. Let's see if it sticks.

@yuyichao
Copy link
Contributor

Seems that the patch have been committed a few month ago? http://git.savannah.gnu.org/gitweb/?p=libunwind.git;a=commit;h=8afc33ce9f20538f048838e18680c120867ee0d2

@Keno
Copy link
Member Author

Keno commented Jan 31, 2016

Oh, huh. That's nice! Nobody told me about it.

@yuyichao
Copy link
Contributor

There also seems to be other improvements on libunwind master since I'm getting much better backtrace with it even without yours or @carnaval 's patch. I'm wondering when will they make a new release (given that the last one is more than 3yrs ago).

@tkelman
Copy link
Contributor

tkelman commented Jan 31, 2016

Hopefully soon after they merge the musl patch I sent them.

@carnaval
Copy link
Contributor

carnaval commented Feb 5, 2016

Alright, what is holding this up ? Can we move on to libunwind master/carry the patch in our tree and merge this ? @Keno ?

We need this also to get inlined frame in backtraces if we start using DWARF for that since even though libunwind guesses correctly it certainly can't invent those ghost frames.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 6, 2016

I just rebased this on current master (old branch backed up at yyc/kf/linuxunwind). Compilation passes with libunwind master but given this needs the UNW_INFO_FORMAT_IP_OFFSET format which is not available on the released version of libunwind, the linux CI will probably fail.

The libunwind master has a lot of features we need (much better unwind on x86 (and no segfault), aarch64 support, new dynamic unwind info format) and since there hasn't been a release since 3 years ago, and given libunwind isn't moving as fast as LLVM, I think we could try to ship the git master version of libunwind (fetch from git/make our own tarball) instead of the current released one. (Unless someone can convience libunwind to make a new release soon)

I am extremely surprised that we didn't do this before - I guess libunwind
was just pretty good at figuring it out anyway, unlike the OS X unwind
library. However, for better backtraces, register the JITted functions
with libunwind, which also prevents libunwind from trying to access
invalid memory in a good number of cases.

WIP, because libunwind only supports 32 bit offsets between the .text
and .eh_frame sections, but MCJIT spreads the farther apart. I am in
the process of fixing this in libunwind.
@yuyichao
Copy link
Contributor

yuyichao commented Feb 6, 2016

I pushed a commit to use my own tarball built from the libunwind master, let's see what the CI thinks.

carnaval added a commit that referenced this pull request Feb 6, 2016
WIP: Register JITTed functions with libunwind
@carnaval carnaval merged commit 79008cd into master Feb 6, 2016
@carnaval carnaval deleted the kf/linuxunwind branch February 6, 2016 19:31
@tkelman
Copy link
Contributor

tkelman commented Feb 7, 2016

Can we move that somewhere more permanent? Or bother upstream into making a new release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants