I’m glad to hear it. As Sean indicated, that was pretty much expected given the nature of the work – unlike the editline reworking which was bound to run into platform-specific issues.
Kate Stone k8st...@apple.com <mailto:k8st...@apple.com> Xcode Runtime Analysis Tools > 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 > <mailto: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 > > <mailto: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 > > <mailto: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 > >> <mailto: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 > >> <mailto: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 > >> > <mailto: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 > >> > <mailto: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 > >> > <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 <mailto:lldb-dev@cs.uiuc.edu> > >> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > >> > <http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev> > >> > _______________________________________________ > >> > lldb-dev mailing list > >> > lldb-dev@cs.uiuc.edu <mailto:lldb-dev@cs.uiuc.edu> > >> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > >> > <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
