large patches vs incremental changes

Subprojects LLDB
1

I wanted to open a discussion about the policy regarding patch submission.

LLVM, and most of its other subprojects, have adopted a policy of making small, incremental changes, as outlined here [http://llvm.org/docs/DeveloperPolicy.html#making-a-major-change]. It seems that either we don’t have this on LLDB, or we do have it and it’s just not enforced.

I personally have a strong preference for the incremental approach. All the reasons are outlined at the above link, so I don’t think I need to repeat them. Is it possible to move towards this model with LLDB?

Additionally, I frequently see patches going in without review. What is LLDB’s policy on this? Are we’re ok with having broken code upstreamed for a short time on the condition that the committer is acting in good faith to fix it as soon as possible? For example, I’ve had a patch up for 6 days that I’ve pinged on a couple of times, with no traction. I’ve been waiting because I was under the impression that getting a review was a requirement. But sometimes I see patches that seem non-trivial getting pushed straight to the server without any kind of review. So I’m not sure if my understanding is correct. Either way, some clarification woudl be nice.

Thanks!

2

Additionally, I frequently see patches going in without review. What is
LLDB's policy on this?

FWIW, I would assume (from the discussions with Chris when LLDB first
joined the LLVM projects) that it has the same policy as LLVM here. Chris,
I don't actually see any clarification about this on the LLDB website,
could we get that in there? I'm somewhat concerned that there is
essentially *no* developer policy posted for LLDB.

However, I want to point out that in LLVM you might see many patches going
in "without review" because the code review takes place *post-commit*. I
suspect that LLDB is much the same here, and a substantial place for code
review to take place is post-commit rather than pre-commit. Using
post-commit review to maintain a reasonable development velocity for
long-standing contributors is (IMO) an important and good part of the LLVM
development process.

Are we're ok with having broken code upstreamed for a short time on the
condition that the committer is acting in good faith to fix it as soon as
possible?

I think this is a separate question. Build bots should *never* be left
broken, etc.

3

*This* may actually be a separate *separate* question. :slight_smile: LLDB's build
bots have been broken for a long time, AFAICT. Also the only build bot is
for Darwin. Understandable why that is, although it would be nice to get
ones for other platforms.

4

I wanted to open a discussion about the policy regarding patch submission.

LLVM, and most of its other subprojects, have adopted a policy of making small, incremental changes, as outlined here [http://llvm.org/docs/DeveloperPolicy.html#making-a-major-change]. It seems that either we don't have this on LLDB, or we do have it and it's just not enforced.

I personally have a strong preference for the incremental approach. All the reasons are outlined at the above link, so I don't think I need to repeat them. Is it possible to move towards this model with LLDB?

I'm of two minds about the "incremental approach". If I'm trying to make a structural change and I'm not sure how I'm going to do it, I want to try a bunch of stupid attempts without having to explain why this or that one seems good. Then by the time I've figured out what I want to do the patch is pretty much ready, and going back and breaking it into stages seems to me make-work. But that is my style, other people work differently. Since we're getting free work out of folks (more or less) I shy away from imposing my style on others. On the other hand, anything that is naturally separable should be separated out into separate patches.

Additionally, I frequently see patches going in without review. What is LLDB's policy on this? Are we're ok with having broken code upstreamed for a short time on the condition that the committer is acting in good faith to fix it as soon as possible? For example, I've had a patch up for 6 days that I've pinged on a couple of times, with no traction. I've been waiting because I was under the impression that getting a review was a requirement. But sometimes I see patches that seem non-trivial getting pushed straight to the server without any kind of review. So I'm not sure if my understanding is correct. Either way, some clarification woudl be nice.

Many of the "unreviewed patches" you see going in are folks committing to code that they are the direct authors of. In that case, gdb had a sometimes useful notion of "area owners" who could check in code to their areas without review. But if you want to go to areas you know less about then you should ask for guidance, and if you haven't yet submitted some critical mass of "good patches" then your patches should wait on approval. Some of that asking for guidance you don't see because for most of its life contributions to the core of lldb have come from folks who share a hallway. We should get better about reflecting that through the mailing lists.

My general impression from working on gdb for many years, and of watching the gcc & llvm compiler communities, is that compilers are a "cool" project (it seems every CS student has built a compiler at some point and even if they haven't they have opinions about them) and there's generally enough interest in adding patches, making changes, etc, and from enough competing directions, that you need to have some more or less strict rules to avoid chaos. Debuggers are much less cool, and the problem is less throttling change than getting folks to work on it at all. To that end I'd prefer being more laissez faire till events show the need.

Jim

5

FreeBSD has had a build bot for a long time, and it's usually green
although a couple of tests have been failing intermittently since the
switch to running the tests multithreaded.

The bot is linked from the LLDB www and is here:
http://llvm-amd64.freebsd.your.org/b/builders/lldb-amd64-freebsd

6

Aside from the coding standards being substantially different (which I’m not thrilled about), LLDB follows the same developer policies as the rest of LLVM.

-Chris

7

I wanted to open a discussion about the policy regarding patch submission.

LLVM, and most of its other subprojects, have adopted a policy of making small, incremental changes, as outlined here [http://llvm.org/docs/DeveloperPolicy.html#making-a-major-change]. It seems that either we don't have this on LLDB, or we do have it and it's just not enforced.

I personally have a strong preference for the incremental approach. All the reasons are outlined at the above link, so I don't think I need to repeat them. Is it possible to move towards this model with LLDB?

Incremental is the preferred approach just like LLVM.

Additionally, I frequently see patches going in without review. What is LLDB's policy on this?

It really depends on the people doing the committing. We have 5 main people on LLDB here at Apple and we all commit without review as we developed LLDB from the ground up and understand the architecture. We also internally will review each other's patches as they go in and ask each other to fix things, so feel free to ask questions and get clarifications as you see commits going in and you want to understand things.

For newer contributors we prefer to review patches for a while until we can trust that the patches going in make sense and follow the architecture and design patterns we have used.

Are we're ok with having broken code upstreamed for a short time on the condition that the committer is acting in good faith to fix it as soon as possible?

Yes, I am not expecting everyone to build for unix, MacOSX, and Windows. If anything breaks things should be fixed ASAP. Sometimes it is easy for people to fix any build issues that come up, and other times the fix is less obvious. For header inclusions and simple build breakages these should be fixed quickly without intervention. Ideally we need to have buildbots on all systems: MacOSX, FreeBSD, Linux, and Windows and anytime anything breaks the buildbots lets users know and things get fixed.

For example, I've had a patch up for 6 days that I've pinged on a couple of times, with no traction.

We see these patches and we need our experts in the field to review them and our local reviewers need to keep up better with this new traffic. We have some time constraints here at Apple that are keeping us busy, so we have a lot on our plate so please be patient.

I've been waiting because I was under the impression that getting a review was a requirement.

Again, for newer contributors we do prefer getting a review.

But sometimes I see patches that seem non-trivial getting pushed straight to the server without any kind of review. So I'm not sure if my understanding is correct. Either way, some clarification woudl be nice.

So long term contributors have the ability to commit with no review.

The areas of expertise are as follows:

Greg Clayton: DWARF parser, ObjectFile, SymbolFile, plug-in architecture, public API, types, overall design and architecture, ValueObject, Python, IOHandlers, debugserver, overall process debugging, ARM architectures, x86 architectures
Jim Ingham: Thread stepping plans, architecture, overall design and architecture, ValueObject, overall process debugging
Sean Callanan: expression parser, clang IR emulation, JIT, disassembler
Enrico Granata: type format, type summaries, type synthetic, data formatters, ValueObject, Python, Python bridging
Jason Molenda: stack backtracing, process queues, disassembler, ARM architectures, x86 architectures

Your latest patches are mostly on the expression parser side and I need Sean Callanan to comment on anything that goes into the expression parser, IR memory map. Sean has been really busy getting other stuff done and I have asked him to make time to get your patches reviewed. Sean is out today and will be back tomorrow.

I hope this helps clear things up a bit, let us know if you have any more questions.

Greg Clayton