-
Notifications
You must be signed in to change notification settings - Fork 60
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
Enable folly::Symbolizer for HHVM #123
base: main
Are you sure you want to change the base?
Conversation
# libgcc_s dependency is already part of HHVM, this enforces the | ||
# order in which the libraries are linked. | ||
if (ENABLE_FOLLY_SYMBOLIZER) | ||
SET(UNWIND_LIB -lgcc_s -lunwind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what happens on mac (clang) with and without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested on either mac or clang, which is why I protected this option under GCC in the related PR in HHVM: facebook/hhvm#8123
Do you think it is better to include the GCC requirement in folly as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually use Folly's build system - but given that https://github.com/facebook/folly/blob/9656bacc814ce9efd24e178ed5e127c5c365a248/CMake/FollyConfigChecks.cmake checks for elf.h , mac's most likely not going to be an issue - but Clang should work.
cc @simpkins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fredemmott can you please clarify if I should change anything for Clang?
This change depends on hhvm/hhvm-third-party#123, which enables 2 additional source files to be compiled and adds libunwind as a dependency for folly. The "-lgcc_s" added before "-lunwind" is used to solve a linking order issue that causes "_Ux86_64_setcontext" to be called from libunwind instead of libgcc_s and messes up the signal mask (more details here http://savannah.nongnu.org/bugs/?48486). The libgcc_s dependency is already part of HHVM, this change enforces the order in which the libraries are linked. By enabling folly::Symbolizer, HHVM will show debug symbols (in stacktraces, perf, GDB) when huge pages are enabled (default behavior for open source builds). Also, one nice side-effect is that this change causes HHVM to show symbols in backtraces (after a crash) for non-huge-pages builds, which were missing for some reason.
I've re-triggered the build; not sure if that's sufficient or a rebase will be needed. |
e4cba51
to
1845927
Compare
This PR enables support for folly::Symbolizer in HHVM if the ENABLE_FOLLY_SYMBOLIZER variable is set to ON.