This is an archive of the discontinued LLVM Phabricator instance.

Differential D156949

[lldb] Update LLDB Code Ownership
ClosedPublic

Authored by JDevlieghere on Aug 2 2023, 4:33 PM.

Details

Reviewers
jasonmolenda
DavidSpickett
jingham
bulbazord
Michael137
clayborg
mib
labath
wallace
compnerd
aprantl
zequanwu
emaste
omjavaid
aaron.ballman
Commits
rG013a8fc26ef8: [lldb] Update LLDB Code Ownership
Summary

https://discourse.llvm.org/t/rfc-update-lldb-code-ownership/72253

Diff Detail

Event Timeline

JDevlieghere created this revision.Aug 2 2023, 4:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 4:33 PM
Herald added a subscriber: yaxunl. · View Herald Transcript
JDevlieghere requested review of this revision.Aug 2 2023, 4:33 PM
Harbormaster completed remote builds in B249906: Diff 546639.Aug 2 2023, 4:36 PM
mib accepted this revision.Aug 2 2023, 4:49 PM

LGTM!

This revision is now accepted and ready to land.Aug 2 2023, 4:49 PM
jasonmolenda accepted this revision.Aug 2 2023, 5:01 PM
tonic added a subscriber: tonic.Aug 2 2023, 6:13 PM

Since you are going to the effort of changing the CodeOwners file format from what is in LLVM, can you go ahead and add Discourse and Discord handles? It is often very helpful to tag someone on Discourse/Discord. I'll be proposing this change for LLVM soon.

JDevlieghere updated this revision to Diff 546694.Aug 2 2023, 9:59 PM

Add Discourse and Discord usernames.

Harbormaster completed remote builds in B249950: Diff 546694.Aug 2 2023, 10:02 PM
DavidSpickett added inline comments.Aug 3 2023, 1:00 AM
lldb/CodeOwners.rst
8–9

This could be taken to mean every review must have approval from a code owner, on top of whatever other review has been done. Is that the intent? Someone coming from a project with strong maintainer rules (e.g. GDB, so I gather) may take it that way.

215

Is the extra space an RST thing, seems placed randomly.

aaron.ballman accepted this revision.Aug 3 2023, 4:58 AM

LGTM modulo nits found by others, thank you for this!

JDevlieghere added inline comments.Aug 3 2023, 7:33 AM
lldb/CodeOwners.rst
8–9

I copied this from the Clang CodeOwners.rst with the aim of being consistent, but I'm happy to tweak it. We could qualify the last sentence with something like "when consensus cannot be reached" or if we think "gatekeeper" is too strong of a work maybe we can use "tie-breaker", though I like that the former implies a sense of duty. Happy to take suggestions!

215

Nope that's just me: I'll clear that up 👍

DavidSpickett added inline comments.Aug 3 2023, 7:42 AM
lldb/CodeOwners.rst
8–9

My understanding was that llvm in general didn't have this hard requirement for an owner to acknowledge every review.

So yeah:
"They are also the gatekeepers for their part of LLDB, with the final word on what goes in or not when consensus cannot be reached."

Sounds good to me.

JDevlieghere updated this revision to Diff 546897.Aug 3 2023, 8:54 AM
JDevlieghere marked 3 inline comments as done.
  • Update wording
  • Remove double newline
lldb/CodeOwners.rst
8–9

My understanding was that llvm in general didn't have this hard requirement for an owner to acknowledge every review.

Yup, that's my understanding as well!

Harbormaster completed remote builds in B250089: Diff 546897.Aug 3 2023, 8:58 AM
tonic added a comment.Aug 3 2023, 11:24 AM
lldb/CodeOwners.rst
8–9

I would not put policy regarding code owners in this document. The policy is already in the DeveloperPolicy for the project. You could reference back to that document if you want.

JDevlieghere updated this revision to Diff 547099.Aug 3 2023, 9:06 PM
JDevlieghere marked an inline comment as done.

Link to developer policy.

Harbormaster completed remote builds in B250249: Diff 547099.Aug 3 2023, 9:09 PM
aaron.ballman added inline comments.Aug 4 2023, 6:07 AM
lldb/CodeOwners.rst
8–9

Good suggestion! I'm going to make similar changes to the Clang code owners file.

labath accepted this revision.Aug 4 2023, 7:07 AM

I've suggested some additional/alternative/backup (choose your interpretation) owners for the components I'm listed as the only owner (I only wish I could find someone to take over android). If they accept, then take this as my endorsement to add to or replace me.

lldb/CodeOwners.rst
108

@JDevlieghere

133

@zequanwu

151

@DavidSpickett

221

@DavidSpickett

DavidSpickett added inline comments.Aug 4 2023, 7:23 AM
lldb/CodeOwners.rst
151

Ok with me.

221

Ok with me.

zequanwu added inline comments.Aug 4 2023, 8:32 AM
lldb/CodeOwners.rst
133

I accept this.

JDevlieghere updated this revision to Diff 547274.Aug 4 2023, 10:35 AM
JDevlieghere marked 7 inline comments as done.
  • Add Pavel's nominations
Harbormaster completed remote builds in B250374: Diff 547274.Aug 4 2023, 10:38 AM
compnerd accepted this revision.Aug 4 2023, 3:18 PM
omjavaid accepted this revision.Aug 7 2023, 4:57 AM
DavidSpickett accepted this revision.Aug 7 2023, 5:52 AM

LGTM.

bulbazord accepted this revision.Aug 7 2023, 9:07 AM
JDevlieghere added a comment.Aug 8 2023, 8:41 AM

Alright, seems like we have consensus. The only person that hasn't chimed in yet is Greg, but based on a comment in another thread he might be OOO. We can alway address concerns post-commit.

Closed by commit rG013a8fc26ef8: [lldb] Update LLDB Code Ownership (authored by JDevlieghere). · Explain WhyAug 8 2023, 8:54 AM
This revision was automatically updated to reflect the committed changes.
JDevlieghere added a commit: rG013a8fc26ef8: [lldb] Update LLDB Code Ownership.
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 8:54 AM

Revision Contents

PathSize
lldb/
244 lines

Diff 546639

lldb/CODE_OWNERS.txt

  • This file was deleted.
This file is a list of the people responsible for ensuring that patches for a
particular part of LLDB are reviewed, either by themself or by someone else.
They are also the gatekeepers for their part of LLDB, with the final word on
what goes in or not.
The list is sorted by surname and formatted to allow easy grepping and
beautification by scripts. The fields are: name (N), email (E), web-address
(W), PGP key ID and fingerprint (P), description (D), and snail-mail address
(S).
N: Greg Clayton
E: clayborg@gmail.com
D: Overall LLDB architecture, Host (common+macosx), Symbol, API, ABI, Mac-specific code,
D: DynamicLoader, ObjectFile, IOHandler, EditLine, Core/Value*, Watchpoints, debugserver
D: Build scripts, Test suite, Platform, gdb-remote, Anything not covered by this file
N: Jim Ingham
E: jingham@apple.com
D: Overall LLDB architecture, Thread plans, Expression parser, ValueObject, Breakpoints, ABI
D: Watchpoints, Trampolines, Target, Command Interpreter, C++ / Objective-C Language runtime
D: Expression evaluator, IR interpreter, Clang integration, Data Formatters
N: Ed Maste
E: emaste@freebsd.org
D: FreeBSD
N: Jason Molenda
E: jmolenda@apple.com
D: ABI, Disassembler, Unwinding, iOS, debugserver, Platform, ObjectFile, SymbolFile,
D: SymbolVendor, DWARF, gdb-remote
N: Kamil Rytarowski
E: kamil@netbsd.org
D: NetBSD
N: Zachary Turner
E: zturner@google.com
D: CMake build, Host (common+windows), Plugins/Process/Windows, Anything Windows-specific
D: Test suite
N: Pavel Labath
E: labath@google.com
D: Linux, Android
N: Walter Erquinigo
E: a20012251@gmail.com
E: waltermelon@fb.com
D: Trace, TraceCursor, TraceExport, intel-pt, lldb-vscode, Data Formatters
N: Med Ismail Bennani
E: ismail@bennani.ma
E: m.i.b@apple.com
D: Breakpoints, Target, General Process Execution, Command Interpreter
D: Script Interpreter, API, Data Formatters, CrashLog, Scripted Process

lldb/CodeOwners.rst

  • This file was added.
================
LLDB Code Owners
================
This file is a list of the people responsible for ensuring that patches for a
particular part of LLDB are reviewed, either by themself or by someone else.
They are also the gatekeepers for their part of LLDB, with the final word on
what goes in or not.
DavidSpickettUnsubmitted
Done
ReplyInline Actions

This could be taken to mean every review must have approval from a code owner, on top of whatever other review has been done. Is that the intent? Someone coming from a project with strong maintainer rules (e.g. GDB, so I gather) may take it that way.

DavidSpickett: This could be taken to mean every review must have approval from a code owner, on top of…
JDevlieghereAuthorUnsubmitted
Done
ReplyInline Actions

I copied this from the Clang CodeOwners.rst with the aim of being consistent, but I'm happy to tweak it. We could qualify the last sentence with something like "when consensus cannot be reached" or if we think "gatekeeper" is too strong of a work maybe we can use "tie-breaker", though I like that the former implies a sense of duty. Happy to take suggestions!

JDevlieghere: I copied this from the Clang `CodeOwners.rst` with the aim of being consistent, but I'm happy…
DavidSpickettUnsubmitted
Done
ReplyInline Actions

My understanding was that llvm in general didn't have this hard requirement for an owner to acknowledge every review.

So yeah:
"They are also the gatekeepers for their part of LLDB, with the final word on what goes in or not when consensus cannot be reached."

Sounds good to me.

DavidSpickett: My understanding was that llvm in general didn't have this hard requirement for an owner to…
JDevlieghereAuthorUnsubmitted
Done
ReplyInline Actions

My understanding was that llvm in general didn't have this hard requirement for an owner to acknowledge every review.

Yup, that's my understanding as well!

JDevlieghere: > My understanding was that llvm in general didn't have this hard requirement for an owner to…
tonicUnsubmitted
Done
ReplyInline Actions

I would not put policy regarding code owners in this document. The policy is already in the DeveloperPolicy for the project. You could reference back to that document if you want.

tonic: I would not put policy regarding code owners in this document. The policy is already in the…
aaron.ballmanUnsubmitted
Done
ReplyInline Actions

Good suggestion! I'm going to make similar changes to the Clang code owners file.

aaron.ballman: Good suggestion! I'm going to make similar changes to the Clang code owners file.
.. contents::
:depth: 2
:local:
Current Code Owners
===================
The following people are the active code owners for the project. Please reach
out to them for code reviews, questions about their area of expertise, or other
assistance.
All parts of LLDB not covered by someone else
----------------------------------------------
| Jonas Devlieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
Components
----------
These code owners are responsible for particular high-level components within
LLDB.
ABI
~~~
| Jason Molenda
| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub)
| David Spickett
| david.spickett\@linaro.org (email), DavidSpickett (Phabricator), DavidSpickett (GitHub)
Breakpoint
~~~~~~~~~~
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
CMake & Build System
~~~~~~~~~~~~~~~~~~~~
| Jonas Devlieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
| Alex Langford
| alangford\@apple.com (email), bulbazord (Phabricator), bulbazord (GitHub)
Commands
~~~~~~~~
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
Expression Parser
~~~~~~~~~~~~~~~~~
| Michael Buch
| michaelbuch12\@gmail.com (email), Michael137 (Phabricator), Michael137 (GitHub)
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
Interpreter
~~~~~~~~~~~
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
| Greg Clayton
| gclayton\@fb.com (email), clayborg (Phabricator), clayborg (GitHub)
Lua
~~~
| Jonas Delvieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
Python
~~~~~~
| Med Ismail Bennani
| ismail\@bennani.ma (email), mib (Phabricator), medismailben (GitHub)
Target/Process Control
~~~~~~~~~~~~~~~~~~~~~~
| Med Ismail Bennani
| ismail\@bennani.ma (email), mib (Phabricator), medismailben (GitHub)
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
Test Suite
~~~~~~~~~~
| Jonas Devlieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
Trace
~~~~~
| Walter Erquinigo
| a20012251\@gmail.com (email), wallace (Phabricator), walter-erquinigo (GitHub)
Unwinding
~~~~~~~~~
| Jason Molenda
| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub)
labathUnsubmitted
Done
ReplyInline Actions

@JDevlieghere

labath: @JDevlieghere
Utility
~~~~~~~
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
ValueObject
~~~~~~~~~~~
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
Watchpoints
~~~~~~~~~~~
| Jason Molenda
| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub)
File Formats
------------
The following people are responsible for decisions involving file and debug
info formats.
(PE)COFF
~~~~~~~~
| Saleem Abdulrasool
| compnerd\@compnerd.org (email), compnerd (Phabricator), compnerd (GitHub)
labathUnsubmitted
Done
ReplyInline Actions

@zequanwu

labath: @zequanwu
zequanwuUnsubmitted
Done
ReplyInline Actions

I accept this.

zequanwu: I accept this.
Breakpad
~~~~~~~~
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
CTF
~~~
| Jonas Devlieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
DWARF
~~~~~
| Adrian Prantl
| aprantl\@apple.com (email), aprantl (Phabricator), adrian-prantl (GitHub)
| Greg Clayton
| gclayton\@fb.com (email), clayborg (Phabricator), clayborg (GitHub)
labathUnsubmitted
Done
ReplyInline Actions

@DavidSpickett

labath: @DavidSpickett
DavidSpickettUnsubmitted
Done
ReplyInline Actions

Ok with me.

DavidSpickett: Ok with me.
ELF
~~~
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
JSON
~~~~
| Jonas Devlieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
MachO
~~~~~
| Greg Clayton
| gclayton\@fb.com (email), clayborg (Phabricator), clayborg (GitHub)
| Jason Molenda
| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub)
PDB
~~~
| Zequan Wu
| zequanwu\@google.com (email), zequanwu (Phabricator), ZequanWu (GitHub)
Platforms
---------
The following people are responsible for decisions involving platforms.
Android
~~~~~~~
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
Darwin
~~~~~~
| Jim Ingham
| jingham\@apple.com (email), jingham (Phabricator), jimingham (GitHub)
| Jason Molenda
| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub)
| Jonas Devlieghere
| jonas\@devlieghere.com (email), jdevlieghere (Phabricator), jdevlieghere (GitHub)
FreeBSD
~~~~~~~
| Ed Maste
| emaste\@freebsd.org (email), emaste (Phabricator), emaste (GitHub)
Linux
~~~~~
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
| David Spickett
| david.spickett\@linaro.org (email), DavidSpickett (Phabricator), DavidSpickett (GitHub)
Windows
~~~~~~~
| Omair Javaid
| omair.javaid\@linaro.org (email), omjavaid (Phabricator), omjavaid (GitHub)
Tools
DavidSpickettUnsubmitted
Done
ReplyInline Actions

Is the extra space an RST thing, seems placed randomly.

DavidSpickett: Is the extra space an RST thing, seems placed randomly.
JDevlieghereAuthorUnsubmitted
Done
ReplyInline Actions

Nope that's just me: I'll clear that up 👍

JDevlieghere: Nope that's just me: I'll clear that up 👍
-----
The following people are responsible for decisions involving specific tools.
debugserver
~~~~~~~~~~~
| Jason Molenda
labathUnsubmitted
Done
ReplyInline Actions

@DavidSpickett

labath: @DavidSpickett
DavidSpickettUnsubmitted
Done
ReplyInline Actions

Ok with me.

DavidSpickett: Ok with me.
| jmolenda\@apple.com (email), jasonmolenda (Phabricator), jasonmolenda (GitHub)
lldb-server
~~~~~~~~~~~
| Pavel Labath
| pavel\@labath.sk (email), labath (Phabricator), labath (GitHub)
lldb-vscode
~~~~~~~~~~~
| Greg Clayton
| gclayton\@fb.com (email), clayborg (Phabricator), clayborg (GitHub)
| Walter Erquinigo
| a20012251\@gmail.com (email), wallace (Phabricator), walter-erquinigo (GitHub)
Former Code Owners
==================
The following people have graciously spent time performing code ownership
responsibilities but are no longer active in that role. Thank you for all your
help with the success of the project!
| Kamil Rytarowski (kamil\@netbsd.org)
| Zachary Turner (zturner\@google.com)