First, please read the Workflow, Code Content, and Code Style guidelines.

A code review is required for any merge to the master branch. We use shared feature branches stored in Github, and use pull requests from a feature branch to master using squash-and-fast-forward-merge as our review mechanism.

Submitting a Change as a Non-Project Member

If you are not (yet) a member of the Committers group for DynamoRIO and thus do not have write access to the repository, you can contribute an individual source code change by having an existing developer commit it. Rather than using a feature branch on a clone of DynamoRIO/dynamorio and using git review as described below, you'd fork DynamoRIO/dynamorio into your personal github account and submit a pull request from there to the master branch of DynamoRIO/dynamorio. You are expected to follow our Code Style and, just like with project feature branches, do not force push to your personal branch: it is shared history once in a pull request.

In order for the DynamoRIO automated tests to run on your pull request, be sure that you have enabled Allow all actions under Actions in the Settings of your forked repository.

We recommend submitting a few small patches in this manner to demonstrate your work before asking to be added as a project member.

Once you become a DynamoRIO project member, please switch to our workflow of using branches within the DynamoRIO/dynamorio repository rather than using personal cloned repository branches. This makes it easier for others to contribute toward or take over and finish off pull requests while still giving you credit upon merge. (It is not uncommon for an open source PR author to be busy and unable to quickly finish off small requested changes, and we'd like to be able to step in and easily finalize it within the same PR.)

Requesting a Review as a Project Member

You should have already cloned the repository and run the devsetup.sh script as described on our Workflow page.

For a typical single-commit review, first develop and test your feature or bug fix locally. Be sure to give your branch a name following the conventions described in Workflow. When the code is ready to be reviewed and has been cleaned up and squashed into one commit (see below for multi-commit larger features), push your local branch to Github via:

git review

That command will only work if you've run the devsetup script (which you should have done!).

Now use Github's web interface to create a pull request. We do not currently have command line support set up for this. Immediately after pushing your branch, there is a convenience "Compare & pull request" under "Your recently pushed branches" on the Code main page. In general, you can click on "New pull request" and select your branch on the right side "compare: <branchname>". Finalize by clicking "Create pull request". You can select a particular reviewer if desired.

Commit Messages

Commit messages should include an initial title line separated from the body of the message by a blank line. The title line should be a concise summary and should start with the issue number followed by a colon which is followed by the description of the issue fix. If the issue is fixed by more than one commit, the title should instead start with the issue number followed by a space and a short issue description, then a colon, and then a description of this particular fix, with a reference to the issue at the bottom for automatic linking in Github. In either case, the body of the message should elaborate on the contents of the commit using complete sentences.

For a commit that fixes an Issue, be sure to resolve the Issue with a message indicating the commit revision number. If you use the exact string "Fixes: #NNNNN" (where NNNNN is replaced by an Issue number) with nothing else on that line in the commit message, the git push will auto-update the Issue to say "Fixed", avoiding any need to manually close the issue. You can also auto-close a Dr. Memory issue with "Fixes: DynamoRIO/drmemory#NNN". You can see other options in the GitHub documentation.

The string "Fixes: #NNNNN" in a Pull Request description will also close that issue, even if the final commit message does not contain that string, so be sure to edit the PR description if it's decided that the issue should stay open.

If a commit does not fully fix an issue, include a reference to the issue with a line of the form "Issue: #NNNNN". Use this to add links to related issues as well, in a format that makes it easier for tools to locate within messages.

An example of a single commit fixing an issue:

i#3192: move delayed branches after markers (#3193)

Fixes the problem of delayed branches being inserted between the
timestamp and cpuid marker in drcachesim's offline traces by delaying
the delayed branch until after all markers.

Fixes: #3192

And an example of a commit contributing to a larger issue:

i#1729 offline traces: fix thread entry order issue

Fixes a raw2trace problem where the first bb for a thread switch can be
incorrectly attributed to the prior thread.

Issue: #1729

For multi-commit solutions to an Issue, adding a "part N" type of label can make it easier to see that each one is incremental:

i#837 AVX2 ISA update, part 4: add VSIB support

Adds support for the new addressing mode, the Vector SIB or VSIB.  This
references multiple addresses based on indices in an xmm or ymm register.

Issue: #837

Note that there is no "i" before the "#" for issue references when communicating with Github, but that our convention is to prefix the "i" for our own references to make it clearer that it's an issue and not a pull request or some other hashtag.

Review Sizes

Reviews should be kept to a moderate size for more focused responses and more isolated commits. Large projects should be split into separate logical pieces for review. Review diffs larger than about 1500 lines should be avoided.

Staging Large Commits

We do not want enormous diffs that are impractical to review because an entire polished feature was developed in isolation. Larger features should be split into pieces and either committed incrementally onto a project branch or merge into master in incremental steps. For sharing work and providing visibility into ongoing projects, we prefer using project branches. For incremental experimental work, the typical approach is to introduce the new code in a disabled state, either via runtime option or ifdef or both, until stable. Unfinished parts that are committed should be clearly labeled as such.

New Committers

For new committers, code review and commit sizes should be small: less than 100 lines for the first few commits. We may reject reviews that are larger.

Continuous Integration Checks

Pull requests are integrated into our continuous integration system, providing pre-commit testing of all commits. Both Github Actions and Jenkins tests are immediately run when a pull request is created and after each subsequent push to the pull request branch. The request can't be merged until the tests complete and pass.

Please note that our test suite is NOT THE SAME AS RUNNING "make test". The pre-commit test suite builds multiple builds and enables more tests than "make test". Running tests in just one build is not sufficient. See Testing for information on setting up and running the test suite locally via the suite/runsuite.cmake script.

Unfortunately we do have some flaky tests that are not yet fixed. These are auto-ignored via a list in the runsuite_wrapper script.

Responding to Review Comments and Submitting Code Updates

Upon receiving the email requesting a review, the reviewer will visit the pull request page and add comments to the code as part of a Github review, including comments on individual lines. An email will be sent with these comments. You should view the comments, address them in your code, reply to each comment on the review site (typically by saying "Done" if you agree with the request and have made the change in your code), and (if the topic is settled) marking as "Resolved". Please read and reply to every comment. Github will not allow a final merge unless every comment is resolved.

Reply to comments individually at the point in the code view where they appear. Do not simply reply at the top level, as such comments are more difficult to follow as a conversation thread with context.

After making changes in response to review comments, commit those changes locally as a new commit on top of your original commit. If the reviewer requested changes to the commit message, use a new proposed final commit message as the message for this change commit so that the reviewer can review the new message.

Project members should then push the new commit to the pull request via:

git review

Do not use a force push to change the history of the shared branch! Use a new, separate commit.

When the requested changes have been pushed, request a re-review from the reviewer so they know that the pull request is ready for another round of reviewing. This can be done by clicking the re-review button next to the reviewer's name at the top of the right sidebar on the pull request page.

The final squash-and-merge step will squash these additional commits with the original to make a single commit that will be fast-forward-merged into master (see below for more details).

Updating from Master

During local development before requesting review, i.e., before your feature branch is on the server, rebasing to pull in new changes from master is the preferred workflow. But once you've pushed your branch and it's shared, do not rebase, as it will destroy history in the pull request. Instead, merge changes from master if an update is needed (but see below: normally you should only do this at the very end after approval). These merge commits will disappear when the feature branch is fast-forward-merged to master.

Generally, it's better to not update at all once a review has started. Do not click on the "Update Branch" until the review is approved and you are ready to merge into master. Clicking on it earlier when you may still need to make changes to your code just wastes resources. Furthermore, in some code review systems it corrupts the reviewer's view of the code, so it is a bad habit to get into.

Merging into Master and Cleaning Up the Final Commit Message

Only once the reviewer marks the proposed code changes as accepted and all of the continuous integration tests pass can the change be merged into master. If you have merge privileges, when performing the squash-and-merge, be sure to edit the final commit message (after pressing the squash-and-merge button, Github presents the final message in an edit box) to create a clean description of the commit without the messy incremental steps from the multiple commits fixing small issues during the review process. (You can resize the edit box by dragging the control in the bottom right.) Especially be sure to remove all commit message titles from the body of the final squash-and-merge message**, as the commit title is in a separate edit box (and defaults to the title of the first commit, so update it as well if the title changed during review). Also be sure to avoid truncation of the commit title, as Github likes to replace the end with "...".

To clarify further, here is an example of what Github will present in the squash-and-merge step for the message body:

* i#3192: move delayed branches after markers

Fixes the problem of delayed branches being inserted between the
timestamp and cpuid marker in drcachesim's offline traces by delaying
the delayed branch until after all markers.

Fixes: #3192

* Fix clang warning

* Address reviewer comment suggestions

All the lines starting with a * are commit message titles from commits in the pull request branch. They should all be removed, including the first one. A separate single-line edit box contains the title line for the commit message, which will be added to the front of the description by Github. Thus, the final description edit box should contain no title at all:

Fixes the problem of delayed branches being inserted between the
timestamp and cpuid marker in drcachesim's offline traces by delaying
the delayed branch until after all markers.

Fixes: #3192

When editing the title edit box, leave in place the pointer to the pull request, which Github automatically adds to the end of the commit message title, in parentheses, for its suggested final squash-and-merge title:

i#3192: move delayed branches after markers (#3193)

Re-iterating as there is a tendency to just click through the squash-and-merge: please edit and clean up the final commit message!

Deleting the Branch

Generally, the feature branch should be deleted from the repository using the button that Github provides immediately after merging.

Review Delays

With an open-source project like this where each contributor has a day job with other priorities, code reviews should be expected to take a day or two, and longer if the change is lengthy. However, if the reviewer expects to not have time to complete the review within 2 days he or she should notify the author up front to give a chance to switch to a different reviewer.

For reviews performed by developers all working on the same active project as part of a day job, reviews should be much more timely: within a few hours, to avoid disrupting productivity.

Project Branches

For larger, more experimental features that do not easily fit into the one-commit-per-merge model, project branches are used. The review process is identical to above but substituting the project branch for master: i.e., single-commit feature branches are still used and reviewed, but they are squash-and-fast-forward-merged into the project branch.

Clean, reviewed commits then accumulate in the project branch until they are all ready to be merged into master. Here, a rebase is used rather than a squash-and-merge.