[cfe-dev] [analyzer] How to walk AST template instantiations

[Ben Craig via cfe-dev]

I think with templates, there are going to be significant trade-offs no matter what.  I think this is one of the reasons this makes more sense as a checker rather than a compiler warning.

 

My current scheme certainly has the “contradictory warning” problem that you mention, but it’s a genuine padding issue at least, even if the fix isn’t easy.

 

I can see two major approaches for basing padding warnings on the template declaration.  I can assume some alignment for dependent fields, or I can pretend the field  isn’t there.  With an assumed alignment, I can both over-diagnose and under-diagnose, depending on my assumption.  If I pretend the dependent field doesn’t exist, then I will generally under-diagnose.

 

I still haven’t gotten to the point where I’m ready to point this analyzer at a significant code base (i.e. clang+llvm).  I expect I’ll find other exceptions or pain points then, and I’ll iterate.  Perhaps I’ll see a million warnings about std::tuple and reconsider my stance :)

 

Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

 

From: David Blaikie [mailto:dblaikie at gmail.com] 
Sent: Monday, November 02, 2015 1:17 PM
To: Ben Craig
Cc: Clang Dev
Subject: Re: [cfe-dev] [analyzer] How to walk AST template instantiations

 

I'm not sure how to do what you need - but is it the right thing to do?

It would seem problematic to visit template instantiations for a warning about sturct layout - you could end up giving two contradictory warnings in two different instantiations? (or otherwise oscillate - one instantiation with a large type & suggests moving it to the beginning, another with a small type which suggests moving it to the middle, etc)


It seems to me that working on the template pattern (so you can account for the ambiguity of dependent-ly typed members) would be the safer diagnostic, no?

 

On Mon, Nov 2, 2015 at 11:11 AM, Ben Craig via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org> > wrote:

My eventual question:

What is the best way for analysis AST visitors to visit template instantiations?

 

Background:

I am working on a checker that will report about excessive padding.  Basically, any place where padding could be reduced by reordering data members, I want to emit a report.  This is different from –Wpadding because I don’t want to report on items where there isn’t a relatively easy fix.

 

The general way I’m implementing this is by using the check::ASTDecl<RecordDecl> visitor, returning early for some really troublesome cases, then using getASTRecordLayout.  I then compare the actual size and padding against a computed optimal padding and size, and report when the observed padding doesn’t meet the optimal padding.

 

I started writing tests for this, and started to run into problems with C++ templates.  Here is the most troublesome code test case I’ve hit so far:

template <typename T>

struct Foo {

  // expected-warning at +1{{Excessive padding in struct 'Foo<int>::Nested'}}

  struct Nested { // doesn’t actually warn though…

    char c1;

    T t;

    char c2;

  };

};

 

struct Holder { //no-warning

  Foo<int>::Nested t1;

  Foo<char>::Nested t2;

};

 

It seems that by default, AnalysisConsumer doesn’t visit template instantiations.  In fact, if I add “bool shouldVisitTemplateInstantiations() const { return true; }” to the AnalysisConsumer class, I get the reports I expect.  Is there a better way for me to accomplish my goal?  I don’t like the idea of turning on instantiation visitation for every checker just because my checker needs it.

 

Before I found AnalysisConsumer, I did make an attempt to walk the instantiations by visiting ClassTemplateDecl and iterating over specializations.  That works ok if I don’t have nested Records, like I posted above.  I hadn’t found the “right” way to recurse into the structure before finding shouldVisitTemplateInsantiations.  Maybe a “better” approach is to basically copy RecursiveASTVisitor::TraverseClassInstantiation?  I’m not sure if I’m comfortable duplicating that code, but if it’s the better option then I will.

 

Thanks for the help,

Ben Craig

 

Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

 


_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org> 
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20151102/8910ae3e/attachment.html>