Indeed, if the code had altered the extant bits of the Host/Platform interfaces, which are a current source of activity amongst many of us, then putting it out for review would have been strongly recommended. Greg is about the only one of us that could really be considered an "expert" on this stuff - though you're probably getting close - and most of us just dabble here...
Jim > > On Dec 5, 2014, at 9:43 AM, Zachary Turner <ztur...@google.com> wrote: > > Fair enough, most of my concerns had been alleviated after glancing over the > patches and seeing that there were little to no system calls in any of them, > which was my biggest concern. In any case, I already fixed up the build > breakages, they were all trivial fixes as you all predicted. > > On Thu Dec 04 2014 at 6:19:18 PM <jing...@apple.com> wrote: > The llvm policies for review include "review after commit" when the code is > in an area that is owned by the committer: > > • Code can be reviewed either before it is committed or after. We > expect major changes to be reviewed before being committed, but smaller > changes (or changes where the developer owns the component) can be reviewed > after commit. > > lldb doesn't have many subsystems with lots of folks who work on that area. > After all, there aren't that many people working on lldb, and as a result we > all tend to work on our own separate areas. So if you are contributing to > your area of expertise, waiting for a "I don't understand how that code > works, but it looks formally correct, or compiles for me" seems like it will > slow down development without any real benefits. > > At some point the areas of code in lldb which at present only one or two > people understand and work on will have many active contributors. When that > happens, it will be worth the delay to get the opinions of those other future > contributors. But for now, in the areas that the current contributors own I > think it is fine that we do review after commit. > > Jim > > > > On Dec 4, 2014, at 5:51 PM, Zachary Turner <ztur...@google.com> wrote: > > > > Well, I haven't had a chance to try out the changes yet, so I guess I'll > > reserve judgement for tomorrow. Ultimately if everything works, no harm no > > foul. Although as I mentioned in the earlier resposne, we don't have > > Windows buildbots yet. > > > > In any case, I would love to see a more active code reviewing culture in > > LLDB someday. They're a source of great benefit for everyone. > > > > On Thu Dec 04 2014 at 5:36:19 PM Sean Callanan <scalla...@apple.com> wrote: > > Zachary, > > > > It’s not clear to me that this is actually a bigger deal for non-OS X > > platforms than the IOHandler change. > > For one thing, unless a Platform specifically signs up for this, it does > > nothing. > > The idea is specifically to start this off on OS X and then have other > > platforms adopt it as they are ready. To that end, I sent a summary of > > what’s going in as an e-mail for people to read as they look at the commits. > > > > The only risk here is that we’ll break someone’s build in a way we didn’t > > anticipate – which is the point of buildbots. > > > > Sean > > > >> On Dec 4, 2014, at 5:33 PM, Zachary Turner <ztur...@google.com> wrote: > >> > >> Yes, but for starters we dont have a windows build bot yet (ive been > >> working on that), and secondly buildbots shouldnt be used as a way to > >> bypass the review process. > >> > >> For example, Kates change to IOHandler recently is small compared to this > >> one, but it still was looked at by people prior to comitting. > >> > >> It's possible these changes won't break anything, but still it doesn't > >> seem unreasonable to ask that particularly huge changes like this at least > >> have a day or two of being made public before going in. > >> On Thu, Dec 4, 2014 at 5:25 PM <jing...@apple.com> wrote: > >> I'm confused. Isn't that what the build bots are for? Seems better to > >> rely on the immediate feedback you get from build bots for "other host" > >> breakages than waiting around to see if anybody is going to grab your > >> patch and apply it and try to build, etc... > >> > >> Jim > >> > >> > On Dec 4, 2014, at 5:18 PM, Zachary Turner <ztur...@google.com> wrote: > >> > > >> > Would it be possible to put patches up for review for at least a couple > >> > days BEFORE comitting? There's a pretty good chance this will break a > >> > build somewhere, and an average chance that the break won't be that easy > >> > to fix. And having broken builds for a couple of days doesn't seem > >> > reasonable. > >> > > >> > Im not sure how thoroughly we can review it, but at least we can prevent > >> > broken builds this way. > >> > On Thu, Dec 4, 2014 at 5:10 PM Sean Callanan <scalla...@apple.com> wrote: > >> > Rationale > >> > > >> > A historical pain point in the expression parser has been using > >> > functions for which the type is not completely known from debug > >> > information. > >> > This is the case e.g. for functions from the C standard library, and > >> > also for Objective-C methods in frameworks. > >> > It’s also unfortunate when handy types, enums, etc. are invisible just > >> > because your DWARF doesn’t happen to contain them. > >> > I’m doing something about that, for OS X – but it should generalize to > >> > other platforms. > >> > > >> > Clang modules: the basics > >> > > >> > The code I’m about to commit adds support for Clang modules to the > >> > expression parser. Clang modules are described in much more detail here: > >> > http://clang.llvm.org/docs/Modules.html > >> > but here is a short introduction: > >> > > >> > A group of header files is encapsulated in a module, which is provided > >> > with a module.map file. > >> > On OS X, this module.map is typically inside the corresponding framework. > >> > Clang reads module.map and the corresponding headers and produces a > >> > compiled module. > >> > This compiled module is essentially a .pch; it provides full information > >> > about all APIs, types, etc. defined in the module. > >> > > >> > LLDB support > >> > > >> > In order to use Clang modules, LLDB must: > >> > > >> > • Know where they live (from Clang’s perspective, this is the > >> > “sysroot”) and what compilation flags to use when parsing them; > >> > • Know which ones the user wants; > >> > • Compile them with its built-in Clang instance; and > >> > • Use the information found in the compiled modules as appropriate. > >> > > >> > LLDB accomplishes each of these goals in the following ways: > >> > > >> > • Platforms can now return the appropriate Clang flags that tell > >> > the module importer where to find modules for the current platform. > >> > Explicit support is enabled in PlatformDarwin. Other platforms can opt > >> > into this by returning true from Platform::SupportsModules() and adding > >> > the appropriate compilation options when > >> > Platform::AddClangModuleCompilationOptions() is called. If they return > >> > false, there should be no change in behavior. > >> > • LLDB adds preprocessor callbacks to Clang that catch @import > >> > directives. When such a directive is found, LLDB directs its built-in > >> > Clang to import the named module. > >> > • Modules are imported into a separate compiler instance (with its > >> > own AST context) encapsulated in ClangModulesDeclVendor so that we can > >> > be careful about what we actually import into expressions. Information > >> > in DWARF will often take precedence, for instance. > >> > • ClangExpressionDeclMap – the code responsible for finding > >> > entities Clang asks about while it parses expressions – is being > >> > extended to load information from modules as appropriate. The initial > >> > commit simply searches for functions (e.g., printf()) but I will be > >> > adding functionality rapidly. > >> > > >> > What the upcoming patches do > >> > > >> > • ClangModulesDeclVendor.h/.cpp implements the portions of LLDB > >> > responsible for driving the modules compiler. This should be > >> > platform-generic code, although in practice we may need to tweak it to > >> > make sure it is flexible enough to handle everything. > >> > • TypeVendor has been changed to DeclVendor, so that the > >> > Objective-C runtime can share the same method signatures with the Clang > >> > module importer. Places that used TypeVendor now use DeclVendor, and > >> > I’ve tweaked the APIs for getting types from decls to make this > >> > transition smooth. > >> > • Platform (as mentioned above) now can return the flags necessary > >> > to tell Clang where modules live. PlatformDarwin has a bunch of new > >> > code to find these; I also have a default implementation that you can > >> > try out if you want, but unless you know what a module.map is I would > >> > hold off on this. > >> > • The LLDB bundle on Mac OS X will now also include the > >> > compiler-specific headers Clang requires to compile standard library > >> > headers (e.g., tgmath.h, stdarg.h). Host can find the location of these > >> > headers; on non-OS X hosts, we’ll need to put them in some sensible > >> > place. > >> > • ClangExpressionParser.cpp now sets up the appropriate context to > >> > intercept @import directives. > >> > • ClangExpressionDeclMap.cpp now searches modules (if available) > >> > for functions if it doesn’t find them in DWARF. > >> > • Targets now vend their ClangModulesDeclVendors as appropriate. > >> > > >> > Timeline and priorities > >> > > >> > I’m going to start committing today, but if any of these commits breaks > >> > anything for you, please let me know. > >> > My priority is getting this working on OS X first. If parts of my code > >> > look platform-myopic, please let me know, though – I really want to see > >> > debugging with modules working on other platforms too. > >> > Of course if any of this breaks any build, let me know immediately and > >> > I’ll get on it. > >> > _______________________________________________ > >> > lldb-dev mailing list > >> > lldb-dev@cs.uiuc.edu > >> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > >> > _______________________________________________ > >> > lldb-dev mailing list > >> > lldb-dev@cs.uiuc.edu > >> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > >> > > _______________________________________________ lldb-dev mailing list lldb-dev@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
