Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
crossgcc: Fix runtime initialization of a constant
GNAT had a constant initialized at runtime which led to trouble with compilers that decided to place it into an actual constant section (e.g. GCC 9). Usually, this would be handled gracefully if the Ada compiler knew about the runtime initialization. How- ever, as the initialization was done by taking the address of the variable, the compiler had no clue.
Change-Id: I73ce4cadc612c814ed2e22b44f429af2ad3db288 Signed-off-by: Nico Huber nico.h@gmx.de --- A util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch 1 file changed, 150 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/34147/1
diff --git a/util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch b/util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch new file mode 100644 index 0000000..e98f933 --- /dev/null +++ b/util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch @@ -0,0 +1,150 @@ +commit b6f742f96c62bab0582021455328ae3be58e16d3 +Author: pmderodat pmderodat@138bc75d-0d04-0410-961f-82ee72b054a4 +Date: Fri May 25 09:05:10 2018 +0000 + + [Ada] Remove "constant" attribute on Osint.Unknown_Attributes + + 2018-05-25 Arnaud Charlet charlet@adacore.com + + gcc/ada/ + + * exp_aggr.adb (Convert_To_Positional): Bump default for + Max_Others_Replicate to 32. Update comments. + * osint.ads (Unknown_Attributes): No longer pretend this is a constant. + (No_File_Info_Cache): Initialize separately. + * osint.adb (No_File_Info_Cache): Update initializer. + + git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@260739 138bc75d-0d04-0410-961f-82ee72b054a4 + +diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog +index e4127e472aa..d56240b7b82 100644 +--- a/gcc/ada/ChangeLog ++++ b/gcc/ada/ChangeLog +@@ -188,6 +188,14 @@ + an allocator if the type is an unconstrained record type with default + discriminant. + ++2018-05-25 Arnaud Charlet charlet@adacore.com ++ ++ * exp_aggr.adb (Convert_To_Positional): Bump default for ++ Max_Others_Replicate to 32. Update comments. ++ * osint.ads (Unknown_Attributes): No longer pretend this is a constant. ++ (No_File_Info_Cache): Initialize separately. ++ * osint.adb (No_File_Info_Cache): Update initializer. ++ + 2018-05-04 John Marino gnugcc@marino.st + + PR ada/85635 +diff --git a/gcc/ada/exp_aggr.adb b/gcc/ada/exp_aggr.adb +index f723c1b4d99..ff5210eb4e4 100644 +--- a/gcc/ada/exp_aggr.adb ++++ b/gcc/ada/exp_aggr.adb +@@ -284,14 +284,14 @@ package body Exp_Aggr is + + procedure Convert_To_Positional + (N : Node_Id; +- Max_Others_Replicate : Nat := 5; ++ Max_Others_Replicate : Nat := 32; + Handle_Bit_Packed : Boolean := False); + -- If possible, convert named notation to positional notation. This + -- conversion is possible only in some static cases. If the conversion is + -- possible, then N is rewritten with the analyzed converted aggregate. + -- The parameter Max_Others_Replicate controls the maximum number of + -- values corresponding to an others choice that will be converted to +- -- positional notation (the default of 5 is the normal limit, and reflects ++ -- positional notation (the default of 32 is the normal limit, and reflects + -- the fact that normally the loop is better than a lot of separate + -- assignments). Note that this limit gets overridden in any case if + -- either of the restrictions No_Elaboration_Code or No_Implicit_Loops is +@@ -301,11 +301,6 @@ package body Exp_Aggr is + -- Packed_Array_Aggregate_Handled, we set this parameter to True, since + -- these are cases we handle in there. + +- -- It would seem useful to have a higher default for Max_Others_Replicate, +- -- but aggregates in the compiler make this impossible: the compiler +- -- bootstrap fails if Max_Others_Replicate is greater than 25. This +- -- is unexpected ??? +- + procedure Expand_Array_Aggregate (N : Node_Id); + -- This is the top-level routine to perform array aggregate expansion. + -- N is the N_Aggregate node to be expanded. +@@ -4292,7 +4287,7 @@ package body Exp_Aggr is + + procedure Convert_To_Positional + (N : Node_Id; +- Max_Others_Replicate : Nat := 5; ++ Max_Others_Replicate : Nat := 32; + Handle_Bit_Packed : Boolean := False) + is + Typ : constant Entity_Id := Etype (N); +diff --git a/gcc/ada/osint.adb b/gcc/ada/osint.adb +index 0c23761b6dc..896fbc7ee37 100644 +--- a/gcc/ada/osint.adb ++++ b/gcc/ada/osint.adb +@@ -250,8 +250,7 @@ package body Osint is + Attr : aliased File_Attributes; + end record; + +- No_File_Info_Cache : constant File_Info_Cache := +- (No_File, Unknown_Attributes); ++ No_File_Info_Cache : constant File_Info_Cache := (No_File, (others => 0)); + + package File_Name_Hash_Table is new GNAT.HTable.Simple_HTable ( + Header_Num => File_Hash_Num, +diff --git a/gcc/ada/osint.ads b/gcc/ada/osint.ads +index 65a87fe4ce3..6c75b521456 100644 +--- a/gcc/ada/osint.ads ++++ b/gcc/ada/osint.ads +@@ -255,10 +255,26 @@ package Osint is + -- from the disk and then cached in the File_Attributes parameter (possibly + -- along with other values). + +- type File_Attributes is private; +- Unknown_Attributes : constant File_Attributes; ++ File_Attributes_Size : constant Natural := 32; ++ -- This should be big enough to fit a "struct file_attributes" on any ++ -- system. It doesn't cause any malfunction if it is too big (which avoids ++ -- the need for either mapping the struct exactly or importing the sizeof ++ -- from C, which would result in dynamic code). However, it does waste ++ -- space (e.g. when a component of this type appears in a record, if it is ++ -- unnecessarily large). Note: for runtime units, use System.OS_Constants. ++ -- SIZEOF_struct_file_attributes instead, which has the exact value. ++ ++ type File_Attributes is ++ array (1 .. File_Attributes_Size) ++ of System.Storage_Elements.Storage_Element; ++ for File_Attributes'Alignment use Standard'Maximum_Alignment; ++ ++ Unknown_Attributes : File_Attributes; + -- A cache for various attributes for a file (length, accessibility,...) +- -- This must be initialized to Unknown_Attributes prior to the first call. ++ -- Will be initialized properly at elaboration (for efficiency later on, ++ -- avoid function calls every time we want to reset the attributes) prior ++ -- to the first usage. We cannot make it constant since the compiler may ++ -- put it in a read-only section. + + function Is_Directory + (Name : C_File_Name; +@@ -754,22 +770,4 @@ private + -- detected, the file being written is deleted, and a fatal error is + -- signalled. + +- File_Attributes_Size : constant Natural := 32; +- -- This should be big enough to fit a "struct file_attributes" on any +- -- system. It doesn't cause any malfunction if it is too big (which avoids +- -- the need for either mapping the struct exactly or importing the sizeof +- -- from C, which would result in dynamic code). However, it does waste +- -- space (e.g. when a component of this type appears in a record, if it is +- -- unnecessarily large). Note: for runtime units, use System.OS_Constants. +- -- SIZEOF_struct_file_attributes instead, which has the exact value. +- +- type File_Attributes is +- array (1 .. File_Attributes_Size) +- of System.Storage_Elements.Storage_Element; +- for File_Attributes'Alignment use Standard'Maximum_Alignment; +- +- Unknown_Attributes : constant File_Attributes := (others => 0); +- -- Will be initialized properly at elaboration (for efficiency later on, +- -- avoid function calls every time we want to reset the attributes). +- + end Osint;
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 1: Code-Review+1
Tested, working. Fixes building Ada stuffs with a toolchain built with Arch Linux gcc 9.1.0-2 (GCC 9.1.0).
I've only given a +1 to avoid a premature submit. This vote's effective value is a +2.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 1: Code-Review+1
nice catch
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 1: Code-Review+2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 1: Code-Review+2
Looks like there's enough eyeballs on this change.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
crossgcc: Fix runtime initialization of a constant
GNAT had a constant initialized at runtime which led to trouble with compilers that decided to place it into an actual constant section (e.g. GCC 9). Usually, this would be handled gracefully if the Ada compiler knew about the runtime initialization. How- ever, as the initialization was done by taking the address of the variable, the compiler had no clue.
Change-Id: I73ce4cadc612c814ed2e22b44f429af2ad3db288 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/34147 Reviewed-by: Patrick Georgi pgeorgi@google.com Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Edward O'Callaghan quasisec@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- A util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch 1 file changed, 150 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved HAOUAS Elyes: Looks good to me, approved Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved
diff --git a/util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch b/util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch new file mode 100644 index 0000000..e98f933 --- /dev/null +++ b/util/crossgcc/patches/gcc-8.3.0_gnat-bad_constant.patch @@ -0,0 +1,150 @@ +commit b6f742f96c62bab0582021455328ae3be58e16d3 +Author: pmderodat pmderodat@138bc75d-0d04-0410-961f-82ee72b054a4 +Date: Fri May 25 09:05:10 2018 +0000 + + [Ada] Remove "constant" attribute on Osint.Unknown_Attributes + + 2018-05-25 Arnaud Charlet charlet@adacore.com + + gcc/ada/ + + * exp_aggr.adb (Convert_To_Positional): Bump default for + Max_Others_Replicate to 32. Update comments. + * osint.ads (Unknown_Attributes): No longer pretend this is a constant. + (No_File_Info_Cache): Initialize separately. + * osint.adb (No_File_Info_Cache): Update initializer. + + git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@260739 138bc75d-0d04-0410-961f-82ee72b054a4 + +diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog +index e4127e472aa..d56240b7b82 100644 +--- a/gcc/ada/ChangeLog ++++ b/gcc/ada/ChangeLog +@@ -188,6 +188,14 @@ + an allocator if the type is an unconstrained record type with default + discriminant. + ++2018-05-25 Arnaud Charlet charlet@adacore.com ++ ++ * exp_aggr.adb (Convert_To_Positional): Bump default for ++ Max_Others_Replicate to 32. Update comments. ++ * osint.ads (Unknown_Attributes): No longer pretend this is a constant. ++ (No_File_Info_Cache): Initialize separately. ++ * osint.adb (No_File_Info_Cache): Update initializer. ++ + 2018-05-04 John Marino gnugcc@marino.st + + PR ada/85635 +diff --git a/gcc/ada/exp_aggr.adb b/gcc/ada/exp_aggr.adb +index f723c1b4d99..ff5210eb4e4 100644 +--- a/gcc/ada/exp_aggr.adb ++++ b/gcc/ada/exp_aggr.adb +@@ -284,14 +284,14 @@ package body Exp_Aggr is + + procedure Convert_To_Positional + (N : Node_Id; +- Max_Others_Replicate : Nat := 5; ++ Max_Others_Replicate : Nat := 32; + Handle_Bit_Packed : Boolean := False); + -- If possible, convert named notation to positional notation. This + -- conversion is possible only in some static cases. If the conversion is + -- possible, then N is rewritten with the analyzed converted aggregate. + -- The parameter Max_Others_Replicate controls the maximum number of + -- values corresponding to an others choice that will be converted to +- -- positional notation (the default of 5 is the normal limit, and reflects ++ -- positional notation (the default of 32 is the normal limit, and reflects + -- the fact that normally the loop is better than a lot of separate + -- assignments). Note that this limit gets overridden in any case if + -- either of the restrictions No_Elaboration_Code or No_Implicit_Loops is +@@ -301,11 +301,6 @@ package body Exp_Aggr is + -- Packed_Array_Aggregate_Handled, we set this parameter to True, since + -- these are cases we handle in there. + +- -- It would seem useful to have a higher default for Max_Others_Replicate, +- -- but aggregates in the compiler make this impossible: the compiler +- -- bootstrap fails if Max_Others_Replicate is greater than 25. This +- -- is unexpected ??? +- + procedure Expand_Array_Aggregate (N : Node_Id); + -- This is the top-level routine to perform array aggregate expansion. + -- N is the N_Aggregate node to be expanded. +@@ -4292,7 +4287,7 @@ package body Exp_Aggr is + + procedure Convert_To_Positional + (N : Node_Id; +- Max_Others_Replicate : Nat := 5; ++ Max_Others_Replicate : Nat := 32; + Handle_Bit_Packed : Boolean := False) + is + Typ : constant Entity_Id := Etype (N); +diff --git a/gcc/ada/osint.adb b/gcc/ada/osint.adb +index 0c23761b6dc..896fbc7ee37 100644 +--- a/gcc/ada/osint.adb ++++ b/gcc/ada/osint.adb +@@ -250,8 +250,7 @@ package body Osint is + Attr : aliased File_Attributes; + end record; + +- No_File_Info_Cache : constant File_Info_Cache := +- (No_File, Unknown_Attributes); ++ No_File_Info_Cache : constant File_Info_Cache := (No_File, (others => 0)); + + package File_Name_Hash_Table is new GNAT.HTable.Simple_HTable ( + Header_Num => File_Hash_Num, +diff --git a/gcc/ada/osint.ads b/gcc/ada/osint.ads +index 65a87fe4ce3..6c75b521456 100644 +--- a/gcc/ada/osint.ads ++++ b/gcc/ada/osint.ads +@@ -255,10 +255,26 @@ package Osint is + -- from the disk and then cached in the File_Attributes parameter (possibly + -- along with other values). + +- type File_Attributes is private; +- Unknown_Attributes : constant File_Attributes; ++ File_Attributes_Size : constant Natural := 32; ++ -- This should be big enough to fit a "struct file_attributes" on any ++ -- system. It doesn't cause any malfunction if it is too big (which avoids ++ -- the need for either mapping the struct exactly or importing the sizeof ++ -- from C, which would result in dynamic code). However, it does waste ++ -- space (e.g. when a component of this type appears in a record, if it is ++ -- unnecessarily large). Note: for runtime units, use System.OS_Constants. ++ -- SIZEOF_struct_file_attributes instead, which has the exact value. ++ ++ type File_Attributes is ++ array (1 .. File_Attributes_Size) ++ of System.Storage_Elements.Storage_Element; ++ for File_Attributes'Alignment use Standard'Maximum_Alignment; ++ ++ Unknown_Attributes : File_Attributes; + -- A cache for various attributes for a file (length, accessibility,...) +- -- This must be initialized to Unknown_Attributes prior to the first call. ++ -- Will be initialized properly at elaboration (for efficiency later on, ++ -- avoid function calls every time we want to reset the attributes) prior ++ -- to the first usage. We cannot make it constant since the compiler may ++ -- put it in a read-only section. + + function Is_Directory + (Name : C_File_Name; +@@ -754,22 +770,4 @@ private + -- detected, the file being written is deleted, and a fatal error is + -- signalled. + +- File_Attributes_Size : constant Natural := 32; +- -- This should be big enough to fit a "struct file_attributes" on any +- -- system. It doesn't cause any malfunction if it is too big (which avoids +- -- the need for either mapping the struct exactly or importing the sizeof +- -- from C, which would result in dynamic code). However, it does waste +- -- space (e.g. when a component of this type appears in a record, if it is +- -- unnecessarily large). Note: for runtime units, use System.OS_Constants. +- -- SIZEOF_struct_file_attributes instead, which has the exact value. +- +- type File_Attributes is +- array (1 .. File_Attributes_Size) +- of System.Storage_Elements.Storage_Element; +- for File_Attributes'Alignment use Standard'Maximum_Alignment; +- +- Unknown_Attributes : constant File_Attributes := (others => 0); +- -- Will be initialized properly at elaboration (for efficiency later on, +- -- avoid function calls every time we want to reset the attributes). +- + end Osint;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34147 )
Change subject: crossgcc: Fix runtime initialization of a constant ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
Nice, more green scores on this patch :-P
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)