Changeset ae265b5 for src/AST/Expr.cpp

Timestamp:

Aug 7, 2019, 5:08:58 PM (2 years ago)

Author:

Michael Brooks <mlbrooks@…>

Branches:

arm-eh, jacob/cs343-translation, master, new-ast, new-ast-unique-expr

Children:

ba417e2, c570806

Parents:

2890212

Message:

Two fixes of MemberExpr? handling on new-AST.

Each bug on its own, and also both bugs together, produce the same symptom: Expressions that use function-pointer typed members of structs, where the function has a generically-typed argument, were lacking casts (vs old-AST) in the output of:

./driver/cfa-cpp --prelude-dir ./libcfa/x64-debug/prelude -tm ../cfa-cc/libcfa/prelude/bootloader.cf bootloader.c

First bug: a conversion inaccuracy, which is a regression of convert-convert-post-resolve, introduced at Jun 28 rev 55b647. The constructor of a MemberExpr?, that Convert old-to-new uses, does type-variable substitution. This is intended behaviour when it occurs during resolve, but is incorrect when done twice. In absence of this fix, the old-to-new converter runs this substitution a second time, both in: oldresolve-old2new-new2old and old2new-newresolve-new2old. The fix is offering/using a no-op constructor to/at the converter.

Second bug: a mutation of a transitively-multiply-referenced object. The type-variable substitution just mentioned, which should be happening to the result type of the MemberExpr? and not to the top-level struct-member declaration, actually happens as:

• old ast: as should; all type-value intentions are clones
• new ast pre this fix: incorrectly to both types; ref-counted sharing means theExpr.result == theDecl.type, substitution happens on sub-object (see details in code comment)
• new ast with this fix: as should; deep copy of the type is explicitly forced upfront

All bootloader output differences (new-AST's resolver vs old-AST's resolver) that remain after this fix are: four constructor-destructor calls in builtin function bodies, and a missing copy-constructor call on invoke-main. These differences were present ahead of this change.

This change has been tested for a clean convert-convert difference (vs no new-AST at all), for convert-convert at: pre-resolve, post-resolve, post-parse. In the case of cvcv post-resolve, this clean difference is being restored from the first-bug regression.

This change has been tested for, and causes a small-to-questionable regression of, cfa-cpp speed of compiling bootloader. Old AST = 6.04 s, New AST before this change = 4.64 s, New ast after this change 4.76 s. All numbers are mean of 5 samples with sample SD ~= 0.3 sec. New-AST improvement before this fix = 23% = 4.2 SDs. New-AST improvement after this fix = 21% = 3.9 SDs.

File:

• src/AST/Expr.cpp (modified) (1 diff)

src/AST/Expr.cpp

@@ -158 +158 @@
         assert( aggregate->result );
 
-        // take ownership of member type
-        result = mem->get_type();
+        // Deep copy on result type avoids mutation on transitively multiply referenced object.
+        //
+        // Example, adapted from parts of builtins and bootloader:
+        //
+        // forall(dtype T)
+        // struct __Destructor {
+        //   T * object;
+        //   void (*dtor)(T *);
+        // };
+        //
+        // forall(dtype S)
+        // void foo(__Destructor(S) &d) {
+        //   if (d.dtor) {  // here
+        //   }
+        // }
+        //
+        // Let e be the "d.dtor" guard espression, which is MemberExpr after resolve.  Let d be the
+        // declaration of member __Destructor.dtor (an ObjectDecl), as accessed via the top-level
+        // declaration of __Destructor.  Consider the types e.result and d.type.  In the old AST, one
+        // is a clone of the other.  Ordinary new-AST use would set them up as a multiply-referenced
+        // object.
+        //
+        // e.result: PointerType
+        // .base: FunctionType
+        // .params.front(): ObjectDecl, the anonymous parameter of type T*
+        // .type: PointerType
+        // .base: TypeInstType
+        // let x = that
+        // let y = similar, except start from d.type
+        //
+        // Consider two code lines down, genericSubstitution(...).apply(result).
+        //
+        // Applying this chosen-candidate's type substitution means modifying x, substituting
+        // S for T.  This mutation should affect x and not y.
+
+        result = deepCopy(mem->get_type());
+
         // substitute aggregate generic parameters into member type
         genericSubstitution( aggregate->result ).apply( result );
         // ensure lvalue and appropriate restrictions from aggregate type
         add_qualifiers( result, aggregate->result->qualifiers | CV::Lvalue );
+}
+
+MemberExpr::MemberExpr( const CodeLocation & loc, const DeclWithType * mem, const Expr * agg,
+    MemberExpr::NoOpConstruction overloadSelector )
+: Expr( loc ), member( mem ), aggregate( agg ) {
+        assert( member );
+        assert( aggregate );
+        assert( aggregate->result );
+        (void) overloadSelector;
 }