Avoid inlining exact_unwind
#52
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matthiasdiener
changed the title
pablogsal
requested changes
Apr 22, 2022
|
One possibility is trying to ensure this is never inlined and adjusting the memray/src/memray/_memray/tracking_api.h Line 152 in a35321a Or another way is transforming the function into a macro as @godlygeek suggested offline. |
|
This patch seems like it ought to do the trick - I tested for a few moments locally, and it seems to pass the test suite both for a diff --git a/src/memray/_memray/tracking_api.h b/src/memray/_memray/tracking_api.h
index 8118b43..06ab288 100644
--- a/src/memray/_memray/tracking_api.h
+++ b/src/memray/_memray/tracking_api.h
@@ -89,6 +89,7 @@ class NativeTrace
if (size == MAX_SIZE) {
d_data.resize(0);
size = exact_unwind();
+ skip += 1; // skip the non-inlined exact_unwind frame
MAX_SIZE = MAX_SIZE * 2 > size ? MAX_SIZE * 2 : size;
d_data.resize(MAX_SIZE);
}
@@ -122,7 +123,7 @@ class NativeTrace
return unw_backtrace((void**)data, MAX_SIZE);
}
- __attribute__((always_inline)) size_t inline exact_unwind()
+ __attribute__((noinline)) size_t inline exact_unwind()
{
unw_context_t context;
if (unw_getcontext(&context) < 0) { |
Thanks, that looks good! Many pytest tests are still failing, but the basic functionality seems to work fine. |
Even with the patch? Mind sharing the pytest output? |
Yes, even with the patch. |
Wow, any chance you can give us a stack trace with gdb? |
Sure, here is the trace: |
|
Hummm, could you compile with Alternatively (or additionally), run "disas" at the segfault to know what is going on with the instructions there. I suspect this may be the libunwind call but it may be the TLS as well. |
|
Taking a slightly closer look at that stack: it's glibc (good, I was afraid it might be musl). It's in the middle of a call to Based on this, I agree - it's more likely that libunwind has failed us than that TLS is the problem (I would have suspected TLS if we were in the new thread instead) |
godlygeek
force-pushed
the
ppc64le
branch
3 times, most recently
from
16a8d36
to
1b347ab
Compare
On some architectures this can't be inlined. We don't actually care whether or not it is inlined, only that whether it is or isn't inlined is consistent regardless of build flags. Signed-off-by: Matthias Diener <mdiener@illinois.edu> Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
godlygeek
changed the title
exact_unwind
pablogsal
approved these changes
May 17, 2022
This was referenced May 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue number of the reported bug or feature request: #51
Describe your changes
Remove inline attribute from exact_unwind()
Testing performed
Compiled and ran a simple test on a ppc64le machine.