Introduction to Upstream GCC Patch Review
Review bandwidth is consistently identified by top maintainers as a primary bottleneck in GCC development. Reviewing patches is a vital contribution that helps the project move faster, and top maintainers frequently emphasize that they need more input from the community. By stepping up to review code, you build your credibility and network in the project, potentially making friends with maintainers you can later meet at events like the GNU Cauldron. Reviewing is also an excellent, lightweight way to keep up with new optimizations in the codebase and is the most obvious path to becoming an official maintainer. This guide is designed to help new contributors and aspiring reviewers take their first steps and understand exactly what to look for.
Overcoming Common Barriers
Many new reviewers hesitate due to perceived barriers, but these can easily be reframed:
"I don't like criticizing people." The critique is focused purely on the code, not the person. As long as your feedback remains focused on technical details, it is a perfectly normal professional interaction.
"What if I make a mistake?" Getting something wrong is part of the process and is perfectly okay in a fast-moving project. GCC has many built-in safety nets, including long release cycles, dedicated bug-fixing phases, nightly CI, and senior maintainers who will ultimately double-check and approve the patch.
"What if the submitter gets angry?" This is extremely rare, and the GCC project strictly follows the GNU Kind Communication guidelines. If someone becomes unpleasant or abusive, they are in the wrong, and other maintainers and community members will support you.
"I don't have enough experience." You can add significant value simply by reviewing low-hanging fruit, such as verifying new tests, checking cover letters, or reviewing simple API usage. Asking clarifying questions like "I don't understand what is going on here, could you explain?" is highly beneficial and helps bring new perspectives to the code.
"Maintainers will be annoyed if I butt in." The opposite is true; maintainers are very keen to have more people help with reviews. Even providing a partial review acts as a helpful shortcut that speeds up the final review for the global maintainers.
How to Pick a Patch to Review
You do not need to review every patch on the mailing list. Here are some heuristics for finding manageable patches:
Code you care about: Look for patches targeting an area important to you, such as a specific backend your company works on. You can speed-read email subjects for tags like aarch64, ipa, or vectorizer to quickly find relevant submissions.
Code you wrote: You are likely best-placed to review fixes and adjustments to code you originally contributed. Since you touched the code last, you will have a deep understanding of its context and original design.
Bug fixes for your reports: If someone fixes a bug you reported or caused, you probably already have the context needed to give useful feedback. You can utilize the analysis already recorded in the upstream Bugzilla PR to quickly understand the patch.
Code from colleagues: You may have more context from internal reviews when your teammates post patches. This also allows you to discuss sensitive issues offline if needed, making the review process much smoother.
The Review Process
Reviewing generally falls into three categories: Design, Implementation, and Testing. You can start with basic sanity checks and move into more complex areas as you gain experience.
1. Basic Sanity Checks
Before diving deep, verify the submission requirements:
- Verify that the patch is actually attached, as submitters sometimes forget, and an early reminder can save weeks of delay before they notice.
- Ensure there is a cover letter that coherently explains the motivation behind the patch.
Check that the ChangeLogs are present and that they accurately correspond to the attached patch.
- Ensure that any newly added files contain the correct copyright headers.
- Verify that the Bugzilla PR is linked correctly and read the PR to understand the full audit trail of the problem.
2. Testing Review
Reviewing testing procedures is often an accessible starting point:
Statement on testing: The submitter should explicitly state how the patch was tested in the cover letter. If they do not, you can simply ask them to provide this testing information.
Coverage: New features and optimizations should always add tests to the testsuite/. If new command-line options are introduced, check if there is coverage for the different argument values.
Test frameworks: Consider if the testing would be better suited for the self-test framework rather than DejaGnu, especially for touching internal data structures.
DejaGnu directives: Check if DejaGnu directives are correct, as newcomers often struggle to get these right.
Performance testing: If the patch adjusts optimization heuristics, verify that benchmark results are provided to justify why the changes make the code better.
3. Implementation Review
This focuses on local correctness and maintainability:
Existing APIs: Ensure the code uses existing internal containers and helpers rather than reinventing the wheel. Check if the code utilizes appropriate APIs (like REG_P(x) instead of GET_CODE(x) == REG) and uses the project's standard hashmaps and vectors.
Complexity: Verify that the implementation avoids O(n^2) algorithms, as GCC strongly prefers to avoid quadratic complexity.
Readability: Code is read 10 to 100 times more often than it is written, so look for helpful comments and factored-out helpers. Ensure the author uses gcc_assert to protect non-obvious invariants.
Memory Management: Check if memory is allocated explicitly or implicitly via new RTL creation. Verify that proper GC markings are correctly used for global containers.
4. Design Review
This is usually the hardest type of review to do without significant experience in the codebase:
API Necessity: Question whether new APIs are actually needed or if existing components could easily be reused. Ensure new APIs do not unnecessarily burden components that don't even use them.
Right Level: Evaluate whether an optimization is implemented at the correct stage, such as GIMPLE versus RTL, or if a target-specific change should be generalized to the middle-end.
External Consistency: For user-facing changes like new command-line options, consider whether the behavior is consistent with other compilers like Clang to avoid diverging user experiences.
Extensibility: Assess whether new code, such as a tuning hook or cost field, will be straightforward for future CPUs to extend.
Composing Your Reply
How you deliver your review is just as important as the technical content:
Be polite: Make sure to spell the submitter's name correctly. If it is their first contribution, explicitly thank them for the patch, as this has proven to be highly motivating for new developers.
Format properly: Always reply inline within the email rather than top-posting.
Structure feedback: Start your comments from high-level design concerns and gradually move down to low-level implementation details.
Batch style comments: Avoid sending reviews that only point out code style issues, as this can be very frustrating for submitters. Instead, batch formatting nits together with a more substantive code review.
Use questions: If you are not 100% sure about a critique, phrase your feedback as a question, such as "Is this correct? Isn't [X] a better fit?".
Call out the good: If the patch looks good overall, say "Looks good to me," even if you cannot officially approve it. Highlight and praise clever or well-written pieces of code rather than only focusing on criticism.
Avoid the "Death of a Thousand Round-trips": Collate your comments into a single coherent reply to prevent the submitter from having to endlessly respin their patch. Additionally, make sure to review the cover letter, as its rationale will be archived in the git commit history.