Fix function end detection #2636
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
umanwizard
force-pushed
the
fix-function-end
branch
2 times, most recently
from
b8a6695
to
709a4e2
Compare
When constructing unwind tables, we add a synthetic entry marking the end of functions. This serves to allow the unwinder to detect program counters that fall between two functions. We only insert this synthetic row if it's possible to give it a program counter that doesn't overlap with anything else; that is, if there is a gap between the end of the function in question and the beginning of the next. It's fine to omit the synthetic entries in the case where there is no gap, because in those cases, there isn't any way the PC can lie between to functions. However, currently we _also_ use the synthetic entries for another purpose: determining where it's okay to break the unwind table into shards (as it's not acceptable for a function to cross a shard boundary). This is imprecise, because the synthetic entries are not added in all cases, as described above. Indeed, binaries that have no gaps in a long enough sequence of functions can't be split anywhere and thus fail to be profiled. This commit fixes that issue by looking at the original FDEs (which have actual function begin and end information) directly, rather than trying to find function boundaries by using the synthetic rows. It also factors out the chunk-splitting logic into a separate function so it can be tested.
umanwizard
force-pushed
the
fix-function-end
branch
from
1b1603c
to
9ae2323
Compare
gnurizen
approved these changes
Mar 26, 2024
| @@ -1821,6 +1823,44 @@ func (m *Maps) allocateNewShard() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Get the largest chunk of `ut` (up to `maxLen`) that | |||
There was a problem hiding this comment.
FYI some linters require the first word of a function doc comment to be the function name itself, apparently we don't have such a linter which surprises me.
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.
When constructing unwind tables, we add a synthetic entry marking the end of functions. This serves to allow the unwinder to detect program counters that fall between two functions. We only insert this synthetic row if it's possible to give it a program counter that doesn't overlap with anything else; that is, if there is a gap between the end of the function in question and the beginning of the next.
It's fine to omit the synthetic entries in the case where there is no gap, because in those cases, there isn't any way the PC can lie between to functions.
However, currently we also use the synthetic entries for another purpose: determining where it's okay to break the unwind table into shards (as it's not acceptable for a function to cross a shard boundary). This is imprecise, because the synthetic entries are not added in all cases, as described above. Indeed, binaries that have no gaps in a long enough sequence of functions can't be split anywhere and thus fail to be profiled.
This commit fixes that issue by looking at the original FDEs (which have actual function begin and end information) directly, rather than trying to find function boundaries by using the synthetic rows. It also factors out the chunk-splitting logic into a separate function so it can be tested.