-
Notifications
You must be signed in to change notification settings - Fork 2
track PC and assert at beginning of blocks #387
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
Merged
Conversation
This file contains hidden or 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
This reverts commit 1765045.
this makes a fairly significant change to the GTIRB loader by inserting PC increment statements after instructions and checking the PC at the entry of blocks. this is done by changing the loader to maintain PC and branchtaken assignments, and inserting a default increment if there is no branch. this surely has bugs
idk why it produces invalid boogie. it's because of an empty target list but idk why
|
this fixes the problemetic cross pattern we saw, where there were extra blocks inserted before the original successor blocks. still remaining: some empty blocks? R0, Gamma_R0 := zero_extend32_32(load14), Gamma_load14; assert Gamma_R0; goto main_1536__0__FF2SjjvqSsOWEiG9v2p92Q_goto_main_1536__2__MYSV1omOQM2b08Jj~q4Iuw, main_1536__0__FF2SjjvqSsOWEiG9v2p92Q_goto_main_1536__1__5dIr9jyaTQaHqLa9MRdoFQ; + main_1536__3__smO~Vm6qTomoCpzw26RqNg: + assume {:captureState "main_1536__3__smO~Vm6qTomoCpzw26RqNg"} true; + goto ; main_1536__0__FF2SjjvqSsOWEiG9v2p92Q_goto_main_1536__1__5dIr9jyaTQaHqLa9MRdoFQ: assume {:captureState "main_1536__0__FF2SjjvqSsOWEiG9v2p92Q_goto_main_1536__1__5dIr9jyaTQaHqLa9MRdoFQ"} true; assume (!((R0[32:0] == 0bv32) == false));
this lets us catch cases outside of gtirb, and it allows the RemoveUnreachableBlocks phase to run before printing the warnings.
this fixes the DifferentialAnalysisTest seeing program counters in its loaded IR, but it is maybe undesirable to do a transformation in the load() method. regardless, the current approach (of keep PC, then remove if unwanted) seems to necessitate this, as later parts of the runutils pipeline make assumptions that PC wont be present and fail in unexpected ways otherwise.
to hopefully make sure it's picked up by the parameter form transform. despite this, it looks like it still is not working.
Co-authored-by: am <[email protected]>
ailrst
approved these changes
May 22, 2025
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.
this makes a fairly significant change to the GTIRB loader, with the goal of
supporting assertions about the PC at the beginning of each block, to make
sure that the PC matches the expected address.
this is done by changing the loader to maintain PC assignments and
inserting a default increment if there is no branch. each procedure in the
lifted program also gains a requires clause, to require PC is equal to its
entry point, and an ensures clause, to ensure PC is equal to R30.
this functionality is gated behind the
--pc
flag. this has 3 options:none
(the default) will not produce any PC statements (as close as possible, this is meant to match the previous behaviour before this PR)keep
will emit PC increment and assignment statements, but no assertions. this should let you inspect the PC behaviour without adding a verification burden. the PC operations are liable to be optimised away by BASIL transforms.assert
will emit PC statements, assertions at beginnings of blocks, and requires/ensures for all procedures.example usage:
however, the checked-in gts files were generated with a bug leading to incorrect PC addresses (UQ-PAC/gtirb-semantics#27). this leads to discrepancies like the lines indicated by
<===
. this is a direct jump, but the addresses from the branch instruction and the block address differ.in some cases (but not this example), this leads to false verification failures. here is a verification which will fail:
by using the updated test case files from #288, you can observe the verification succeeding.
extra changes affecting existing code
_PC
as an implicit global variable (like the registers)mem
issue in IRToBoogieNoVCbugs / todo