[073/203] Introduce class operation

Message ID 20210101214723.1784144-74-tom@tromey.com
State Superseded
Headers show
Series
  • Refactor expressions
Related show

Commit Message

Tom Tromey Jan. 1, 2021, 9:45 p.m.
This patch introduces class operation, the new base class for all
expression operations.

In the new approach, an operation is simply a class that presents a
certain interface.  Operations own their operands, and management is
done via unique_ptr.

The operation interface is largely ad hoc, based on the evolution of
expression handling in GDB.  Parts (for example,
evaluate_with_coercion) are probably redundant; however I took this
approach to try to avoid mixing different kinds of refactorings.

In some specific situations, rather than add a generic method across
the entire operation class hierarchy, I chose instead to use
dynamic_cast and specialized methods on certain concrete subclasses.
This will appear in some subsequent patches.

One goal of this work is to avoid the kinds of easy-to-make errors
that affected the old implementation.  To this end, some helper
subclasses are also added here.  These helpers automate the
implementation of the 'dump', 'uses_objfile', and 'constant_p'
methods.  Nearly every concrete operation that is subsequently added
will use these facilities.  (Note that the 'dump' implementation is
only outlined here, the body appears in the next patch.)

gdb/ChangeLog
2021-01-01  Tom Tromey  <tom@tromey.com>

	* expression.h (expr::operation): New class.
	(expr::make_operation): New function.
	(expr::operation_up): New typedef.
	* expop.h: New file.
	* eval.c (operation::evaluate_for_cast)
	(operation::evaluate_for_address, operation::evaluate_for_sizeof):
	New methods.
	* ax-gdb.c (operation::generate_ax): New method.
---
 gdb/ChangeLog    |  11 ++
 gdb/ax-gdb.c     |  26 ++++
 gdb/eval.c       |  36 ++++++
 gdb/expop.h      | 313 +++++++++++++++++++++++++++++++++++++++++++++++
 gdb/expression.h |  99 +++++++++++++++
 5 files changed, 485 insertions(+)
 create mode 100644 gdb/expop.h

-- 
2.26.2

Comments

Joel Brobecker Jan. 3, 2021, 7:09 a.m. | #1
Hi Tom,

> This patch introduces class operation, the new base class for all

> expression operations.

> 

> In the new approach, an operation is simply a class that presents a

> certain interface.  Operations own their operands, and management is

> done via unique_ptr.

> 

> The operation interface is largely ad hoc, based on the evolution of

> expression handling in GDB.  Parts (for example,

> evaluate_with_coercion) are probably redundant; however I took this

> approach to try to avoid mixing different kinds of refactorings.

> 

> In some specific situations, rather than add a generic method across

> the entire operation class hierarchy, I chose instead to use

> dynamic_cast and specialized methods on certain concrete subclasses.

> This will appear in some subsequent patches.

> 

> One goal of this work is to avoid the kinds of easy-to-make errors

> that affected the old implementation.  To this end, some helper

> subclasses are also added here.  These helpers automate the

> implementation of the 'dump', 'uses_objfile', and 'constant_p'

> methods.  Nearly every concrete operation that is subsequently added

> will use these facilities.  (Note that the 'dump' implementation is

> only outlined here, the body appears in the next patch.)

> 

> gdb/ChangeLog

> 2021-01-01  Tom Tromey  <tom@tromey.com>

> 

> 	* expression.h (expr::operation): New class.

> 	(expr::make_operation): New function.

> 	(expr::operation_up): New typedef.

> 	* expop.h: New file.

> 	* eval.c (operation::evaluate_for_cast)

> 	(operation::evaluate_for_address, operation::evaluate_for_sizeof):

> 	New methods.

> 	* ax-gdb.c (operation::generate_ax): New method.


Thanks for this!

I don't have much comment to share on this, at least not just yet
until I have had a chance to see this new API in action in the rest
of the patch series. I do have one small-tiny-mini suggestion:

It's about:

> +typedef std::unique_ptr<operation> operation_up;


FWIW, the connection between "up" and the rest of the declaration
wasn't immediately obvious to me. I think this is because "up"
is an english word, and spelled like that, all in lowercase,
that's really what my brain keeps analyzing it as first. It takes
an effort to adjust my thinking against this bias.

It's not a problem for me if we decide to keep this like that,
but what do you think about about renaming this to something like
"operation_uptr"? I think it'll make the intention more immediately
clear to the reader.

-- 
Joel
Carl Love via Gdb-patches Jan. 3, 2021, 1:55 p.m. | #2
Hi everyone

On 03/01/2021 07:09, Joel Brobecker wrote:
> Hi Tom,

>

>> 	* ax-gdb.c (operation::generate_ax): New method.

> Thanks for this!

>

> I don't have much comment to share on this, at least not just yet

> until I have had a chance to see this new API in action in the rest

> of the patch series. I do have one small-tiny-mini suggestion:

>

> It's about:

>

>> +typedef std::unique_ptr<operation> operation_up;

> FWIW, the connection between "up" and the rest of the declaration

> wasn't immediately obvious to me. I think this is because "up"

> is an english word, and spelled like that, all in lowercase,

> that's really what my brain keeps analyzing it as first. It takes

> an effort to adjust my thinking against this bias.

>

> It's not a problem for me if we decide to keep this like that,

> but what do you think about about renaming this to something like

> "operation_uptr"? I think it'll make the intention more immediately

> clear to the reader.

>

I am fairly new to this codebase, but as far as I can say, this sort of 
naming convention (the _up suffix for a typedefed std::unique_ptr) is 
already used here and there:

$ git grep 'typedef std::unique_ptr.*_up;'|wc -l
33

I have to admin that the first time I came across it it was not obvious 
to me either, but it would make sense to keep it this way for 
consistency (I guess).

BR
Lancelot.
Tom Tromey Feb. 10, 2021, 12:57 a.m. | #3
>>> +typedef std::unique_ptr<operation> operation_up;


>> FWIW, the connection between "up" and the rest of the declaration

>> wasn't immediately obvious to me. I think this is because "up"

>> is an english word, and spelled like that, all in lowercase,

>> that's really what my brain keeps analyzing it as first. It takes

>> an effort to adjust my thinking against this bias.


Lancelot> I am fairly new to this codebase, but as far as I can say, this sort
Lancelot> of naming convention (the _up suffix for a typedefed std::unique_ptr)
Lancelot> is already used here and there:

Lancelot> $ git grep 'typedef std::unique_ptr.*_up;'|wc -l
Lancelot> 33

Lancelot> I have to admin that the first time I came across it it was not
Lancelot> obvious to me either, but it would make sense to keep it this way for
Lancelot> consistency (I guess).

Yeah, IIRC the "_up" suffix (for "unique pointer") was introduced a
while back by Pedro; this is just following the existing naming
convention.

Tom

Patch

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index e18e968b852..728b21dfd6a 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -2270,6 +2270,32 @@  gen_expr (struct expression *exp, union exp_element **pc,
     }
 }
 
+namespace expr
+{
+
+void
+operation::generate_ax (struct expression *exp,
+			struct agent_expr *ax,
+			struct axs_value *value,
+			struct type *cast_type)
+{
+  if (constant_p ())
+    {
+      struct value *v = evaluate (nullptr, exp, EVAL_AVOID_SIDE_EFFECTS);
+      ax_const_l (ax, value_as_long (v));
+      value->kind = axs_rvalue;
+      value->type = check_typedef (value_type (v));
+    }
+  else
+    {
+      do_generate_ax (exp, ax, value, cast_type);
+      if (cast_type != nullptr)
+	gen_cast (ax, value, cast_type);
+    }
+}
+
+}
+
 /* This handles the middle-to-right-side of code generation for binary
    expressions, which is shared between regular binary operations and
    assign-modify (+= and friends) expressions.  */
diff --git a/gdb/eval.c b/gdb/eval.c
index 421cfb8e393..970e7d7c0bc 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -40,6 +40,7 @@ 
 #include "objfiles.h"
 #include "typeprint.h"
 #include <ctype.h>
+#include "expop.h"
 
 /* Prototypes for local functions.  */
 
@@ -3242,6 +3243,29 @@  evaluate_subexp_for_address (struct expression *exp, int *pos,
     }
 }
 
+namespace expr
+{
+
+value *
+operation::evaluate_for_cast (struct type *expect_type,
+			      struct expression *exp,
+			      enum noside noside)
+{
+  value *val = evaluate (expect_type, exp, noside);
+  if (noside == EVAL_SKIP)
+    return eval_skip_value (exp);
+  return value_cast (expect_type, val);
+}
+
+value *
+operation::evaluate_for_address (struct expression *exp, enum noside noside)
+{
+  value *val = evaluate (nullptr, exp, noside);
+  return evaluate_subexp_for_address_base (exp, noside, val);
+}
+
+}
+
 /* Evaluate like `evaluate_subexp' except coercing arrays to pointers.
    When used in contexts where arrays will be coerced anyway, this is
    equivalent to `evaluate_subexp' but much faster because it avoids
@@ -3430,6 +3454,18 @@  evaluate_subexp_for_sizeof (struct expression *exp, int *pos,
   return evaluate_subexp_for_sizeof_base (exp, type);
 }
 
+namespace expr
+{
+
+value *
+operation::evaluate_for_sizeof (struct expression *exp, enum noside noside)
+{
+  value *val = evaluate (nullptr, exp, EVAL_AVOID_SIDE_EFFECTS);
+  return evaluate_subexp_for_sizeof_base (exp, value_type (val));
+}
+
+}
+
 /* Evaluate a subexpression of EXP, at index *POS, and return a value
    for that subexpression cast to TO_TYPE.  Advance *POS over the
    subexpression.  */
diff --git a/gdb/expop.h b/gdb/expop.h
new file mode 100644
index 00000000000..1ce57a10d9a
--- /dev/null
+++ b/gdb/expop.h
@@ -0,0 +1,313 @@ 
+/* Definitions for expressions in GDB
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef EXPOP_H
+#define EXPOP_H
+
+#include "block.h"
+#include "c-lang.h"
+#include "cp-abi.h"
+#include "expression.h"
+#include "objfiles.h"
+#include "gdbsupport/traits.h"
+#include "gdbsupport/enum-flags.h"
+
+struct agent_expr;
+struct axs_value;
+
+namespace expr
+{
+
+/* The check_objfile overloads are used to check whether a particular
+   component of some operation references an objfile.  The passed-in
+   objfile will never be a debug objfile.  */
+
+/* See if EXP_OBJFILE matches OBJFILE.  */
+static inline bool
+check_objfile (struct objfile *exp_objfile, struct objfile *objfile)
+{
+  if (exp_objfile->separate_debug_objfile_backlink)
+    exp_objfile = exp_objfile->separate_debug_objfile_backlink;
+  return exp_objfile == objfile;
+}
+
+static inline bool 
+check_objfile (struct type *type, struct objfile *objfile)
+{
+  struct objfile *ty_objfile = TYPE_OBJFILE (type);
+  if (ty_objfile != nullptr)
+    return check_objfile (ty_objfile, objfile);
+  return false;
+}
+
+static inline bool 
+check_objfile (struct symbol *sym, struct objfile *objfile)
+{
+  return check_objfile (symbol_objfile (sym), objfile);
+}
+
+static inline bool 
+check_objfile (const struct block *block, struct objfile *objfile)
+{
+  return check_objfile (block_objfile (block), objfile);
+}
+
+static inline bool
+check_objfile (minimal_symbol *minsym, struct objfile *objfile)
+{
+  /* This may seem strange but minsyms are only used with an objfile
+     as well.  */
+  return false;
+}
+
+static inline bool
+check_objfile (internalvar *ivar, struct objfile *objfile)
+{
+  return false;
+}
+
+static inline bool
+check_objfile (const std::string &str, struct objfile *objfile)
+{
+  return false;
+}
+
+static inline bool 
+check_objfile (const operation_up &op, struct objfile *objfile)
+{
+  return op->uses_objfile (objfile);
+}
+
+static inline bool
+check_objfile (enum exp_opcode val, struct objfile *objfile)
+{
+  return false;
+}
+
+static inline bool
+check_objfile (ULONGEST val, struct objfile *objfile)
+{
+  return false;
+}
+
+template<typename T>
+static inline bool
+check_objfile (enum_flags<T> val, struct objfile *objfile)
+{
+  return false;
+}
+
+template<typename T>
+static inline bool 
+check_objfile (const std::vector<T> &collection, struct objfile *objfile)
+{
+  for (const auto &item : collection)
+    {
+      if (check_objfile (item, objfile))
+	return true;
+    }
+  return false;
+}
+
+template<typename S, typename T>
+static inline bool 
+check_objfile (const std::pair<S, T> &item, struct objfile *objfile)
+{
+  return (check_objfile (item.first, objfile)
+	  || check_objfile (item.second, objfile));
+}
+
+/* Base class for most concrete operations.  This class holds data,
+   specified via template parameters, and supplies generic
+   implementations of the 'dump' and 'uses_objfile' methods.  */
+template<typename... Arg>
+class tuple_holding_operation : public operation
+{
+public:
+
+  explicit tuple_holding_operation (Arg... args)
+    : m_storage (std::forward<Arg> (args)...)
+  {
+  }
+
+  DISABLE_COPY_AND_ASSIGN (tuple_holding_operation);
+
+  bool uses_objfile (struct objfile *objfile) const override
+  {
+    return do_check_objfile<0, Arg...> (objfile, m_storage);
+  }
+
+  void dump (struct ui_file *stream, int depth) const override
+  {
+    do_dump<0, Arg...> (stream, depth, m_storage);
+  }
+
+protected:
+
+  /* Storage for the data.  */
+  std::tuple<Arg...> m_storage;
+
+private:
+
+  /* do_dump does the work of dumping the data.  */
+  template<int I, typename... T>
+  typename std::enable_if<I == sizeof... (T), void>::type
+  do_dump (struct ui_file *stream, int depth, const std::tuple<T...> &value)
+    const
+  {
+  }
+
+  template<int I, typename... T>
+  typename std::enable_if<I < sizeof... (T), void>::type
+  do_dump (struct ui_file *stream, int depth, const std::tuple<T...> &value)
+    const
+  {
+    do_dump<I + 1, T...> (stream, depth, value);
+  }
+
+  /* do_check_objfile does the work of checking whether this object
+     refers to OBJFILE.  */
+  template<int I, typename... T>
+  typename std::enable_if<I == sizeof... (T), bool>::type
+  do_check_objfile (struct objfile *objfile, const std::tuple<T...> &value)
+    const
+  {
+    return false;
+  }
+
+  template<int I, typename... T>
+  typename std::enable_if<I < sizeof... (T), bool>::type
+  do_check_objfile (struct objfile *objfile, const std::tuple<T...> &value)
+    const
+  {
+    if (check_objfile (std::get<I> (value), objfile))
+      return true;
+    return do_check_objfile<I + 1, T...> (objfile, value);
+  }
+};
+
+/* The check_constant overloads are used to decide whether a given
+   concrete operation is a constant.  This is done by checking the
+   operands.  */
+
+static inline bool
+check_constant (const operation_up &item)
+{
+  return item->constant_p ();
+}
+
+static inline bool
+check_constant (struct minimal_symbol *msym)
+{
+  return false;
+}
+
+static inline bool
+check_constant (struct type *type)
+{
+  return true;
+}
+
+static inline bool
+check_constant (const struct block *block)
+{
+  return true;
+}
+
+static inline bool
+check_constant (const std::string &str)
+{
+  return true;
+}
+
+static inline bool
+check_constant (struct objfile *objfile)
+{
+  return true;
+}
+
+static inline bool
+check_constant (ULONGEST cst)
+{
+  return true;
+}
+
+static inline bool
+check_constant (struct symbol *sym)
+{
+  return (SYMBOL_CLASS (sym) == LOC_BLOCK
+	  || SYMBOL_CLASS (sym) == LOC_CONST
+	  || SYMBOL_CLASS (sym) == LOC_CONST_BYTES);
+}
+
+template<typename T>
+static inline bool
+check_constant (const std::vector<T> &collection)
+{
+  for (const auto &item : collection)
+    if (!check_constant (item))
+      return false;
+  return true;
+}
+
+template<typename S, typename T>
+static inline bool
+check_constant (const std::pair<S, T> &item)
+{
+  return check_constant (item.first) && check_constant (item.second);
+}
+
+/* Base class for concrete operations.  This class supplies an
+   implementation of 'constant_p' that works by checking the
+   operands.  */
+template<typename... Arg>
+class maybe_constant_operation
+  : public tuple_holding_operation<Arg...>
+{
+public:
+
+  using tuple_holding_operation<Arg...>::tuple_holding_operation;
+
+  bool constant_p () const override
+  {
+    return do_check_constant<0, Arg...> (this->m_storage);
+  }
+
+private:
+
+  template<int I, typename... T>
+  typename std::enable_if<I == sizeof... (T), bool>::type
+  do_check_constant (const std::tuple<T...> &value) const
+  {
+    return true;
+  }
+
+  template<int I, typename... T>
+  typename std::enable_if<I < sizeof... (T), bool>::type
+  do_check_constant (const std::tuple<T...> &value) const
+  {
+    if (!check_constant (std::get<I> (value)))
+      return false;
+    return do_check_constant<I + 1, T...> (value);
+  }
+};
+
+} /* namespace expr */
+
+#endif /* EXPOP_H */
diff --git a/gdb/expression.h b/gdb/expression.h
index 8c0bcc90f15..08a6424fdd2 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -89,6 +89,105 @@  enum noside
 				   does in many situations.  */
   };
 
+struct expression;
+struct agent_expr;
+struct axs_value;
+struct type;
+struct ui_file;
+
+namespace expr
+{
+
+class operation;
+typedef std::unique_ptr<operation> operation_up;
+
+/* Base class for an operation.  An operation is a single component of
+   an expression.  */
+
+class operation
+{
+protected:
+
+  operation () = default;
+  DISABLE_COPY_AND_ASSIGN (operation);
+
+public:
+
+  virtual ~operation () = default;
+
+  /* Evaluate this operation.  */
+  virtual value *evaluate (struct type *expect_type,
+			   struct expression *exp,
+			   enum noside noside) = 0;
+
+  /* Evaluate this operation in a context where C-like coercion is
+     needed.  */
+  virtual value *evaluate_with_coercion (struct expression *exp,
+					 enum noside noside)
+  {
+    return evaluate (nullptr, exp, noside);
+  }
+
+  /* Evaluate this expression in the context of a cast to
+     EXPECT_TYPE.  */
+  virtual value *evaluate_for_cast (struct type *expect_type,
+				    struct expression *exp,
+				    enum noside noside);
+
+  /* Evaluate this expression in the context of a sizeof
+     operation.  */
+  virtual value *evaluate_for_sizeof (struct expression *exp,
+				      enum noside noside);
+
+  /* Evaluate this expression in the context of an address-of
+     operation.  Must return the address.  */
+  virtual value *evaluate_for_address (struct expression *exp,
+				       enum noside noside);
+
+  /* True if this is a constant expression.  */
+  virtual bool constant_p () const
+  { return false; }
+
+  /* Return true if this operation uses OBJFILE (and will become
+     dangling when OBJFILE is unloaded), otherwise return false.
+     OBJFILE must not be a separate debug info file.  */
+  virtual bool uses_objfile (struct objfile *objfile) const
+  { return false; }
+
+  /* Generate agent expression bytecodes for this operation.  */
+  void generate_ax (struct expression *exp, struct agent_expr *ax,
+		    struct axs_value *value,
+		    struct type *cast_type = nullptr);
+
+  /* Return the opcode that is implemented by this operation.  */
+  virtual enum exp_opcode opcode () const = 0;
+
+  /* Print this operation to STREAM.  */
+  virtual void dump (struct ui_file *stream, int depth) const = 0;
+
+protected:
+
+  /* Called by generate_ax to do the work for this particular
+     operation.  */
+  virtual void do_generate_ax (struct expression *exp,
+			       struct agent_expr *ax,
+			       struct axs_value *value,
+			       struct type *cast_type)
+  {
+    error (_("Cannot translate to agent expression"));
+  }
+};
+
+/* A helper function for creating an operation_up, given a type.  */
+template<typename T, typename... Arg>
+operation_up
+make_operation (Arg... args)
+{
+  return operation_up (new T (std::forward<Arg> (args)...));
+}
+
+}
+
 union exp_element
   {
     enum exp_opcode opcode;