Skip to content
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

generator: ensure generation of conditional branches #38

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

cwshugg
Copy link
Collaborator

@cwshugg cwshugg commented Jan 20, 2023

This PR implements two new config fields that give the user better control over the program generator's branch placement. (This seeks to resolve issue #37). Here's a description of what these new fields do:

New Config Field 1 - conditional_branch_selection_attempts

The program generator uses an internal list of control-flow instruction specifications to generate random branch instructions at the end of basic blocks. This internal list contains both conditional and unconditional branches. So, when a random control-flow instruction is generated, it's possible either type will be produced.

The conditional_branch_selection_attempts config field helps control this. This PR adds a loop to the program generator that is controlled by this field. When generating a random control-flow instruction, this loop will:

  • (If the field is set to a positive integer, X) Repeatedly generate control-flow instructions X number of times until a conditional branch is found.
  • (If the field is set to -1) Repeatedly generate control-flow instructions, forever, until a conditional branch is found.
  • (If the field is set to 0) Don't bother looking for a conditional branch; always use unconditional branches.

New Config Field 2 - place_adjacent_unconditional_branches

(This doesn't address issue #37, but it was trivial to implement and some may find it useful.) By default, the program generator places an unconditional branch instruction at the end of each basic block that targets the next-adjacent basic block. For example:

# ... code
.bb_main.2:
# ... code
B .bb_main.3 # jump to BB3
.bb_main.3: # BB3 appears immediately after
# ... code

Now, place_adjacent_unconditional_branches controls whether or not this happens. It's true by default (to maintain the original behavior), but setting it to false will prevent these unconditional-branches-to-the-next-adjacent-basic-block from being placed.

@OleksiiOleksenko
Copy link
Contributor

Unfortunately, this solution is incompatible with the other parts of the generation algorithm. Each test case has a well-defined control flow graph (see here). If we randomly select between conditional and unconditional branches, then in many cases the basic blocks that are supposed to have two successors will have one (and the other way around).

To maintain consistency, issue #37 has to be fixed at an earlier stage of the generation algorithm, when the control flow graph is being created.

@cwshugg
Copy link
Collaborator Author

cwshugg commented Jan 23, 2023

Thanks for clarifying @OleksiiOleksenko. When inspecting Revizor's test cases, I noticed several cases where a basic block ended with two unconditional branches, such as:

# ...
AND X0, X3, X5
.bb_main.0:
AND W3, W2, W4
B .bb_main.1
B .bb_main.2
.bb_main.1:
# ...

I discovered the second unconditional branch was generated in a function call that appeared to expected a conditional branch to be returned (here). In many cases, at least for the Arm ISA specification, that function would spit out an unconditional branch. That's why I took the "try X number of times to get that function to spit out a conditional branch" approach with CONF.conditional_branch_selection_attempts.

I certainly don't want to tamper with the successor generation prior to where I modified the code. How can we ensure the random control-flow choice at line 547 always chooses a conditional branch?

@OleksiiOleksenko
Copy link
Contributor

Oh, I see, I misunderstood the PR then, sorry. Re-openning.

@cwshugg
Copy link
Collaborator Author

cwshugg commented Jan 23, 2023

No worries - thanks for taking a second look. I'll be sure to double-check things, but these two new config fields, when left at their default values, shouldn't modify the generator's behavior (including the two new min/max successor config fields you added today). I'll merge in your changes and do some testing, then make a final push.

@OleksiiOleksenko
Copy link
Contributor

Let me do it. I want to make a few performance-related changes anyways - e.g., searching for conditional branches every time we generate a BB is redundant. It would be much faster to make a list of cond branches beforehand and then just randomly pick from them.

@cwshugg
Copy link
Collaborator Author

cwshugg commented Jan 23, 2023

Sounds good, the branch is all yours 👍. And great point, that would be much more efficient than my approach. Thanks for the help!

@OleksiiOleksenko
Copy link
Contributor

place_adjacent_unconditional_branches won't work in the current form. Conditional branches have a limited range of how far they can jump (2^19 bytes on ARM and as low as 2^8 on x86). Hence, setting anything besides the next basic block as the target of the cond jump will often lead to an invalid assembly. This feature is much more complicated to implement than it may seem. I'm rolling it back now, but feel free to take a crack at it in a separate PR - if you feel adventurous.

@OleksiiOleksenko OleksiiOleksenko changed the title Configurable Branch Placement generator: ensure generation of conditional branches Jan 23, 2023
@OleksiiOleksenko OleksiiOleksenko merged commit e575879 into arm-port Jan 23, 2023
@OleksiiOleksenko OleksiiOleksenko deleted the configurable_branches branch January 23, 2023 16:18
OleksiiOleksenko added a commit that referenced this pull request Feb 16, 2023
Co-authored-by: Oleksii Oleksenko <alexo_o@ukr.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants