On Thu, Jan 8, 2015 at 11:03 AM, Sven Mikael Persson < [email protected]> wrote:
> Thanks for the feedback Nick! > > I fixed all the minor cosmetic issues you pointed out, and I also fixed a > few "more than 80 char" lines in the original diff. > > I changed the name "Observer" to "Callbacks" as requested in the mailing > list discussion: > > http://clang-developers.42468.n3.nabble.com/Templight-Templight-quot-v2-quot-with-gdb-style-debugger-td4038413.html > > Here the new diff (how do I replace the original diff with this one in > this llvm-reviews system? I'm new to this system). > > F330648: templight_clang_patch.diff <http://reviews.llvm.org/F330648> > > To your comments: > > > I think we'll want a way to test out this interface; it should probably > come with a command-line tool that has a very simple template observer that > prints out the callbacks as they're called, and a test which runs this tool > to exercise it. This might also be a FrontendAction in clang, similar to > -ast-dump, instead of a separate tool. (This is not a trivial amount of > work, you may want to wait for another clang developer to chime in before > spending time on it.) > > > That is essentially what I have done for the templight tool ( > https://github.com/mikael-s-persson/templight). I created a new > FrontendAction that creates the templight tracer / debugger as a > TemplateInstCallbacks. The templight tracer is essentially what you > describe, it dumps a trace of all the callbacks (plus a bit of optional > filtering and gathering / pretty-formatting of the information). If you > want me to distil it down to a more "raw" tracer that only dumps everything > to stdout, then that can easily be done. > > But there is one hick-up. If the idea is to use this as a kind of > unit-test for the tool, that is, to verify that the trace of callbacks are > "correct" (as in, later changes to clang should preserve the same trace), > then I think that is a great idea in principle. However, I see two problems > with that. > > First, it's hard to be sure that the current traces are "correct", as in, > maybe this patch has some loop-holes that miss some instantiations, or > maybe clang is currently incorrect in some ways that might potentially be > fixed in the future (at least, I have seen several "FIXME" notes all over > the template instantiation code of the Sema and Parser classes). > > Second, according to my tests so far, the traces produced by clang are > **non-deterministic**. Different runs on the same source code will produce > different trace files. They only differ in inconsequential ways (different > ordering of nested instantiations and memoizations). But the point is that > a simple comparison of the trace file with some reference trace file would > most certainly always fail, unless the test is really trivial. > I'm wondering where this non-determinism comes from. I would certain ephemeral computations inside clang to proceed non-deterministically (but are supposed to have a deterministic final result) due to ASLR and the pervasive use of pointers as keys. If you turn off ASLR, does the non-determinism go away? -- Sean Silva > > Therefore, the notion of what makes a trace "correct" for a given source > file is not well-defined, and might never be. But if such a notion could be > defined (for example, from the C++ standard), this could end up as a great > way to evaluate the correctness of Clang (e.g., "does clang instantiate > templates in a standard-compliant way?"). > > But this is far beyond the scope of this patch. > > > Alphabetize. > > > Fixed in new patch. > > > What about a callback when clang transitions from parsing the TU to > actOnEndOfTranslationUnit? Template instantiation is different before and > after we reach the end-of-TU. > > > Thanks for pointing that out. I'm not sure what impact this really has. > The initialize / finalize pair of callbacks bracket the entire TU parsing, > as far as I know. Therefore, all instantiations are captured by the trace, > again, AFAIK. After this comment, I know that there are some late implicit > instantiations being instantiated when EOF is reached, basically, right? > AFAIK, these are still captured by the callbacks. > > I don't know if there is any meaning to a callback at this point. If I > were debugging Clang, I might see some value in being able to distinguish > the instantiations happening before and after this point. But from the > perspective of profiling the template instantiations, I don't really need > to know this. In any case, I think that this can be the subject of a future > patch if a good concrete use-case / motivation is brought forward. And this > also raises the question (maybe more appropriate for cfe-dev mailing list) > of whether Clang developers would be interested in using this > TemplateInstCallbacks as a means to evaluate / debug the clang compiler > itself, which might motivate the addition of more callbacks in it. > > > Is there a guarantee that this is a template? Offhand it looks like this > could fire on any struct, couldn't it? > > > You are correct in that this callback (for memoizations) picks up a lot of > things that are not templates. I am very well aware of this issue and it > has been pointed out before, but there is really not a lot I can do to > change this. Firstly, memoizations are really important (which I am just > beginning to realize after making plans for a call-graph like visualization > of the instantiations). Secondly, much of the clang template instantiation > code is structured as a "check if instantiated already, if not, instantiate > it", which means that there are many many places in clang where some kind > of "check if instantiated already" logic exists and it varies greatly from > context to context. This makes it very hard to insert the memoization > callbacks in proper places. So, as far as I know, this particular > placements is not too bad because it picks up most, if not all, checks, and > generates noise that can be dealt with (I don't do it currently in > templight, but I plan to add that soon, an! > d this is logic that could go into the TemplateInstCallbacks if needed), > i.e., it's easier to filter out spurious callbacks than to miss some > important ones. > > But at the end of the day, the only way that this can be fixed is for > someone from the Clang development team who is very much familiar with the > complete template instantiation code to provide his/her hands-on assistance > for this. I am simply not in a position to be able to figure out how to > appropriately trace the memoizations reliably and without too much noise. > > > http://reviews.llvm.org/D5767 > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
