Widen modidx field in offline_entry_t. #2969
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
Extend the modidx field from 12 bits to 16. The 4 bits are claimed from the instr_count field which records the number of instructions in the basic block. The number of instructions is controlled by a command line parameter. We check that the max_bb_instr is set to number lower than what we can accomodate in the bitfield. Fixes DynamoRIO#2956
Extend the modidx field from 12 bits to 16. The 4 bits are claimed from the instr_count field which records the number of instructions in the basic block. The number of instructions is controlled by a command line parameter. We check that the max_bb_instr is set to number lower than what we can accomodate in the bitfield. Fixes DynamoRIO#2956
…o into i2956-modidx-overflow
Extend the modidx field from 12 bits to 16. The 4 bits are claimed from the instr_count field which records the number of instructions in the basic block. The number of instructions is controlled by a command line parameter. We check that the max_bb_instr is set to number lower than what we can accomodate in the bitfield. Fixes DynamoRIO#2956
Extend the modidx field from 12 bits to 16. The 4 bits are claimed from the instr_count field which records the number of instructions in the basic block. The number of instructions is controlled by a command line parameter. We check that the max_bb_instr is set to number lower than what we can accomodate in the bitfield. Fixes DynamoRIO#2956
| uint64_t modidx:12; | ||
| uint64_t instr_count:16; | ||
| uint64_t modidx:16; | ||
| uint64_t instr_count:12; |
There was a problem hiding this comment.
I would suggest either a comment here advising that a change here needs to also update the hardcoded sizes in the 2 other files, or to use named constants (as is done with EXT_VALUE_A_BITS, e.g.) -- I would vote for the named constants.
| uint64_t modoffs = dr_app_pc_as_jump_target(instr_get_isa_mode(where), pc) - modbase; | ||
| // Check that the values we want to assign to the bitfields in offline_entry_t do not | ||
| // overflow. In i#2956 we observed an overflow for the modidx field. | ||
| DR_ASSERT(modoffs < uint64_t(1) << 34); |
There was a problem hiding this comment.
These should all be the bitfield size, not +1
Use named constants for the bit widths. Fix bug in bit width check.
|
PTAL. Thanks! |
Fix typo in bitfield declaration.
derekbruening
approved these changes
Apr 28, 2018
snehasish
added a commit
to snehasish/dynamorio
that referenced
this pull request
Apr 29, 2018
This reverts commit 48db566 which reverted the changes in PR DynamoRIO#2940. The changes pushed caused some apps to overflow the modidx field (issue DynamoRIO#2956). PR DynamoRIO#2969 increased the width of the modidx field. We can now safely revert the revert.
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.
Extend the modidx field from 12 bits to 16. The 4 bits are claimed from
the instr_count field which records the number of instructions in the
basic block. The number of instructions is controlled by a command line
parameter. We check that the max_bb_instr is set to number lower than
what we can accomodate in the bitfield.
Fixes #2956