diff mbox

[Ada] Cleanup work in internal support for renaming (2)

Message ID 20150526081808.GA18772@adacore.com
State New
Headers show

Commit Message

Arnaud Charlet May 26, 2015, 8:18 a.m. UTC
As explained in the previous change, Remove_Side_Effects already knows to
avoid renaming function calls in most cases and, more generally, to avoid
renaming something that does not denote a name. But the implementation is
a little ad-hoc and not bullet-proof, for example a function call subject
to an unchecked conversion will be renamed.

The change introduces a new predicate Is_Name_Reference, which is closely
modeled on Is_Object_Reference but returns true only for nodes that denote
a name and arranges for Remove_Side_Effects to generate a renaming only in
this case.

No functional changes.

Tested on x86_64-pc-linux-gnu, committed on trunk

2015-05-26  Eric Botcazou  <ebotcazou@adacore.com>

	* exp_ch6.adb (Add_Call_By_Copy_Code): Remove restrictive
	condition in the detection of the effects of Remove_Side_Effects.
	* exp_util.ads (Remove_Side_Effects): Add general and historical note.
	* exp_util.adb (Is_Name_Reference): New predicate.
	(Remove_Side_Effects): Use it in lieu of Is_Object_Reference
	in order to decide whether to use the renaming to capture the
	side effects of the subexpression.
	(Side_Effect_Free): Remove obsolete test.
diff mbox

Patch

Index: exp_util.adb
===================================================================
--- exp_util.adb	(revision 223667)
+++ exp_util.adb	(working copy)
@@ -7428,6 +7428,12 @@ 
       --  is present (xxx is taken from the Chars field of Related_Nod),
       --  otherwise it generates an internal temporary.
 
+      function Is_Name_Reference (N : Node_Id) return Boolean;
+      --  Determine if the tree referenced by N represents a name. This is
+      --  similar to Is_Object_Reference but returns true only if N can be
+      --  renamed without the need for a temporary, the typical example of
+      --  an object not in this category being a function call.
+
       ---------------------
       -- Build_Temporary --
       ---------------------
@@ -7458,6 +7464,58 @@ 
          end if;
       end Build_Temporary;
 
+      -----------------------
+      -- Is_Name_Reference --
+      -----------------------
+
+      function Is_Name_Reference (N : Node_Id) return Boolean is
+      begin
+         if Is_Entity_Name (N) then
+            return Present (Entity (N)) and then Is_Object (Entity (N));
+         end if;
+
+         case Nkind (N) is
+            when N_Indexed_Component | N_Slice =>
+               return
+                 Is_Name_Reference (Prefix (N))
+                   or else Is_Access_Type (Etype (Prefix (N)));
+
+            --  Attributes 'Input, 'Old and 'Result produce objects
+
+            when N_Attribute_Reference =>
+               return
+                 Nam_In
+                   (Attribute_Name (N), Name_Input, Name_Old, Name_Result);
+
+            when N_Selected_Component =>
+               return
+                 Is_Name_Reference (Selector_Name (N))
+                   and then
+                     (Is_Name_Reference (Prefix (N))
+                       or else Is_Access_Type (Etype (Prefix (N))));
+
+            when N_Explicit_Dereference =>
+               return True;
+
+            --  A view conversion of a tagged name is a name reference
+
+            when N_Type_Conversion =>
+               return Is_Tagged_Type (Etype (Subtype_Mark (N)))
+                 and then Is_Tagged_Type (Etype (Expression (N)))
+                 and then Is_Name_Reference (Expression (N));
+
+            --  An unchecked type conversion is considered to be a name if
+            --  the operand is a name (this construction arises only as a
+            --  result of expansion activities).
+
+            when N_Unchecked_Type_Conversion =>
+               return Is_Name_Reference (Expression (N));
+
+            when others =>
+               return False;
+         end case;
+      end Is_Name_Reference;
+
       --  Local variables
 
       Loc          : constant Source_Ptr      := Sloc (Exp);
@@ -7498,34 +7556,25 @@ 
          return;
       end if;
 
-      --  The remaining procesaing is done with all checks suppressed
+      --  The remaining processing is done with all checks suppressed
 
       --  Note: from now on, don't use return statements, instead do a goto
       --  Leave, to ensure that we properly restore Scope_Suppress.Suppress.
 
       Scope_Suppress.Suppress := (others => True);
 
-      --  If it is a scalar type and we need to capture the value, just make
-      --  a copy. Likewise for a function call, an attribute reference, a
-      --  conditional expression, an allocator, or an operator. And if we have
-      --  a volatile reference and Name_Req is not set (see comments for
-      --  Side_Effect_Free).
+      --  If it is an elementary type and we need to capture the value, just
+      --  make a constant. Likewise if this is not a name reference, except
+      --  for a type conversion because we would enter an infinite recursion
+      --  with Checks.Apply_Predicate_Check if the target type has predicates.
+      --  And type conversions need a specific treatment anyway, see below.
+      --  Also do it if we have a volatile reference and Name_Req is not set
+      --  (see comments for Side_Effect_Free).
 
       if Is_Elementary_Type (Exp_Type)
-
-        --  Note: this test is rather mysterious??? Why can't we just test ONLY
-        --  Is_Elementary_Type and be done with it. If we try that approach, we
-        --  get some failures (infinite recursions) from the Duplicate_Subexpr
-        --  call at the end of Checks.Apply_Predicate_Check. To be
-        --  investigated ???
-
         and then (Variable_Ref
-                   or else Nkind_In (Exp, N_Attribute_Reference,
-                                          N_Allocator,
-                                          N_Case_Expression,
-                                          N_If_Expression,
-                                          N_Function_Call)
-                   or else Nkind (Exp) in N_Op
+                   or else (not Is_Name_Reference (Exp)
+                             and then Nkind (Exp) /= N_Type_Conversion)
                    or else (not Name_Req
                              and then Is_Volatile_Reference (Exp)))
       then
@@ -7645,21 +7694,14 @@ 
             Insert_Action (Exp, E);
          end if;
 
-      --  For expressions that denote objects, we can use a renaming scheme.
+      --  For expressions that denote names, we can use a renaming scheme.
       --  This is needed for correctness in the case of a volatile object of
       --  a non-volatile type because the Make_Reference call of the "default"
       --  approach would generate an illegal access value (an access value
       --  cannot designate such an object - see Analyze_Reference).
 
-      elsif Is_Object_Reference (Exp)
-        and then Nkind (Exp) /= N_Function_Call
+      elsif Is_Name_Reference (Exp)
 
-        --  In Ada 2012 a qualified expression is an object, but for purposes
-        --  of removing side effects it still need to be transformed into a
-        --  separate declaration, particularly in the case of an aggregate.
-
-        and then Nkind (Exp) /= N_Qualified_Expression
-
         --  We skip using this scheme if we have an object of a volatile
         --  type and we do not have Name_Req set true (see comments for
         --  Side_Effect_Free).
@@ -7667,38 +7709,14 @@ 
         and then (Name_Req or else not Treat_As_Volatile (Exp_Type))
       then
          Def_Id := Build_Temporary (Loc, 'R', Exp);
+         Res := New_Occurrence_Of (Def_Id, Loc);
 
-         if Nkind (Exp) = N_Selected_Component
-           and then Nkind (Prefix (Exp)) = N_Function_Call
-           and then Is_Array_Type (Exp_Type)
-         then
-            --  Avoid generating a variable-sized temporary, by generating
-            --  the renaming declaration just for the function call. The
-            --  transformation could be refined to apply only when the array
-            --  component is constrained by a discriminant???
+         Insert_Action (Exp,
+           Make_Object_Renaming_Declaration (Loc,
+             Defining_Identifier => Def_Id,
+             Subtype_Mark        => New_Occurrence_Of (Exp_Type, Loc),
+             Name                => Relocate_Node (Exp)));
 
-            Res :=
-              Make_Selected_Component (Loc,
-                Prefix => New_Occurrence_Of (Def_Id, Loc),
-                Selector_Name => Selector_Name (Exp));
-
-            Insert_Action (Exp,
-              Make_Object_Renaming_Declaration (Loc,
-                Defining_Identifier => Def_Id,
-                Subtype_Mark        =>
-                  New_Occurrence_Of (Base_Type (Etype (Prefix (Exp))), Loc),
-                Name                => Relocate_Node (Prefix (Exp))));
-
-         else
-            Res := New_Occurrence_Of (Def_Id, Loc);
-
-            Insert_Action (Exp,
-              Make_Object_Renaming_Declaration (Loc,
-                Defining_Identifier => Def_Id,
-                Subtype_Mark        => New_Occurrence_Of (Exp_Type, Loc),
-                Name                => Relocate_Node (Exp)));
-         end if;
-
          --  If this is a packed reference, or a selected component with
          --  a non-standard representation, a reference to the temporary
          --  will be replaced by a copy of the original expression (see
@@ -7715,8 +7733,20 @@ 
             Set_Is_Renaming_Of_Object (Def_Id, False);
          end if;
 
-      --  Otherwise we generate a reference to the value
+      --  Avoid generating a variable-sized temporary, by generating the
+      --  reference just for the function call. The transformation could be
+      --  refined to apply only when the array component is constrained by a
+      --  discriminant???
 
+      elsif Nkind (Exp) = N_Selected_Component
+        and then Nkind (Prefix (Exp)) = N_Function_Call
+        and then Is_Array_Type (Exp_Type)
+      then
+         Remove_Side_Effects (Prefix (Exp), Name_Req, Variable_Ref);
+         goto Leave;
+
+      --  Otherwise we generate a reference to the expression
+
       else
          --  An expression which is in SPARK mode is considered side effect
          --  free if the resulting value is captured by a variable or a
@@ -8974,23 +9004,10 @@ 
             return Side_Effect_Free (Expression (N), Name_Req, Variable_Ref);
 
          --  A selected component is side effect free only if it is a side
-         --  effect free prefixed reference. If it designates a component
-         --  with a rep. clause it must be treated has having a potential
-         --  side effect, because it may be modified through a renaming, and
-         --  a subsequent use of the renaming as a macro will yield the
-         --  wrong value. This complex interaction between renaming and
-         --  removing side effects is a reminder that the latter has become
-         --  a headache to maintain, and that it should be removed in favor
-         --  of the gcc mechanism to capture values ???
+         --  effect free prefixed reference.
 
          when N_Selected_Component =>
-            if Nkind (Parent (N)) = N_Explicit_Dereference
-              and then Has_Non_Standard_Rep (Designated_Type (Typ))
-            then
-               return False;
-            else
-               return Safe_Prefixed_Reference (N);
-            end if;
+            return Safe_Prefixed_Reference (N);
 
          --  A range is side effect free if the bounds are side effect free
 
Index: exp_util.ads
===================================================================
--- exp_util.ads	(revision 223661)
+++ exp_util.ads	(working copy)
@@ -872,8 +872,8 @@ 
    --  call and is analyzed and resolved on return. Name_Req may only be set to
    --  True if Exp has the form of a name, and the effect is to guarantee that
    --  any replacement maintains the form of name. If Renaming_Req is set to
-   --  TRUE, the routine produces an object renaming reclaration capturing the
-   --  expression. If Variable_Ref is set to TRUE, a variable is considered as
+   --  True, the routine produces an object renaming reclaration capturing the
+   --  expression. If Variable_Ref is set to True, a variable is considered as
    --  side effect (used in implementing Force_Evaluation). Note: after call to
    --  Remove_Side_Effects, it is safe to call New_Copy_Tree to obtain a copy
    --  of the resulting expression.
@@ -885,6 +885,26 @@ 
    --  Chars (Related_Id)_FIRST/_LAST. If Related_Id is set, then exactly one
    --  of the Is_xxx_Bound flags must be set. For use of these parameters see
    --  the warning in the body of Sem_Ch3.Process_Range_Expr_In_Decl.
+   --
+   --  The side effects are captured using one of the following methods:
+   --
+   --    1) a constant initialized with the value of the subexpression
+   --    2) a renaming of the subexpression
+   --    3) a reference to the subexpression
+   --
+   --  For elementary types, methods 1) and 2) are used; for composite types,
+   --  methods 2) and 3) are used. The renaming (method 2) is used only when
+   --  the subexpression denotes a name, so that it can be elaborated by gigi
+   --  without evaluating the subexpression.
+   --
+   --  Historical note: the reference (method 3) used to be the common fallback
+   --  method but it gives rise to aliasing issues if the subexpression denotes
+   --  a name that is not aliased, since it is equivalent to taking the address
+   --  in this case. The renaming (method 2) used to be applied to any objects
+   --  in the RM sense, that is to say to the cases where a renaming is legal
+   --  in Ada. But for some of these cases, most notably functions calls, the
+   --  renaming cannot be elaborated without evaluating the subexpression, so
+   --  gigi would resort to method 1) or 3) under the hood for them.
 
    function Represented_As_Scalar (T : Entity_Id) return Boolean;
    --  Returns True iff the implementation of this type in code generation
Index: exp_ch6.adb
===================================================================
--- exp_ch6.adb	(revision 223667)
+++ exp_ch6.adb	(working copy)
@@ -1257,7 +1257,6 @@ 
             begin
                if Is_Renaming_Of_Object (Var)
                  and then Nkind (Renamed_Object (Var)) = N_Selected_Component
-                 and then Is_Entity_Name (Prefix (Renamed_Object (Var)))
                  and then Nkind (Original_Node (Prefix (Renamed_Object (Var))))
                    = N_Indexed_Component
                  and then