Changeset 79970ed

Timestamp:

Aug 24, 2016, 12:36:33 PM (8 years ago)

Author:

Rob Schluntz <rschlunt@…>

Branches:

ADT, aaron-thesis, arm-eh, ast-experimental, cleanup-dtors, ctor, deferred_resn, demangler, enum, forall-pointer-decay, jacob/cs343-translation, jenkins-sandbox, master, new-ast, new-ast-unique-expr, new-env, no_list, persistent-indexer, pthread-emulation, qualifiedEnum, resolv-new, with_gc

Children:

0555f4b

Parents:

f87408e

Message:

implement warnings for missing struct member constructor calls, remove bad clones

Location:

src

Files:

• CodeGen/GenType.h (modified) (1 diff)
• Common/utility.h (modified) (2 diffs)
• InitTweak/FixInit.cc (modified) (7 diffs)
• InitTweak/InitTweak.cc (modified) (1 diff)
• InitTweak/InitTweak.h (modified) (1 diff)
• Parser/LinkageSpec.cc (modified) (7 diffs)
• ResolvExpr/AlternativeFinder.cc (modified) (5 diffs)
• SymTab/Validate.cc (modified) (2 diffs)
• SynTree/AggregateDecl.cc (modified) (1 diff)
• SynTree/Expression.cc (modified) (2 diffs)
• SynTree/TypeSubstitution.h (modified) (1 diff)

src/CodeGen/GenType.h

@@ -5 +5 @@
 // file "LICENCE" distributed with Cforall.
 //
-// GenType.h -- 
+// GenType.h --
 //
 // Author           : Richard C. Bilson

src/Common/utility.h

@@ -144 +144 @@
 
 template < typename T >
-std::string toString ( T value ) {
+void toString_single ( std::ostream & os, const T & value ) {
+        os << value;
+}
+
+template < typename T, typename... Params >
+void toString_single ( std::ostream & os, const T & value, const Params & ... params ) {
+        os << value;
+        toString_single( os, params ... );
+}
+
+template < typename ... Params >
+std::string toString ( const Params & ... params ) {
         std::ostringstream os;
-        os << value; // << std::ends;
+        toString_single( os, params... );
         return os.str();
 }
@@ -218 +229 @@
 }
 
+template< typename T >
+void warn_single( const T & arg ) {
+        std::cerr << arg << std::endl;
+}
+
+template< typename T, typename... Params >
+void warn_single(const T & arg, const Params & ... params ) {
+        std::cerr << arg;
+        warn_single( params... );
+}
+
+template< typename... Params >
+void warn( const Params & ... params ) {
+        std::cerr << "Warning: ";
+        warn_single( params... );
+}
+
 #endif // _UTILITY_H
 

src/InitTweak/FixInit.cc

@@ -33 +33 @@
 #include "GenPoly/PolyMutator.h"
 #include "SynTree/AddStmtVisitor.h"
+#include "CodeGen/GenType.h"  // for warnings
 
 bool ctordtorp = false;
@@ -174 +175 @@
                         virtual Expression * mutate( ImplicitCopyCtorExpr * impCpCtorExpr );
                 };
+
+                class WarnStructMembers : public Visitor {
+                  public:
+                        typedef Visitor Parent;
+                        /// warn if a user-defined constructor or destructor is missing calls for
+                        /// a struct member or if a member is used before constructed
+                        static void warnings( std::list< Declaration * > & translationUnit );
+
+                        virtual void visit( FunctionDecl * funcDecl );
+
+                        virtual void visit( MemberExpr * memberExpr );
+                        virtual void visit( ApplicationExpr * appExpr );
+
+                  private:
+                        void handleFirstParam( Expression * firstParam );
+
+                        FunctionDecl * function = 0;
+                        std::set< DeclarationWithType * > unhandled;
+                        ObjectDecl * thisParam = 0;
+                };
         } // namespace
 
@@ -187 +208 @@
                 // FixCopyCtors must happen after FixInit, so that destructors are placed correctly
                 FixCopyCtors::fixCopyCtors( translationUnit );
+
+                WarnStructMembers::warnings( translationUnit );
         }
 
@@ -231 +254 @@
                 }
 
+                void WarnStructMembers::warnings( std::list< Declaration * > & translationUnit ) {
+                        if ( true ) { // fix this condition to skip this pass if warnings aren't enabled
+                                WarnStructMembers warner;
+                                acceptAll( translationUnit, warner );
+                        }
+                }
+
                 Expression * InsertImplicitCalls::mutate( ApplicationExpr * appExpr ) {
                         appExpr = dynamic_cast< ApplicationExpr * >( Mutator::mutate( appExpr ) );
@@ -242 +272 @@
                                         FunctionType * ftype = dynamic_cast< FunctionType * >( GenPoly::getFunctionType( funcDecl->get_type() ) );
                                         assert( ftype );
-                                        if ( (funcDecl->get_name() == "?{}" || funcDecl->get_name() == "?=?") && ftype->get_parameters().size() == 2 ) {
+                                        if ( (isConstructor( funcDecl->get_name() ) || funcDecl->get_name() == "?=?") && ftype->get_parameters().size() == 2 ) {
                                                 Type * t1 = ftype->get_parameters().front()->get_type();
                                                 Type * t2 = ftype->get_parameters().back()->get_type();
@@ -253 +283 @@
                                                         return appExpr;
                                                 } // if
-                                        } else if ( funcDecl->get_name() == "^?{}" ) {
+                                        } else if ( isDestructor( funcDecl->get_name() ) ) {
                                                 // correctness: never copy construct arguments to a destructor
                                                 return appExpr;
@@ -670 +700 @@
                         } // switch
                 }
+
+                bool checkWarnings( FunctionDecl * funcDecl ) {
+                        // only check for warnings if the current function is a user-defined
+                        // constructor or destructor
+                        if ( ! funcDecl ) return false;
+                        if ( ! funcDecl->get_statements() ) return false;
+                        return isCtorDtor( funcDecl->get_name() ) && ! LinkageSpec::isOverridable( funcDecl->get_linkage() );
+                }
+
+                void WarnStructMembers::visit( FunctionDecl * funcDecl ) {
+                        WarnStructMembers old = *this;
+                        *this = WarnStructMembers();
+
+                        function = funcDecl;
+                        if ( checkWarnings( funcDecl ) ) {
+                                FunctionType * type = funcDecl->get_functionType();
+                                assert( ! type->get_parameters().empty() );
+                                thisParam = safe_dynamic_cast< ObjectDecl * >( type->get_parameters().front() );
+                                PointerType * ptrType = safe_dynamic_cast< PointerType * > ( thisParam->get_type() );
+                                StructInstType * structType = dynamic_cast< StructInstType * >( ptrType->get_base() );
+                                if ( structType ) {
+                                        StructDecl * structDecl = structType->get_baseStruct();
+                                        for ( Declaration * member : structDecl->get_members() ) {
+                                                if ( ObjectDecl * field = dynamic_cast< ObjectDecl * >( member ) ) {
+                                                        // record all of the struct type's members that need to be constructed or
+                                                        // destructed by the end of the function
+                                                        unhandled.insert( field );
+                                                }
+                                        }
+                                }
+                        }
+                        Parent::visit( funcDecl );
+
+                        for ( DeclarationWithType * member : unhandled ) {
+                                // emit a warning for each unhandled member
+                                warn( "in ", CodeGen::genType( function->get_functionType(), function->get_name() ), ", member ", member->get_name(), " may not have been ", isConstructor( funcDecl->get_name() ) ? "constructed" : "destructed" );
+                        }
+
+                        *this = old;
+                }
+
+                void WarnStructMembers::visit( ApplicationExpr * appExpr ) {
+                        if ( ! checkWarnings( function ) ) return;
+
+                        std::string fname = getFunctionName( appExpr );
+                        if ( fname == function->get_name() ) {
+                                // call to same kind of function
+                                Expression * firstParam = appExpr->get_args().front();
+
+                                if ( VariableExpr * varExpr = dynamic_cast< VariableExpr * >( firstParam ) ) {
+                                        // if calling another constructor on thisParam, assume that function handles
+                                        // all members - if it doesn't a warning will appear in that function.
+                                        if ( varExpr->get_var() == thisParam ) {
+                                                unhandled.clear();
+                                        }
+                                } else {
+                                        // if first parameter is a member expression then
+                                        // remove the member from unhandled set.
+                                        handleFirstParam( firstParam );
+                                }
+                        } else if ( fname == "?=?" && isIntrinsicCallExpr( appExpr ) ) {
+                                // forgive use of intrinsic assignment to construct, since instrinsic constructors
+                                // codegen as assignment anyway.
+                                assert( appExpr->get_args().size() == 2 );
+                                handleFirstParam( appExpr->get_args().front() );
+                        }
+
+                        Parent::visit( appExpr );
+                }
+
+                void WarnStructMembers::handleFirstParam( Expression * firstParam ) {
+                        using namespace std;
+                        if ( AddressExpr * addrExpr = dynamic_cast< AddressExpr * >( firstParam ) ) {
+                                if ( MemberExpr * memberExpr = dynamic_cast< MemberExpr * >( addrExpr->get_arg() ) ) {
+                                        if ( ApplicationExpr * deref = dynamic_cast< ApplicationExpr * >( memberExpr->get_aggregate() ) ) {
+                                                if ( getFunctionName( deref ) == "*?" && deref->get_args().size() == 1 ) {
+                                                        if ( VariableExpr * varExpr = dynamic_cast< VariableExpr * >( deref->get_args().front() ) ) {
+                                                                if ( varExpr->get_var() == thisParam ) {
+                                                                        unhandled.erase( memberExpr->get_member() );
+                                                                }
+                                                        }
+                                                }
+                                        }
+                                }
+                        }
+                }
+
+                void WarnStructMembers::visit( MemberExpr * memberExpr ) {
+                        if ( ! checkWarnings( function ) ) return;
+                        if ( ! isConstructor( function->get_name() ) );
+
+                        if ( ApplicationExpr * deref = dynamic_cast< ApplicationExpr * >( memberExpr->get_aggregate() ) ) {
+                                if ( getFunctionName( deref ) == "*?" && deref->get_args().size() == 1 ) {
+                                        if ( VariableExpr * varExpr = dynamic_cast< VariableExpr * >( deref->get_args().front() ) ) {
+                                                if ( varExpr->get_var() == thisParam ) {
+                                                        if ( unhandled.count( memberExpr->get_member() ) ) {
+                                                                // emit a warning because a member was used before it was constructed
+                                                                warn( "in ", CodeGen::genType( function->get_functionType(), function->get_name() ), ", member ", memberExpr->get_member()->get_name(), " used before being constructed" );
+                                                        }
+                                                }
+                                        }
+                                }
+                        }
+                        Parent::visit( memberExpr );
+                }
         } // namespace
 } // namespace InitTweak

src/InitTweak/InitTweak.cc

@@ -433 +433 @@
         }
 
+        bool isConstructor( const std::string & str ) { return str == "?{}"; }
+        bool isDestructor( const std::string & str ) { return str == "^?{}"; }
+        bool isCtorDtor( const std::string & str ) { return isConstructor( str ) || isDestructor( str ); }
 }

src/InitTweak/InitTweak.h

@@ -26 +26 @@
 // helper functions for initialization
 namespace InitTweak {
+        bool isConstructor( const std::string & );
+        bool isDestructor( const std::string & );
+        bool isCtorDtor( const std::string & );
+
         /// transform Initializer into an argument list that can be passed to a call expression
         std::list< Expression * > makeInitList( Initializer * init );

src/Parser/LinkageSpec.cc

@@ -5 +5 @@
 // file "LICENCE" distributed with Cforall.
 //
-// LinkageSpec.cc -- 
-// 
+// LinkageSpec.cc --
+//
 // Author           : Rodolfo G. Esteves
 // Created On       : Sat May 16 13:22:09 2015
@@ -12 +12 @@
 // Last Modified On : Sun Aug 21 12:32:53 2016
 // Update Count     : 17
-// 
+//
 
 #include <string>
@@ -32 +32 @@
 
 std::string LinkageSpec::toString( LinkageSpec::Spec linkage ) {
+        assert( linkage >= 0 && linkage < LinkageSpec::NoOfSpecs );
         static const char *linkageKinds[LinkageSpec::NoOfSpecs] = {
                 "intrinsic", "Cforall", "C", "automatically generated", "compiler built-in",
@@ -39 +40 @@
 
 bool LinkageSpec::isDecoratable( Spec spec ) {
+        assert( spec >= 0 && spec < LinkageSpec::NoOfSpecs );
         static bool decoratable[LinkageSpec::NoOfSpecs] = {
                 //      Intrinsic,      Cforall,        C,              AutoGen,        Compiler
-                        true,           true,           false,  true,           false, 
+                        true,           true,           false,  true,           false,
         };
         return decoratable[spec];
@@ -47 +49 @@
 
 bool LinkageSpec::isGeneratable( Spec spec ) {
+        assert( spec >= 0 && spec < LinkageSpec::NoOfSpecs );
         static bool generatable[LinkageSpec::NoOfSpecs] = {
                 //      Intrinsic,      Cforall,        C,              AutoGen,        Compiler
-                        true,           true,           true,   true,           false, 
+                        true,           true,           true,   true,           false,
         };
         return generatable[spec];
@@ -55 +58 @@
 
 bool LinkageSpec::isOverridable( Spec spec ) {
+        assert( spec >= 0 && spec < LinkageSpec::NoOfSpecs );
         static bool overridable[LinkageSpec::NoOfSpecs] = {
                 //      Intrinsic,      Cforall,        C,              AutoGen,        Compiler
-                        true,           false,          false,  true,           false, 
+                        true,           false,          false,  true,           false,
         };
         return overridable[spec];
@@ -63 +67 @@
 
 bool LinkageSpec::isBuiltin( Spec spec ) {
+        assert( spec >= 0 && spec < LinkageSpec::NoOfSpecs );
         static bool builtin[LinkageSpec::NoOfSpecs] = {
                 //      Intrinsic,      Cforall,        C,              AutoGen,        Compiler
-                        true,           false,          false,  false,          true, 
+                        true,           false,          false,  false,          true,
         };
         return builtin[spec];

src/ResolvExpr/AlternativeFinder.cc

@@ -244 +244 @@
                 for ( std::list< Declaration* >::const_iterator i = members.begin(); i != members.end(); ++i ) {
                         if ( DeclarationWithType *dwt = dynamic_cast< DeclarationWithType* >( *i ) ) {
-                                alternatives.push_back( Alternative( new MemberExpr( dwt->clone(), expr->clone() ), env, newCost ) );
+                                alternatives.push_back( Alternative( new MemberExpr( dwt, expr->clone() ), env, newCost ) );
                                 renameTypes( alternatives.back().expr );
                         } else {
@@ -418 +418 @@
         // /// Map of declaration uniqueIds (intended to be the assertions in an AssertionSet) to their parents and the number of times they've been included
         //typedef std::unordered_map< UniqueId, std::unordered_map< UniqueId, unsigned > > AssertionParentSet;
-        
+
         static const int recursionLimit = /*10*/ 4;  ///< Limit to depth of recursion satisfaction
         //static const unsigned recursionParentLimit = 1;  ///< Limit to the number of times an assertion can recursively use itself
@@ -429 +429 @@
                 }
         }
-        
+
         template< typename ForwardIterator, typename OutputIterator >
-        void inferRecursive( ForwardIterator begin, ForwardIterator end, const Alternative &newAlt, OpenVarSet &openVars, const SymTab::Indexer &decls, const AssertionSet &newNeed, /*const AssertionParentSet &needParents,*/ 
+        void inferRecursive( ForwardIterator begin, ForwardIterator end, const Alternative &newAlt, OpenVarSet &openVars, const SymTab::Indexer &decls, const AssertionSet &newNeed, /*const AssertionParentSet &needParents,*/
                                                  int level, const SymTab::Indexer &indexer, OutputIterator out ) {
                 if ( begin == end ) {
@@ -469 +469 @@
                                 std::cerr << std::endl;
                         )
-                        
+
                         AssertionSet newHave, newerNeed( newNeed );
                         TypeEnvironment newEnv( newAlt.env );
@@ -847 +847 @@
                 for ( std::list< Declaration* >::const_iterator i = members.begin(); i != members.end(); ++i ) {
                         if ( DeclarationWithType *dwt = dynamic_cast< DeclarationWithType* >( *i ) ) {
-                                alternatives.push_back( Alternative( new OffsetofExpr( aggInst->clone(), dwt->clone() ), env, Cost::zero ) );
+                                alternatives.push_back( Alternative( new OffsetofExpr( aggInst->clone(), dwt ), env, Cost::zero ) );
                                 renameTypes( alternatives.back().expr );
                         } else {

src/SymTab/Validate.cc

@@ -58 +58 @@
 #include "Autogen.h"
 #include "ResolvExpr/typeops.h"
+#include <algorithm>
 
 #define debugPrint( x ) if ( doDebug ) { std::cout << x; }
@@ -372 +373 @@
                 } // if
 
-                applySubstitution( ctx->get_parameters().begin(), ctx->get_parameters().end(), contextInst->get_parameters().begin(), ctx->get_members().begin(), ctx->get_members().end(), back_inserter( contextInst->get_members() ) );
+                // need to clone members of the context for ownership purposes
+                std::list< Declaration * > members;
+                std::transform( ctx->get_members().begin(), ctx->get_members().end(), back_inserter( members ), [](Declaration * dwt) { return dwt->clone(); } );
+
+                applySubstitution( ctx->get_parameters().begin(), ctx->get_parameters().end(), contextInst->get_parameters().begin(), members.begin(), members.end(), back_inserter( contextInst->get_members() ) );
         }
 

src/SynTree/AggregateDecl.cc

@@ -5 +5 @@
 // file "LICENCE" distributed with Cforall.
 //
-// AggregateDecl.cc -- 
+// AggregateDecl.cc --
 //
 // Author           : Richard C. Bilson

src/SynTree/Expression.cc

@@ -200 +200 @@
 
 OffsetofExpr::OffsetofExpr( const OffsetofExpr &other ) :
-        Expression( other ), type( maybeClone( other.type ) ), member( maybeClone( other.member ) ) {}
+        Expression( other ), type( maybeClone( other.type ) ), member( other.member ) {}
 
 OffsetofExpr::~OffsetofExpr() {
         delete type;
-        delete member;
 }
 
@@ -359 +358 @@
         assert( member );
         os << std::string( indent + 2, ' ' );
+        os << (void*)member << " ";
         member->print( os, indent + 2 );
         os << std::endl;

src/SynTree/TypeSubstitution.h

@@ -169 +169 @@
         TypeSubstitution sub = TypeSubstitution( formalBegin, formalEnd, actual );
         for ( std::list< Declaration* >::iterator i = memberBegin; i != memberEnd; ++i ) {
-                Declaration *newdecl = (*i)->clone();
-                sub.apply( newdecl );
-                *out++ = newdecl;
+                sub.apply( *i );
+                *out++ = *i;
         } // for
 }