r205728 - Thread Safety Analysis: update to internal SExpr handling.

[Aaron Ballman]

I've attached some proposed changes. :-) More comments below.

On Tue, Apr 8, 2014 at 6:36 PM, Delesley Hutchins <delesley at google.com> wrote:
> Thanks for the long and detailed review!  Responses are below, fixes
> are in r205809.
>
>> It seems like a whole lot of these could be forward declares.
>
> This is only included in one spot, so it doesn't matter all that much,
> but I've tried to trim down the list.  :-)
>
>>> +// Simple Visitor class for traversing a clang CFG.
>>> +class CFGVisitor {
>>> ...
>>> +};
>>
>> These all look very much like they should be virtual methods.
>
> No.  The traversal routine is templatized to avoid the overhead of
> virtual calls.

That means this entire class is mostly for documentation. In the
attached patch, I simply commented it out and explained it a bit
further.

>
>> nullptr instead of 0?
>
> Fixed, here and elsewhere.
>
>>> +  ~CFGWalker() { }
>>
>> Why is this required?
>
> It's not.  Deleted.
>
>>> +  // Initialize the CFGWalker.  This setup only needs to be done once, even
>>> +  // if there are multiple passes over the CFG.
>>> +  bool init(AnalysisDeclContext &AC) {
>>
>> Why is this not a constructor when it only needs to be done once?
>
> I want to decouple construction and initialization here.
>
>>> +      for (CFGBlock::const_iterator BI = CurrBlock->begin(),
>>> +                                    BE = CurrBlock->end();
>>> +           BI != BE; ++BI) {
>>
>> Range-based for loop?
>
> Done.
>
>> Getting rid of these const_casts would be wonderful, if possible.
>
> I agree, but it's not possible in this case.

My attach patch managed it (but perhaps that's just because I was
testing with MSVC).

>
>> Why are these all public?
>
> Because of the way things are hooked together right now.  Added TODO;
> will fix later.
>
>> It seems like a lot of these methods shouldn't be public.
>
> Switched to protected.
>
>> Why protected?
>
> So it's possible to create derived builders.
>
>>> +// Dump an SCFG to llvm::errs().
>>> +void printSCFG(CFGWalker &walker);
>>
>> This seems like it'd be more reasonable as a dump() method on
>> CFGWalker than a stand-alone function.
>
> I disagree.  CFGWalker is a tool to walk a clang::CFG, and is designed
> for multiple passes with different visitors.  printSCFG constructs and
> prints a til::SCFG.  It is mainly for testing purposes, and should not
> be part of the public interface of CFGWalker.

Agreed.

>
>> Everything in this file is in the til namespace, yet it has an odd
>> mixture of naming conventions used. Eg) til::TILPrettyPrinter,
>
> Good point.  I fixed most of these.  I kept the opcode ones, because I
> like green bike sheds.  :-)

Okay, fine, you can have a green bikeshed. ;-)

>
>> Can an existing container class suffice? I'm not keen on adding
>> another container data type, much less exposing it like this.
>
> I will switch to a different container in a future patch.
>
>> You're kind of missing a virtual destructor here. Future inherits from
>> SExpr and exposes virtual functions, so polymorphic deletion of a
>> Future through a static type of SExpr would be problematic.
>
> No, I very much do not want a virtual method table, because SExprs are
> supposed to be very small, bump-allocated classes, and I have gone to
> great lengths to avoid the extra overhead.  Moreover, because they use
> bump-allocation, SExpr destructors are never called.  Future is the
> only class which has a virtual method, and where I'm willing to pay
> the memory penalty for having it.

That's kind of what I figured, but what's strange to me is that SExpr
(and subclasses) seem to always be created in the Arena -- so the
destructors aren't being called anyway (I didn't see any explicit
destructor calls happening). That's why my proposed patch tries to
make this more clear -- you must create them in an arena, and you
cannot delete them.

So there's no extra overhead cost, but it's a bit more abuse-resistant.

>
>> This isn't a typical pattern used in the code base, and it makes it
>> harder to write const-correct code.
>
> Already fixed in my last patch.
>
>> expr() instead of clangExpr()? A bit of a bikeshed, but the name is
>> just kind of odd to me.
>
> No.  expr() in other classes returns an SExpr, whereas this method
> links back to clang::Expr.

Fair enough.

>
>> decl() instead of clangDecl() for the same reasoning as above?
>
> No.  Same reason as before.  The TIL is a separate language, so any
> links back to clang are spelled out for clarity.
>
>> Why _x?
>
> The default name for an unnamed variable is always 'x', but there
> might be real variables with that name, so I add a leading underscore.
>  :-)
>
>> Should there be an assert that NumUses >= 0 when detaching (to catch
>> mismatches)?
>
> Good idea.  Added.
>
>> Seems like NumUses should be unsigned.
>
> Done.  Now that there's an assert, I'm more comfortable with that decision.  :-)
>
>> I would really rather not see a constructor which steals memory from
>> something else -- could it be a factory function instead, so it's at
>> least a little more readily apparent to callers?
>
> I've changed these to use move semantics for clarity.  (As you have
> noticed, my first patch had very little C++11.)  A factory method
> would break the pattern used by all of the other constructors.

Shouldn't that be defined as a swap (since right now, it's just
copying the pointer)?

SExprRef(SExprRef &&R) { std::swap(Ptr, R.Ptr); }

>
>> This would be more clear with reverse iterators (until we get a
>> reverse range adapter).
>
> Added TODO.
>
>>> +  void setArena(MemRegionRef A) { Arena = A; }
>>
>> This seems like it could be used as a constructor parameter instead.
>
> Can't, without making it a constructor parameter of Traversal, which
> is intended for use with classes that don't involve MemRegions.

Makes sense.

>
>>> +// Basic class for comparison operations over expressions.
>>> +template <typename Self>
>>> +class TILComparator {
>>> +public:
>>> +  Self *self() { return reinterpret_cast<Self *>(this); }
>>
>> Does this need to be public?
>
> Changed to protected.
>
>> Why not private?
>
> For future extension.  There is more than one way to pretty print an expression.
>
>> Why not an enumeration instead?
>
> Mainly because I'm doing arithmetic with them.
>
>> These two lines can be removed entirely instead of left commented out.
>
> Except that I constantly uncomment them for testing.  :-)
>
>> Seems like CallExpr could use an arguments() range function that could
>> be used here.  ;-)
>
> True.  :-)  As could several CFG routines.
>
>>> +// Build a complete SCFG from a clang CFG.
>>> +class SCFGBuilder : public CFGVisitor {
>>> +public:
>>> +  // return true if E should be included in the SCFG
>>> +  bool includeExpr(til::SExpr* E) {
>>
>> This should be private. That goes for other members of this class as well.
>
> Done.
>
>> These are being leaked. unique_ptr perhaps?
>
> Nice catch!  Thanks!  Fixed.
>
>> All of these blocks wind up being leaked.
>
> Thanks again.  Fixed.

~Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ThreadSafety.patch
Type: application/octet-stream
Size: 13001 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140409/34c04fd9/attachment.obj>