[PATCH] Template Instantiation Observer + a few other templight-related changes

[Sven Mikael Persson]

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.

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, and 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/