code review 83750044: compiler: Use backend interface to build function code.

compiler: Use backend interface to build function code.

[cmang, 2014-04-02 23:50:08]

Hello iant@golang.org (cc: gofrontend-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/gofrontend

[cmang, 2014-04-02 23:51:38]

On 2014/04/02 23:50:08, cmang wrote:
> Hello mailto:iant@golang.org (cc: mailto:gofrontend-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/gofrontend

Diff of gcc/go/go-gcc.cc:

Index: gcc/go/go-gcc.cc
======================================================================
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -406,6 +406,14 @@ class Gcc_backend : public Backend
   function_defer_statement(Bfunction* function, Bexpression* undefer,
                            Bexpression* defer, Location);
 
+  void
+  function_set_parameters(Bfunction* function, const std::vector<Bvariable*>&);
+
+  void
+  function_set_body(Bfunction* function, Bblock* code_block, Bblock* var_block,
+                    Bstatement* var_init, Bstatement* except_stmt,
+                    Bstatement* finally_stmt, Location);
+
  private:
   // Make a Bexpression from a tree.
   Bexpression*
@@ -2427,6 +2435,85 @@ Gcc_backend::function_defer_statement(Bfunction*
function, Bexpression* undefer,
   return this->make_statement(stmt_list);
 }
 
+// Set the parameters of a function FUNCTION to the PARAM_VARS created
+// when building the function.
+
+void
+Gcc_backend::function_set_parameters(Bfunction* function,
+                                     const std::vector<Bvariable*>& param_vars)
+{
+  tree func_tree = function->get_tree();
+  if (func_tree == error_mark_node)
+    return;
+
+  tree params = NULL_TREE;
+  tree *pp = &params;
+  for (std::vector<Bvariable*>::const_iterator pv = param_vars.begin();
+       pv != param_vars.end();
+       ++pv)
+    {
+      *pp = (*pv)->get_tree();
+      gcc_assert(*pp != error_mark_node);
+      pp = &DECL_CHAIN(*pp);
+    }
+  *pp = NULL_TREE;
+  DECL_ARGUMENTS(func_tree) = params;
+}
+
+
+// Set the function body for FUNCTION using the code in CODE_BLOCK.  Variables
+// that must be declared in this function will be declared in VAR_BLOCK and
+// initialized by the VAR_INIT statement.  If VAR_BLOCK is NULL, VAR_INIT
+// is a no-op.  If this function has a defer stack, the body will contain an
+// EXCEPT_STMT and FINALLY_STMT to handle the defer.
+// The body of the function should be of this form in C++:
+//   VAR_BLOCK;
+//   VAR_INIT;
+//   try {  CODE_BLOCK; } catch { EXCEPT_STMT; } finally { FINALLY_STMT }
+
+void
+Gcc_backend::function_set_body(Bfunction* function, Bblock* code_block,
+                               Bblock* var_block, Bstatement* var_init,
+                               Bstatement* except_stmt,
+                               Bstatement* finally_stmt, Location location)
+{
+  tree func_tree = function->get_tree();
+  tree code = code_block->get_tree();
+  tree var_decls = var_block == NULL ? NULL_TREE : var_block->get_tree();
+  tree var_init_tree = var_init->get_tree();
+  tree except_tree = except_stmt == NULL ? NULL_TREE : except_stmt->get_tree();
+  tree finally_tree = finally_stmt == NULL
+      ? NULL_TREE
+      : finally_stmt->get_tree();
+
+  if (func_tree == error_mark_node)
+    return;
+
+  if (code != NULL_TREE && code != error_mark_node)
+    {
+      if (var_init_tree != NULL_TREE && var_init_tree != error_mark_node)
+        code = build2_loc(location.gcc_location(), COMPOUND_EXPR,
+                          void_type_node, var_init_tree, code);
+      if (except_tree != NULL_TREE && except_tree != error_mark_node)
+        code = build2_loc(location.gcc_location(), TRY_CATCH_EXPR,
+                          void_type_node, code,
+                          build2_loc(location.gcc_location(), CATCH_EXPR,
+                                     void_type_node, NULL, except_tree));
+      if (finally_tree != NULL_TREE && finally_tree != error_mark_node)
+        code = build2_loc(location.gcc_location(), TRY_FINALLY_EXPR,
+                          void_type_node, code, finally_tree);
+    }
+
+  // Stick the code into the block we built for the receiver, if
+  // we built one.
+  if (var_decls != NULL_TREE && code != NULL_TREE && code != error_mark_node)
+    {
+      BIND_EXPR_BODY(var_decls) = code;
+      code = var_decls;
+    }
+  DECL_SAVED_TREE(func_tree) = code;
+}
+
 // The single backend.
 
 static Gcc_backend gcc_backend;

[iant, 2014-04-03 00:05:43]

https://codereview.appspot.com/83750044/diff/110001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/83750044/diff/110001/go/backend.h#newcode630
go/backend.h:630: // Set the parameters of a function FUNCTION to the PARAM_VARS
created
Change the return type to bool, so that it returns true on success, false on
failure.

I think the comment should be more like:

Record PARAM_VARS as the variables to use for the parameters of FUNCTION.  This
will only be called for a function definition (IS_DECLARATION passed as false
when Backend::function was called).

https://codereview.appspot.com/83750044/diff/110001/go/backend.h#newcode638
go/backend.h:638: // initialized by the VAR_INIT statement.  If VAR_BLOCK is
NULL, VAR_INIT
This doesn't quite make sense to me.  I would expect a function to have only one
block.  And I would expect that statements that refer to a variable would be
within the block that defines that variable.  So why do we have both code_block
and var_block?  And why isn't var_init part of var_block?

[cmang, 2014-04-03 20:38:51]

I added a new backend statement, exception_handler_statement, which made it
easier to separate concerns in function_set_body. PTAL.

Diff of gcc/go/go-gcc:

Index: gcc/go/go-gcc.cc
======================================================================
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -324,6 +324,10 @@ class Gcc_backend : public Backend
   Bstatement*
   statement_list(const std::vector<Bstatement*>&);
 
+  Bstatement*
+  exception_handler_statement(Bstatement* bstat, Bstatement* except_stmt,
+                              Bstatement* finally_stmt, Location);
+
   // Blocks.
 
   Bblock*
@@ -406,6 +410,12 @@ class Gcc_backend : public Backend
   function_defer_statement(Bfunction* function, Bexpression* undefer,
                            Bexpression* defer, Location);
 
+  bool
+  function_set_parameters(Bfunction* function, const std::vector<Bvariable*>&);
+
+  bool
+  function_set_body(Bfunction* function, Bstatement* code_stmt);
+
  private:
   // Make a Bexpression from a tree.
   Bexpression*
@@ -1727,6 +1737,40 @@ Gcc_backend::return_statement(Bfunction* bfunction,
   return this->make_statement(ret);
 }
 
+// Create a statement that attempts to execute BSTAT and calls EXCEPT_STMT if
an
+// error occurs.  EXCEPT_STMT may be NULL.  FINALLY_STMT may be NULL and if not
+// NULL, it will always be executed.  This is used for handling defers in Go
+// functions.  In C++, the resulting code is of this form:
+//   try { BSTAT; } catch { EXCEPT_STMT; } finally { FINALLY_STMT; }
+
+Bstatement*
+Gcc_backend::exception_handler_statement(Bstatement* bstat,
+                                         Bstatement* except_stmt,
+                                         Bstatement* finally_stmt,
+                                         Location location)
+{
+  tree stat_tree = bstat->get_tree();
+  tree except_tree = except_stmt == NULL ? NULL_TREE : except_stmt->get_tree();
+  tree finally_tree = finally_stmt == NULL
+      ? NULL_TREE
+      : finally_stmt->get_tree();
+
+  if (stat_tree == error_mark_node
+      || except_tree == error_mark_node
+      || finally_tree == error_mark_node)
+    return this->error_statement();
+
+  if (except_tree != NULL_TREE)
+    stat_tree = build2_loc(location.gcc_location(), TRY_CATCH_EXPR,
+                           void_type_node, stat_tree,
+                           build2_loc(location.gcc_location(), CATCH_EXPR,
+                                      void_type_node, NULL, except_tree));
+  if (finally_tree != NULL_TREE)
+    stat_tree = build2_loc(location.gcc_location(), TRY_FINALLY_EXPR,
+                           void_type_node, stat_tree, finally_tree);
+  return this->make_statement(stat_tree);
+}
+
 // If.
 
 Bstatement*
@@ -2427,6 +2471,46 @@ Gcc_backend::function_defer_statement(Bfunction*
function, Bexpression* undefer,
   return this->make_statement(stmt_list);
 }
 
+// Record PARAM_VARS as the variables to use for the parameters of FUNCTION.
+// This will only be called for a function definition.
+
+bool
+Gcc_backend::function_set_parameters(Bfunction* function,
+                                     const std::vector<Bvariable*>& param_vars)
+{
+  tree func_tree = function->get_tree();
+  if (func_tree == error_mark_node)
+    return false;
+
+  tree params = NULL_TREE;
+  tree *pp = &params;
+  for (std::vector<Bvariable*>::const_iterator pv = param_vars.begin();
+       pv != param_vars.end();
+       ++pv)
+    {
+      *pp = (*pv)->get_tree();
+      gcc_assert(*pp != error_mark_node);
+      pp = &DECL_CHAIN(*pp);
+    }
+  *pp = NULL_TREE;
+  DECL_ARGUMENTS(func_tree) = params;
+  return true;
+}
+
+// Set the function body for FUNCTION using the code in CODE_BLOCK.
+
+bool
+Gcc_backend::function_set_body(Bfunction* function, Bstatement* code_stmt)
+{
+  tree func_tree = function->get_tree();
+  tree code = code_stmt->get_tree();
+
+  if (func_tree == error_mark_node || code == error_mark_node)
+    return false;
+  DECL_SAVED_TREE(func_tree) = code;
+  return true;
+}
+
 // The single backend.
 
 static Gcc_backend gcc_backend;

https://codereview.appspot.com/83750044/diff/110001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/83750044/diff/110001/go/backend.h#newcode630
go/backend.h:630: // Set the parameters of a function FUNCTION to the PARAM_VARS
created
On 2014/04/03 00:05:43, iant wrote:
> Change the return type to bool, so that it returns true on success, false on
> failure.
> 
> I think the comment should be more like:
> 
> Record PARAM_VARS as the variables to use for the parameters of FUNCTION. 
This
> will only be called for a function definition (IS_DECLARATION passed as false
> when Backend::function was called).

Done.

https://codereview.appspot.com/83750044/diff/110001/go/backend.h#newcode638
go/backend.h:638: // initialized by the VAR_INIT statement.  If VAR_BLOCK is
NULL, VAR_INIT
On 2014/04/03 00:05:43, iant wrote:
> This doesn't quite make sense to me.  I would expect a function to have only
one
> block.  And I would expect that statements that refer to a variable would be
> within the block that defines that variable.  So why do we have both
code_block
> and var_block?  And why isn't var_init part of var_block?

This doesn't make any sense because this function is trying to do too much work
that isn't related to actually setting the code of the function body.

[iant, 2014-04-04 00:05:20]

https://codereview.appspot.com/83750044/diff/150001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/83750044/diff/150001/go/backend.h#newcode419
go/backend.h:419: // an error occurs. EXCEPT_STMT may be NULL.  FINALLY_STMT may
be NULL and if
s/an error/an exception/

[iant, 2014-04-04 00:10:03]

https://codereview.appspot.com/83750044/diff/150001/go/gogo.cc
File go/gogo.cc (right):

https://codereview.appspot.com/83750044/diff/150001/go/gogo.cc#newcode4230
go/gogo.cc:4230: return;
Add a go_assert(saw_errors()) if function_set_parameters fails.

https://codereview.appspot.com/83750044/diff/150001/go/gogo.cc#newcode4314
go/gogo.cc:4314: return;
go_assert(saw_errors());

[cmang, 2014-04-04 00:27:45]

https://codereview.appspot.com/83750044/diff/150001/go/backend.h
File go/backend.h (right):

https://codereview.appspot.com/83750044/diff/150001/go/backend.h#newcode419
go/backend.h:419: // an error occurs. EXCEPT_STMT may be NULL.  FINALLY_STMT may
be NULL and if
On 2014/04/04 00:05:21, iant wrote:
> s/an error/an exception/

Done.

https://codereview.appspot.com/83750044/diff/150001/go/gogo.cc
File go/gogo.cc (right):

https://codereview.appspot.com/83750044/diff/150001/go/gogo.cc#newcode4230
go/gogo.cc:4230: return;
On 2014/04/04 00:10:04, iant wrote:
> Add a go_assert(saw_errors()) if function_set_parameters fails.

Done.

https://codereview.appspot.com/83750044/diff/150001/go/gogo.cc#newcode4314
go/gogo.cc:4314: return;
On 2014/04/04 00:10:04, iant wrote:
> go_assert(saw_errors());

Done.

[iant, 2014-04-04 02:55:55]

LGTM

[iant, 2014-04-04 02:56:10]

*** Submitted as
https://code.google.com/p/gofrontend/source/detail?r=d59c79a574bc ***

compiler: Use backend interface to build function code.

R=iant
CC=gofrontend-dev
https://codereview.appspot.com/83750044

Committer: Ian Lance Taylor <iant@golang.org>