From patchwork Tue Apr 25 10:03:53 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arnaud Charlet X-Patchwork-Id: 754678 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wBzMD5Cjwz9s7q for ; Tue, 25 Apr 2017 20:04:08 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="lp1pqZav"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; q=dns; s=default; b=nlp9CPtwsuzOFlgIeDRVBE5oW9D1tbapNauS2+A7RNWyhsVKQm jonrn1e+x3g7Bf/3sukpGcrRSMnPFgDozJpq5TpXVY7xgXvXuxoNxg0AsBeU2Wpx tz8sA/ue3vREXpOrpBrh60Riv0+OQOaiEJg6lLTyt8HT8dOCQ55pBr3Eo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; s= default; bh=43l7ejcdpPHa/btpZEr1aeM1AgA=; b=lp1pqZavdpbxomsLvyz2 eFDKwcjij/ngGIVh9L1IcuZ3e/3yXcBzbBzIvfDh970fh+dgIB85ZRy84mwmfn7c pi/cGp5wZtfB6f8Lum5c7eR/PtMHc8D8gzxtAst/faf2TleCy4hTYQoDauWOnBLm eyuVuThcnkR5rD0gs+ayofk= Received: (qmail 86884 invoked by alias); 25 Apr 2017 10:03:56 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 86868 invoked by uid 89); 25 Apr 2017 10:03:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=reflected X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Apr 2017 10:03:53 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9D2353546; Tue, 25 Apr 2017 06:03:53 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id p0eAk6pZXJDV; Tue, 25 Apr 2017 06:03:53 -0400 (EDT) Received: from tron.gnat.com (tron.gnat.com [205.232.38.10]) by rock.gnat.com (Postfix) with ESMTP id 8B97C129C99; Tue, 25 Apr 2017 06:03:53 -0400 (EDT) Received: by tron.gnat.com (Postfix, from userid 4192) id 8A45A521; Tue, 25 Apr 2017 06:03:53 -0400 (EDT) Date: Tue, 25 Apr 2017 06:03:53 -0400 From: Arnaud Charlet To: gcc-patches@gcc.gnu.org Cc: Stephen Baird Subject: [Ada] Validity checks and volatility Message-ID: <20170425100353.GA54400@adacore.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) This patch partially reimplements validity checks to prevent multiple reads or copies of volatile expressions. This is achieved by first capturing the value of a volatile object into a variable (rather than a constant). The variable is then tested for validity (rather than the object again) and used in place of the original object reference (rather than the object again). In addition, if the object reference is utilized as an actual in a call where the corresponding formal is of mode IN OUT or OUT, any changes to the value upon return from the call are now properly reflected back into the object. ------------ -- Source -- ------------ -- main.adb with Ada.Text_IO; use Ada.Text_IO; procedure Main is type Small_Int is new Integer range 1 .. 31; pragma Volatile (Small_Int); procedure Double_Swap (A : in out Small_Int; B : in out Small_Int); procedure Read (A : Small_Int; B : Small_Int); function Self (A : Small_Int) return Small_Int; procedure Swap (A : in out Small_Int; B : in out Small_Int); procedure Tripple_Swap (A : in out Small_Int; B : in out Small_Int); procedure Write (A : out Small_Int; B : out Small_Int); procedure Double_Swap (A : in out Small_Int; B : in out Small_Int) is begin Swap (A, B); Swap (B, A); end Double_Swap; procedure Read (A : Small_Int; B : Small_Int) is begin Put_Line ("A:" & A'Img); Put_Line ("B:" & B'Img); end Read; function Self (A : Small_Int) return Small_Int is begin return A; end Self; procedure Swap (A : in out Small_Int; B : in out Small_Int) is T : Small_Int; begin T := A; A := B; B := T; end Swap; procedure Tripple_Swap (A : in out Small_Int; B : in out Small_Int) is begin Swap (A, B); Swap (B, A); Swap (A, B); end Tripple_Swap; procedure Write (A : out Small_Int; B : out Small_Int) is begin A := 3; B := 4; end Write; X : Small_Int := 1; Y : Small_Int := 2; begin Double_Swap (X, Y); if X /= 1 or else Y /= 2 then Put_Line ("ERROR: Double_Swap failed"); end if; Read (X, Y); Read (Self (X), Self (Y)); Swap (X, Y); if X /= 2 or else Y /= 1 then Put_Line ("ERROR: Swap failed"); end if; Tripple_Swap (X, Y); if X /= 1 or else Y /= 2 then Put_Line ("ERROR: Tripple_Swap failed"); end if; Write (X, Y); if X /= 3 or else Y /= 4 then Put_Line ("ERROR: Write failed"); end if; end Main; ---------------------------- -- Compilation and output -- ---------------------------- $ gnatmake -q -gnatVa main.adb $ ./main A: 1 B: 2 A: 1 B: 2 Tested on x86_64-pc-linux-gnu, committed on trunk 2017-04-25 Steve Baird * exp_ch7.adb (Build_Array_Deep_Procs, Build_Record_Deep_Procs, Make_Finalize_Address_Body): Don't generate Finalize_Address routines for CodePeer. Index: exp_attr.adb =================================================================== --- exp_attr.adb (revision 247141) +++ exp_attr.adb (working copy) @@ -6488,32 +6488,48 @@ --------------------- function Make_Range_Test return Node_Id is - Temp : constant Node_Id := Duplicate_Subexpr (Pref); + Temp : Node_Id; begin - -- The value whose validity is being checked has been captured in - -- an object declaration. We certainly don't want this object to - -- appear valid because the declaration initializes it. + -- The prefix of attribute 'Valid should always denote an object + -- reference. The reference is either coming directly from source + -- or is produced by validity check expansion. - if Is_Entity_Name (Temp) then - Set_Is_Known_Valid (Entity (Temp), False); + -- If the prefix denotes a variable which captures the value of + -- an object for validation purposes, use the variable in the + -- range test. This ensures that no extra copies or extra reads + -- are produced as part of the test. Generate: + + -- Temp : ... := Object; + -- if not Temp in ... then + + if Is_Validation_Variable_Reference (Pref) then + Temp := New_Occurrence_Of (Entity (Pref), Loc); + + -- Otherwise the prefix is either a source object or a constant + -- produced by validity check expansion. Generate: + + -- Temp : constant ... := Pref; + -- if not Temp in ... then + + else + Temp := Duplicate_Subexpr (Pref); end if; return Make_In (Loc, - Left_Opnd => - Unchecked_Convert_To (Btyp, Temp), + Left_Opnd => Unchecked_Convert_To (Btyp, Temp), Right_Opnd => Make_Range (Loc, - Low_Bound => + Low_Bound => Unchecked_Convert_To (Btyp, Make_Attribute_Reference (Loc, - Prefix => New_Occurrence_Of (Ptyp, Loc), + Prefix => New_Occurrence_Of (Ptyp, Loc), Attribute_Name => Name_First)), High_Bound => Unchecked_Convert_To (Btyp, Make_Attribute_Reference (Loc, - Prefix => New_Occurrence_Of (Ptyp, Loc), + Prefix => New_Occurrence_Of (Ptyp, Loc), Attribute_Name => Name_Last)))); end Make_Range_Test; Index: einfo.adb =================================================================== --- einfo.adb (revision 247156) +++ einfo.adb (working copy) @@ -270,6 +270,8 @@ -- Entry_Max_Queue_Lengths_Array Node35 -- Import_Pragma Node35 + -- Validated_Object Node36 + -- Class_Wide_Preconds List38 -- Class_Wide_Postconds List39 @@ -3477,6 +3479,12 @@ return Flag95 (Id); end Uses_Sec_Stack; + function Validated_Object (Id : E) return N is + begin + pragma Assert (Ekind (Id) = E_Variable); + return Node36 (Id); + end Validated_Object; + function Warnings_Off (Id : E) return B is begin return Flag96 (Id); @@ -6618,6 +6626,12 @@ Set_Flag95 (Id, V); end Set_Uses_Sec_Stack; + procedure Set_Validated_Object (Id : E; V : N) is + begin + pragma Assert (Ekind (Id) = E_Variable); + Set_Node36 (Id, V); + end Set_Validated_Object; + procedure Set_Warnings_Off (Id : E; V : B := True) is begin Set_Flag96 (Id, V); @@ -10881,9 +10895,14 @@ ------------------------ procedure Write_Field36_Name (Id : Entity_Id) is - pragma Unreferenced (Id); begin - Write_Str ("Field36??"); + case Ekind (Id) is + when E_Variable => + Write_Str ("Validated_Object"); + + when others => + Write_Str ("Field36??"); + end case; end Write_Field36_Name; ------------------------ Index: einfo.ads =================================================================== --- einfo.ads (revision 247157) +++ einfo.ads (working copy) @@ -4514,6 +4514,10 @@ -- task). Set to True when secondary stack is used in this scope and must -- be released on exit unless Sec_Stack_Needed_For_Return is set. +-- Validated_Object (Node36) +-- Defined in variables. Contains the object whose value is captured by +-- the variable for validity check purposes. + -- Warnings_Off (Flag96) -- Defined in all entities. Set if a pragma Warnings (Off, entity-name) -- is used to suppress warnings for a given entity. It is also used by @@ -6609,6 +6613,7 @@ -- Linker_Section_Pragma (Node33) -- Contract (Node34) -- Anonymous_Designated_Type (Node35) + -- Validated_Object (Node36) -- SPARK_Pragma (Node40) -- Has_Alignment_Clause (Flag46) -- Has_Atomic_Components (Flag86) @@ -7342,6 +7347,7 @@ function Used_As_Generic_Actual (Id : E) return B; function Uses_Lock_Free (Id : E) return B; function Uses_Sec_Stack (Id : E) return B; + function Validated_Object (Id : E) return N; function Warnings_Off (Id : E) return B; function Warnings_Off_Used (Id : E) return B; function Warnings_Off_Used_Unmodified (Id : E) return B; @@ -8029,6 +8035,7 @@ procedure Set_Used_As_Generic_Actual (Id : E; V : B := True); procedure Set_Uses_Lock_Free (Id : E; V : B := True); procedure Set_Uses_Sec_Stack (Id : E; V : B := True); + procedure Set_Validated_Object (Id : E; V : N); procedure Set_Warnings_Off (Id : E; V : B := True); procedure Set_Warnings_Off_Used (Id : E; V : B := True); procedure Set_Warnings_Off_Used_Unmodified (Id : E; V : B := True); @@ -8871,6 +8878,7 @@ pragma Inline (Used_As_Generic_Actual); pragma Inline (Uses_Lock_Free); pragma Inline (Uses_Sec_Stack); + pragma Inline (Validated_Object); pragma Inline (Warnings_Off); pragma Inline (Warnings_Off_Used); pragma Inline (Warnings_Off_Used_Unmodified); @@ -9346,6 +9354,7 @@ pragma Inline (Set_Used_As_Generic_Actual); pragma Inline (Set_Uses_Lock_Free); pragma Inline (Set_Uses_Sec_Stack); + pragma Inline (Set_Validated_Object); pragma Inline (Set_Warnings_Off); pragma Inline (Set_Warnings_Off_Used); pragma Inline (Set_Warnings_Off_Used_Unmodified); Index: checks.adb =================================================================== --- checks.adb (revision 247140) +++ checks.adb (working copy) @@ -7180,52 +7180,93 @@ Exp := Expression (Exp); end loop; + -- Do not generate a check for a variable which already validates the + -- value of an assignable object. + + if Is_Validation_Variable_Reference (Exp) then + return; + end if; + -- We are about to insert the validity check for Exp. We save and -- reset the Do_Range_Check flag over this validity check, and then -- put it back for the final original reference (Exp may be rewritten). declare DRC : constant Boolean := Do_Range_Check (Exp); + CE : Node_Id; + Obj : Node_Id; PV : Node_Id; - CE : Node_Id; + Var : Entity_Id; begin Set_Do_Range_Check (Exp, False); - -- Force evaluation to avoid multiple reads for atomic/volatile + -- If the expression denotes an assignable object, capture its value + -- in a variable and replace the original expression by the variable. + -- This approach has several effects: - -- Note: we set Name_Req to False. We used to set it to True, with - -- the thinking that a name is required as the prefix of the 'Valid - -- call, but in fact the check that the prefix of an attribute is - -- a name is in the parser, and we just don't require it here. - -- Moreover, when we set Name_Req to True, that interfered with the - -- checking for Volatile, since we couldn't just capture the value. + -- 1) The evaluation of the object results in only one read in the + -- case where the object is atomic or volatile. - if Is_Entity_Name (Exp) - and then Is_Volatile (Entity (Exp)) - then - -- Same reasoning as above for setting Name_Req to False + -- Temp ... := Object; -- read - Force_Evaluation (Exp, Name_Req => False); - end if; + -- 2) The captured value is the one verified by attribute 'Valid. + -- As a result the object is not evaluated again, which would + -- result in an unwanted read in the case where the object is + -- atomic or volatile. - -- Build the prefix for the 'Valid call. If the expression denotes - -- a non-volatile name, use a renaming to alias it, otherwise use a - -- constant to capture the value of the expression. + -- if not Temp'Valid then -- OK, no read of Object - -- Temp : ... renames Expr; -- non-volatile name - -- Temp : constant ... := Expr; -- all other cases + -- if not Object'Valid then -- Wrong, extra read of Object - PV := - Duplicate_Subexpr_No_Checks - (Exp => Exp, - Name_Req => False, - Renaming_Req => - Is_Name_Reference (Exp) and then not Is_Volatile (Typ), - Related_Id => Related_Id, - Is_Low_Bound => Is_Low_Bound, - Is_High_Bound => Is_High_Bound); + -- 3) The captured value replaces the original object reference. + -- As a result the object is not evaluated again, in the same + -- vein as 2). + -- ... Temp ... -- OK, no read of Object + + -- ... Object ... -- Wrong, extra read of Object + + -- 4) The use of a variable to capture the value of the object + -- allows the propagation of any changes back to the original + -- object. + + -- procedure Call (Val : in out ...); + + -- Temp : ... := Object; -- read Object + -- if not Temp'Valid then -- validity check + -- Call (Temp); -- modify Temp + -- Object := Temp; -- update Object + + if Is_Variable (Exp) then + Obj := New_Copy_Tree (Exp); + Var := Make_Temporary (Loc, 'T', Exp); + + Insert_Action (Exp, + Make_Object_Declaration (Loc, + Defining_Identifier => Var, + Object_Definition => New_Occurrence_Of (Typ, Loc), + Expression => Relocate_Node (Exp))); + Set_Validated_Object (Var, Obj); + + Rewrite (Exp, New_Occurrence_Of (Var, Loc)); + PV := New_Occurrence_Of (Var, Loc); + + -- Otherwise the expression does not denote a variable. Force its + -- evaluation by capturing its value in a constant. Generate: + + -- Temp : constant ... := Exp; + + else + Force_Evaluation + (Exp => Exp, + Related_Id => Related_Id, + Is_Low_Bound => Is_Low_Bound, + Is_High_Bound => Is_High_Bound); + + PV := New_Copy_Tree (Exp); + end if; + -- A rather specialized test. If PV is an analyzed expression which -- is an indexed component of a packed array that has not been -- properly expanded, turn off its Analyzed flag to make sure it Index: sem_util.adb =================================================================== --- sem_util.adb (revision 247167) +++ sem_util.adb (working copy) @@ -15277,6 +15277,19 @@ return T = Universal_Integer or else T = Universal_Real; end Is_Universal_Numeric_Type; + -------------------------------------- + -- Is_Validation_Variable_Reference -- + -------------------------------------- + + function Is_Validation_Variable_Reference (N : Node_Id) return Boolean is + begin + return + Is_Entity_Name (N) + and then Present (Entity (N)) + and then Ekind (Entity (N)) = E_Variable + and then Present (Validated_Object (Entity (N))); + end Is_Validation_Variable_Reference; + ---------------------------- -- Is_Variable_Size_Array -- ---------------------------- @@ -15643,7 +15656,6 @@ ------------------------ function Is_Volatile_Object (N : Node_Id) return Boolean is - function Is_Volatile_Prefix (N : Node_Id) return Boolean; -- If prefix is an implicit dereference, examine designated type Index: sem_util.ads =================================================================== --- sem_util.ads (revision 247162) +++ sem_util.ads (working copy) @@ -1786,6 +1786,10 @@ pragma Inline (Is_Universal_Numeric_Type); -- True if T is Universal_Integer or Universal_Real + function Is_Validation_Variable_Reference (N : Node_Id) return Boolean; + -- Determine whether N denotes a reference to a variable which captures the + -- value of an object for validation purposes. + function Is_Variable_Size_Array (E : Entity_Id) return Boolean; -- Returns true if E has variable size components Index: exp_ch6.adb =================================================================== --- exp_ch6.adb (revision 247169) +++ exp_ch6.adb (working copy) @@ -1901,6 +1901,21 @@ then Add_Call_By_Copy_Code; + -- The actual denotes a variable which captures the value of an + -- object for validation purposes. Add a copy-back to reflect any + -- potential changes in value back into the original object. + + -- Temp : ... := Object; + -- if not Temp'Valid then ... + -- Call (Temp); + -- Object := Temp; + + elsif Is_Validation_Variable_Reference (Actual) then + Append_To (Post_Call, + Make_Assignment_Statement (Loc, + Name => Validated_Object (Entity (Actual)), + Expression => New_Occurrence_Of (Entity (Actual), Loc))); + elsif Nkind (Actual) = N_Indexed_Component and then Is_Entity_Name (Prefix (Actual)) and then Has_Volatile_Components (Entity (Prefix (Actual)))