Fix long-standing PT "taken" bug. #1568
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
For a while now we've noticed fishy behaviour in the PT decoder, where a compressed return is detected, but the following TNT bit is not one, as it should be. We also know that carrying on after seeing this causes nonsense traces. For this reason we recently added code to detect the bug and abort tracing. The reason for the bug is that `seek_tnt_or_tip()`, whose job it is to read packets until the next control flow event, would return any packet that carries a TIP (target instruction pointer) payload, even if the payload was "out of context" (PT terminology for "unknown TIP"). To follow the CFG properly, some (not all) call-sites to `seek_tnt_or_tip()` will be uninterested in out of context TIP updates and will want to ignore them. Critically `is_compressed_return()` *must* skip out of context TIPs, since they have no bearing on whether the return is compressed or not. This was the underlying cause of the taken bug. Rather than forcing call-sites to loop if they get an out of context TIP when they want to skip them, this change adds a `skip_ooc` flag to `seek_tnt_or_tip()` which determines whether it will skip OOC TIPs or not. Adding the flag also forces us to think about whether we should be skipping such packets or not. [Since compressed returns are currently disabled at tracing time, we should in fact never decode a compressed return. Due to this I've replaced a chunk of compressed return logic with an unreachable!()]
|
Excellent! |
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.
For a while now we've noticed fishy behaviour in the PT decoder, where a compressed return is detected, but the following TNT bit is not one, as it should be.
We also know that carrying on after seeing this causes nonsense traces. For this reason we recently added code to detect the bug and abort tracing.
The reason for the bug is that
seek_tnt_or_tip(), whose job it is to read packets until the next control flow event, would return any packet that carries a TIP (target instruction pointer) payload, even if the payload was "out of context" (PT terminology for "unknown TIP").To follow the CFG properly, some (not all) call-sites to
seek_tnt_or_tip()will be uninterested in out of context TIP updates and will want to ignore them.Critically
is_compressed_return()must skip out of context TIPs, since they have no bearing on whether the return is compressed or not. This was the underlying cause of the taken bug.Rather than forcing call-sites to loop if they get an out of context TIP when they want to skip them, this change adds a
skip_oocflag toseek_tnt_or_tip()which determines whether it will skip OOC TIPs or not.Adding the flag also forces us to think about whether we should be skipping such packets or not.
[Since compressed returns are currently disabled at tracing time, we should in fact never decode a compressed return. Due to this I've replaced a chunk of compressed return logic with an unreachable!()]