Implement namespace.follow for symbol types #79

xbauch · 2018-11-26T13:55:17Z

In driver.adb call Follow_Irep on every symbol in symbol table.
In ireps_generator.py reuse To_JSON to implement Follow_Irep: traverse the irep tree to follow I_Symbol_Type nodes. The irep table is not modified: we only change what ireps are referred to in some node (of the irep tree).

hannes-steffenhagen-diffblue

I'll have to take a closer look tomorrow, but I feel like the additions to the python code need some cleanup and some explanation for what is happening in there.

hannes-steffenhagen-diffblue · 2018-11-26T14:07:44Z

gnat2goto/driver/driver.adb

@@ -51,6 +52,8 @@ package body Driver is
   procedure Translate_Standard_Types;
   procedure Initialize_CProver_Internal_Variables (Start_Body : Irep);
   procedure Add_CProver_Internal_Symbols;
+   procedure Follow_Type_Declarations (Old_Table : Symbol_Table;
+                                       New_Table : in out Symbol_Table);


Why do we create a new symbol table rather than following symbols in the old one?

Also, if you're not reading fron New_Table it should just be out rather than in out I think.

Well I'm rewriting the symbols and I don't think I can modify a hash map while iterating over it. The second part: yes, will rewrite to just out.

hannes-steffenhagen-diffblue · 2018-11-26T14:09:42Z

gnat2goto/driver/driver.adb

+      for Sym_Iter in Old_Table.Iterate loop
+         declare
+            Ignored : Boolean;
+            Unused  : Symbol_Maps.Cursor;


Unused_Inserted and Unused_Position?

I copied it from a function above this one, but ok, this sounds more descriptive.

hannes-steffenhagen-diffblue · 2018-11-26T14:10:44Z

gnat2goto/driver/tree_walk.adb

@@ -1835,6 +1835,8 @@ package body Tree_Walk is
         when E_Record_Subtype => Do_Itype_Record_Subtype (N),
         when E_Signed_Integer_Type => Do_Itype_Integer_Type (N),
         when E_Floating_Point_Type => Create_Dummy_Irep,
+         when E_Anonymous_Access_Type => Make_Pointer_Type


This appears to be unrelated to the follow functionality?

Yes, although it is required to make the primitive_pointer test work. So, separate commit or separate PR?

Preference for me would be separate PR but separate commit at the least

hannes-steffenhagen-diffblue · 2018-11-26T14:13:05Z

gnat2goto/ireps/ireps_generator.py

        write(s, "")
-        write(s, "with GNATCOLL.JSON; use GNATCOLL.JSON;")  # JSON
+        write(s, "with GNATCOLL.JSON;     use GNATCOLL.JSON;")  # JSON


Unrelated formatting change? I don't really mind this being part of this PR but could you do it in a separate commit?

Shouldn't be here, will remove.

hannes-steffenhagen-diffblue · 2018-11-26T14:17:09Z

gnat2goto/driver/driver.adb

+            Value : constant Irep := Current_Symbol.Value;
+         begin
+            Modified_Symbol.SymType :=
+                    Follow_Irep (SymType, Follow_Symbol'Access);


I think this might be easier to follow if we just passed the Old_Table as a parameter to Follow_Irep and did the Follow_Symbol logic over there? Or is there some other reason why we have to use the function pointer here?

That was the original plan but I couldn't figure out how to declare Symbol_Table in Ireps.adb. I always ended up with circular dependencies.

hannes-steffenhagen-diffblue · 2018-11-26T14:20:42Z

gnat2goto/ireps/ireps_generator.py

@@ -1596,6 +1612,15 @@ def generate_code(self, optimize, schema_file_names):
        continuation(b)
        write(b, "")

+        write(b, "function Follow_Irep (L : Irep_List;")
+#        with indent(b):


commented out code?

Sorry, will be removed.

hannes-steffenhagen-diffblue · 2018-11-26T14:22:11Z

gnat2goto/ireps/ireps_generator.py

@@ -1816,6 +1841,98 @@ def generate_code(self, optimize, schema_file_names):
        write(b, "end To_JSON;")
        write(b, "")

+        write_comment_block(b, "Follow_Irep")
+        write(b, "function Follow_Irep (I : Irep;")


I feel this bit is complicated enough to warrant being factored out into its own function

You mean another python script?

I'm also uncomfortable with how big this new code inserted is. I understand it's codegen stuff, which normally is long, but I'm also under the impression that it has to be factored out into a function, and then called here.

NlightNFotis

LGTM, just an organisational comment. I'm not too fussed about it. Good job.

NlightNFotis · 2018-11-27T11:06:27Z

gnat2goto/ireps/ireps_generator.py

@@ -1816,6 +1841,98 @@ def generate_code(self, optimize, schema_file_names):
        write(b, "end To_JSON;")
        write(b, "")

+        write_comment_block(b, "Follow_Irep")
+        write(b, "function Follow_Irep (I : Irep;")


I'm also uncomfortable with how big this new code inserted is. I understand it's codegen stuff, which normally is long, but I'm also under the impression that it has to be factored out into a function, and then called here.

NlightNFotis · 2018-12-04T13:33:45Z

@xbauch The tests show that gnat2goto now crashes on (almost?) all tests? Was it like this before, and/or is it depending on another PR going in?

xbauch · 2018-12-04T16:19:22Z

@NlightNFotis better?

hannes-steffenhagen-diffblue · 2018-12-04T19:04:53Z

Some of the CI failures may be fixed by a rebase (which you'll need anyway)

Context: array copies are called via a wrapper function - this took the array length as a parameter. Now this is set directly (the parameter is kept and assigned to a dummy var to minimise code change as we might want to revert this in future. The dummy var is to prevent gnat error over unused var.

The TO_JSON part at least.

In driver.adb call Follow_Irep on every symbol in symbol table. In ireps_generator.py reuse to_json to implement follow_irep: traverse the irep tree to follow I_Symbol_Type ireps.

NlightNFotis

This looks to be in a good shape now.

xbauch force-pushed the testsuite/primitive_pointer branch from 276b48f to 80cd0e2 Compare November 26, 2018 14:04

hannes-steffenhagen-diffblue reviewed Nov 26, 2018

View reviewed changes

NlightNFotis reviewed Nov 27, 2018

View reviewed changes

xbauch force-pushed the testsuite/primitive_pointer branch from 80cd0e2 to d5f6f99 Compare November 28, 2018 13:30

xbauch mentioned this pull request Nov 28, 2018

Testsuite/arrays #83

Merged

xbauch force-pushed the testsuite/primitive_pointer branch 2 times, most recently from 2b87045 to 79445d0 Compare December 4, 2018 13:11

xbauch force-pushed the testsuite/primitive_pointer branch from 92cec80 to 724d839 Compare December 5, 2018 09:05

Petr Bauch added 2 commits December 6, 2018 14:26

Refactor ireps_generator.py

c0fd87a

The TO_JSON part at least.

Implement namespace.follow for symbol types

5ff36f8

In driver.adb call Follow_Irep on every symbol in symbol table. In ireps_generator.py reuse to_json to implement follow_irep: traverse the irep tree to follow I_Symbol_Type ireps.

xbauch force-pushed the testsuite/primitive_pointer branch 2 times, most recently from 54cc8c5 to 72a22cd Compare December 6, 2018 15:02

NlightNFotis approved these changes Dec 6, 2018

View reviewed changes

chrisr-diffblue approved these changes Dec 6, 2018

View reviewed changes

Fix testsuite/primitive_pointer

39df83c

xbauch force-pushed the testsuite/primitive_pointer branch from d2dae7e to 39df83c Compare December 6, 2018 15:56

xbauch merged commit 8718af2 into diffblue:master Dec 6, 2018

sonodtt mentioned this pull request Dec 11, 2018

Replace cpp new with malloc call sonodtt/gnat2goto#2

Open

xbauch deleted the testsuite/primitive_pointer branch March 14, 2019 10:24

Implement namespace.follow for symbol types #79

Implement namespace.follow for symbol types #79

Uh oh!

Conversation

xbauch commented Nov 26, 2018

Uh oh!

hannes-steffenhagen-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NlightNFotis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NlightNFotis commented Dec 4, 2018

Uh oh!

xbauch commented Dec 4, 2018

Uh oh!

hannes-steffenhagen-diffblue commented Dec 4, 2018

Uh oh!

NlightNFotis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!