[cfe-dev] [Templight] Templight "v2" with gdb-style debugger

[Nick Lewycky]

On 6 January 2015 at 15:03, Mikael Persson <mikael.s.persson at gmail.com>
wrote:

> Hi Nick,
>
> Glad to finally get a sign of life from the clang dev team!
>

I'm more on the LLVM side. While I'm willing to make some changes to clang
I'll sometimes conclude that I don't know an area of clang well enough to
approve a patch, on a case by case basis.

> The llvm project requires that patches be submitted by their author (or
> authors). If you grabbed the original templight code and refactored it,
> then it may be coauthored between you and the original authors. Our policy
> requires that an author submit their own code, not mail in someone else's
>
> The patches from this email are very out-dated (and I was completely
> unfamiliar with the process of patch submissions... well, I still am to be
> honest ;) ). I submitted a much smaller patch after successfully managing
> to extract all the templight code out of the clang source, to make it a
> separate tool instead, leaving only very minor "hooks" to be added to Clang
> itself. That patch can be found at:
>
> http://reviews.llvm.org/D5767
>
> As for the authorship, there are a few things to notice.
>
> First of all, in the new patch that I submitted, all of the code is my own
> code, because all of the original templight code has been essentially
> factored out from the original patch, i.e., the "real" templight code now
> sits in a separate project at:
>
> https://github.com/mikael-s-persson/templight
>
> There is also a more up-to-date clang patch on there, which I will
> re-submit eventually to Clang when there is more sign of life from the
> community (I don't like wasting my valuable time throwing patch-updates
> into a bottomless pit).
>
> The original patch also lacked any specific authorship notice and
> licensing. But I'm more than happy to add any co-author that is needed to
> satisfy your submission requirements. All I want is the patch to go
> through, and I think all other interested parties and original authors feel
> the same. And after all, we are only talking about of few dozen trivial
> lines of code here.
>
> And for the record, I'm a bit annoyed by the "mail in someone else's
> code", as it kind of belittles the massive refactoring work I did on the
> original patch, and the fact that the majority of what it is now is my own
> code.
>

My sincere apologies, I did not intend to belittle. I was only summarizing
what the policy says, not making any claim about the work you have done. If
you tell me that the code meets the requirements in the developer's policy,
that is enough for me.

> I think that ActiveTemplateInstantiation should be changed to a form of
> InstantiationRecord and held from the relevant AST nodes.
>
> Maybe so, I don't know, and frankly I don't care. Well, I do care for
> having good template instantiation code in Clang, but I cannot afford to
> care too much about design decisions that are not mine to take.
>

Ok. It matters to me due to code refactoring tools.

> The changes to ATI to pull it out of Sema and make it part of the AST are
> complicated enough, I think you should send that out as a first patch.
>
> I think there is a confusion here. I never pulled the ATI into the AST, I
> only pulled it out of the Sema class. It has literally just gone from being
> a nested class within Sema to being a class at global scope (clang
> namespace, of course!), that's all. There is no change whatsoever in the
> code except for a change in scope and header-file placement (to avoid
> including the Sema header, and just because nested classes are a bad idea
> (I come from Boost library development, the design guidelines are more
> strict over there)).
>

My mistake, I was thinking that you pulled it out of Sema-the-library, not
Sema-the-class. In particular, it still uses sema::TemplateDeductionInfo.

If you want to work on making ATIs a part of the AST, you're welcome to try
> / propose it. It is none of my concern, and in fact I would object to it
> (see below).
>
> > There's also template instantiation for different reasons which are not
> currently recorded (ie., due to overload resolution, due to virtual
> methods). Since that's a change to ATI, that would logically go next.
>
> You're right, the categories of template instantiations are too broad.
> That is something I would be interested in discussing and intend to bring
> up after my patch has gone through. The cases of overload resolution and
> virtual methods are not exactly the first ones that come to mind for me
> though. The additional categories I was going to propose are for
> differentiating class template instantiations, function declaration
> instantiations and function definition instantiations. The lack of
> categorization of those things currently cause templight traces to have
> ambiguous entries, which I would like to avoid.
>
> > I think we usually use "Callbacks" instead of "Observer", similar to
> PPCallbacks?
>
> Fine. When we get closer to getting this (D5767) patch through, that can
> easily be changed, no problem.
>
> > At a high level, I think that what we currently do in Sema by tracking
> template instantiations as a stack should be available in the AST after
> instantiation is complete.
>
> I'm not sure if this makes sense in terms of the AST, after all, the stack
> of template instantiations are compilation steps, not actual entities in
> the code. My understanding is that the AST is only there to record all the
> entities in the code. I see this as a case of semantic pollution. What
> other records of compilation steps are recorded in the AST?
>

Implicit casts, for one. Expressions in the C++ standard have associated
grammar therefore implicit casts are not expressions, even though we call
them ImplicitCastExpr in clang. We could remove them and have the consumers
of the AST imagine them as needed, saving memory and possibly compilation
time. Or possibly costing compile time, if removing them would make Clang's
own code more complicated or force us to check more conditions and
recompute properties more often.

Also, if the AST is to keep a record of all the template instantiations
> that occurred, this could significantly increase memory consumption (and
> thus, speed as well).
>

Yes. I am also concerned about this. We had the same concern about keeping
around source locations and macro expansions, but it turns out that clang
was able to make it work efficiently. Similar efficiency would be a
requirement, and no, I don't know how to do it. I'm assuming we would
figure it out.

It is a good thing that the compiler throws away this information because
> it would and could amount to a lot if it kept it all (I would say
> approximately 5-10% increase in memory consumption, from my experience).
> This would be a lot of passive overhead (and therefore, would have to be
> disabled by default, of course). I certainly would not want such overhead
> by default in any compiler that I use (and I'm not sure that people who use
> libclang would want this either).
>

Part of the question is whether we could use it to simplify code in Sema
itself. I don't think anyone wants it to be an optional feature, it should
either be efficient enough to be always-on, or not there at all.

> Then we could query the AST to ask why something was instantiated instead
> of tracing the act of instantiation. Is there any use case for templight
> where this wouldn't work?
>
> Yes, pretty much all of templight as a profiler would not work that way,
> unless the AST also held records of time- and memory- consumption during
> those instantiation steps. The AST is useful to the templight debugger and
> is also used in conjunction with the templight traces by the metashell
> utilities (that Zoltan linked to). The AST describes the code, the
> templight traces describe the compilation process, they are entirely
> orthogonal features (yet complementary), as far as I'm concerned.
>
> To me, it is important to stress that, although the debugging and
> interactive querying of the AST is a neat feature, the real
> "production-critical" feature here is the profiling. This is my primary
> interest, and those of my peers, i.e., the people who write production
> template-heavy code that need to optimize the compilation time and memory.
> Focusing too much on the debugging and AST querying aspects could impede
> that primary goal, as far as I'm concerned.
>
> And as Zoltan mentioned, there could be issues with the safe-mode and
> dealing with failures to instantiated the templates if we are going to rely
> on "successful" AST nodes to describe the compilation process.
>
> There are basically three use-case categories for templight (from most
> "novice" to most "professional"): learning, debugging and profiling.
> Learning and debugging imply that things might be failing a lot, and
> profiling imply that there is no need to understand the process but a need
> to identify performance bottlenecks. Your approach does not serve any of
> these domains particularly well.
>

This is really enlightening, thanks.

I'm convinced that my idea of moving template instantiation information
into the AST is orthogonal to the needs of templight. Also, I think the
observer you want to add is a fine idea and we should add that.

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150107/464ea715/attachment.html>