LLDB’s CODE_OWNERS file has been outdated for a long time. Some of the owners haven’t been actively involved for some time while some active contributors and de-facto code owners aren’t listed there. Inspired by @AaronBallman 's revamp of the clang code owners, I’m proposing we adopt the same model for LLDB and use it as an opportunity to update code ownership.
Motivation
Aaron did a great job describing why code ownership is important in his RFC, so I won’t repeat it here. One additional argument is that we’re well on our way to transition to Github PRs. This will lower the bar for new contributors and I expect to see an increase in contributors. That’s great, but to guarantee the quality of LLDB it will be imperative that those changes are appropriately reviewed. Having an up-to-date code ownership file will help with that. Github has way to automatically add code owners as reviewers and I know people on the infrastructure workgroup are actively looking into this.
Proposal
I propose to follow clang’s current code ownership model. Code ownership is structured by component/subject area with one or multiple code owners. I think this is representative of how things already work in practice for LLDB. In addition to the code owners, the blame list should always be used to pick reviewers.
Below is my proposal. I’ve broken it down in components, symbol and object file formats, supported platforms and finally specific tools. I’ve based the list below on my experience on the project, the current code ownership file and the git history. I have tried to be as objective as possible. If you feel someone has been overlooked: please speak up and make a nomination in the thread. I’ve omitted a few components that I felt are too heterogeneous, such as the Data Formatters for example. If you feel these areas deserve their own code owner or there are other areas that deserve their own code owner: please bring it up in this thread.
Components
ABI: Jason Molenda, David Spickett
Breakpoint: Jim Ingham
CMake & Build System: Jonas Devlieghere, Alex Langford
Commands: Jim Ingham
Expression Parser: Michael Buch, Jim Ingham
Interpreter: Jim Ingham, Greg Clayton
Lua: Jonas Delvieghere
Python: Med Ismail Bennani
Target/Process Control: Med Ismail Bennani, Jim Ingham
Test Suite: Jonas Devlieghere, Pavel Labath
Trace: Walter Erquinigo
Unwinding: Jason Molenda
Utility: Pavel Labath
ValueObject: Jim Ingham
Watchpoints: Jason Molenda
File Formats
(PE)COFF: Saleem Abdulrasool
Breakpad: Pavel Labath
CTF: Jonas Devlieghere
DWARF: Adrian Prantl, Greg Clayton
ELF: Pavel Labath
JSON: Jonas Devlieghere
MachO: Greg Clayton, Jason Molenda
PDB: Zequan Wu
Platforms
Android: Pavel Labath
Darwin: Jim Ingham, Jason Molenda, Jonas Devlieghere
FreeBSD: Ed Maste, Michał Górny
Linux: Pavel Labath, David Spickett
Windows: Omair Javaid
Tools
debugserver: Jason Molenda
lldb-server: Michał Górny, Pavel Labath
lldb-vscode: Greg Clayton, Walter Erquinigo
Election Process
The Developer Policy says we currently don’t have an official policy on how one gets elected to be a code owner. I suggest we follow the process we’ve used historically across LLVM where code owners are nominated and approved through consensus from multiple community members. This situation is a slightly special because I have included myself in the list. I hope it goes without saying that if anyone is uncomfortable with that or disagrees, please speak up!
I hadn’t really considered it, mostly because I would expected it to be covered by the owners of the respective LLVM backends. But it’s an important part of LLDB and as such I think it’s a good idea. Did you have someone in mind?
The lldbInterpreter library (source/Interpreter subdirectory). This covers the command and script interpreter and all the option classes.
I think the disassembler in particular was on the mind because Ted has a patch up against the disassembler and after looking at it for a while, I think it’s fine to try but I was waiting a bit to see if Greg wanted to have a final say on it (having commented earlier).
There are holes in the code owners layout - if I want to change the OperatingSystem or LanguageRuntime or InstrumentationRuntime plugins, whose approval do I need? The disassembler is a bit unique because it is a light layer of UI on top of llvm’s MCInst, although honestly I think this is one of our more undeveloped feature areas in lldb and someone really passionate about disassembly, and with the time, could add a lot to how we display assembly.
My guess is that Ted was pointing out the disassembler as an example of something that isn’t comprehensively covered by these general categories. There’s so many things in lldb, I don’t know if I can really argue for trying to document owners for all of them but maybe that would be best way to be approachable to new contributors. We have some categories that cover a lot of things “Platforms - Darwin” we can say that covers the Objective-C LanguageRuntime, or the Darwin DynamicLoader plugin, but who is the default reviewer for the C++ language runtime?
It is worth considering explicitly if we’re leaving holes in this document with the understanding that Responsible People will keep an eye on incoming PRs and say “oh, I’ll pick that up” or “I know who should review this” and add them to the list. Or if we want to spell out all of the codebase. I think the latter is a tough job.
I think you raise an excellent question. There’s definitely a trade-off here and as you point out, it’s worth making that explicit. We could reduce the granularity and assign code owners to bigger parts of the code base. It’s much easier to break up the codebase into larger pieces. As the developer policy points out, code owners don’t need to review code themselves, so they could defer to relevant reviewers. That puts a pretty big burden on a smaller group of people and was something I was explicitly trying to avoid.
Another option is to have a “top level” code owner who’s responsible for everything that’s not covered by others. If I had to nominate someone for this role it would be Pavel. He has shown a high level of understanding of many fundamental pieces of lldb. Plus he has essentially filled this role (de-facto) for a long time. @labath is this something you would be interested in and have the bandwidth for?
I want to make sure everyone is on the same page about what code ownership means in LLVM. Unlike some other projects where code owners have the final say about what goes in or not that’s not per se the case here. From the developer policy:
The sole responsibility of a code owner is to ensure that a commit to their area of the code is appropriately reviewed, either by themself or by someone else. […] Note that code ownership is completely different than reviewers: anyone can review a piece of code, and we welcome code review from anyone who is interested. Code owners are the “last line of defense” to guarantee that all patches that are committed are actually reviewed.
We still expect people to consult the blame list and pick relevant reviewers, regardless of whether they’re code owners.
We could list architectures too but since they cut across platforms and the work often has to be done multiple times for each one, it’s more of a “knows about” than “owns”. The owner for the first platform being modified could do the leg work if needed to find the architecture experts.
@JDevlieghere Re Windows: Though I am not the original author but I ll take ownership for the Windows port as I am maintaining the LLDB Windows on Arm buildbot and LLVM Windows on Arm releases.
Thank you for working to refresh code ownership within LLDB – having an updated list of code owners who are actually active in the project is super helpful! I fully support this RFC and the nominated folks.
I would recommend having such a person. It makes it easier for contributors to know there’s one person they can add to a review who ultimately is responsible for ensuring the review makes progress. It’s also theoretically helpful in the extremely rare case where you need a “tie breaker” between co-code owners (FWIW, we’ve yet to have such a situation arise in Clang). I would support @labath’s nomination for such a role.
While I don’t exactly mind being listed, I’d just like to point out that I haven’t touched this stuff in quite some time, and it’s unlikely that I will be working on LLDB in the near future.
Thank you for speaking up! In that case, I’d recommend not adding you as a code owner so that there’s less potential for friction (you feeling pressured to do reviews you don’t have time for, patch authors feeling like they’ve hit a bottleneck). Your voice still carries weight when you are able to comment on reviews and we could certainly nominate you again if you become more active in LLDB in the future.
One of the things I plan to bring up in another discussion is listing code owner Discourse and Discord handles in the CodeOwner files. Often times one needs a way to have a discussion with a code owner that is not through email or to bring someone into an active discussion on Discourse or Discord. I think at the bare minimum every single person listed here should be available on Discourse for RFC purposes.
I haven’t gone through this list and I see you have tagged some folks, but are all these folks on Discourse to see this post?
Everyone but Kamil is on Discourse and tagged in this thread. I reached out to Kamil over email, asking if they want to continue to remain code owner and willing to create/reactivate a Discourse account.
