[PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs

Tom Tromey-2
This is a new version of the series to add -fsanitize=undefined to the
build.

It's only added to gdb, though it occurred to me later that it would
probably be better to add it to all the libraries as well.

This version addresses the review comments, and in particular adds
documentation in patch #10 about performance.  It also fixes a bug
observed on the S390 builds in patch #2.

Regression tested by the buildbot.

Tom


Reply | Threaded
Open this post in threaded view
|

[PATCH v2 01/10] Do not pass NULL to memcpy

Tom Tromey-2
-fsanitize=undefined pointed out a spot that passes NULL to memcpy,
which is undefined behavior according to the C standard.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * namespace.c (add_using_directive): Don't pass NULL to memcpy.
---
 gdb/ChangeLog   | 4 ++++
 gdb/namespace.c | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/namespace.c b/gdb/namespace.c
index be998d9d49..85c0c4b14d 100644
--- a/gdb/namespace.c
+++ b/gdb/namespace.c
@@ -111,8 +111,9 @@ add_using_directive (struct using_direct **using_directives,
   else
     newobj->declaration = declaration;
 
-  memcpy (newobj->excludes, excludes.data (),
-  excludes.size () * sizeof (*newobj->excludes));
+  if (!excludes.empty ())
+    memcpy (newobj->excludes, excludes.data (),
+    excludes.size () * sizeof (*newobj->excludes));
   newobj->excludes[excludes.size ()] = NULL;
 
   newobj->next = *using_directives;
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector

Tom Tromey-2
In reply to this post by Tom Tromey-2
This changes dwarf2_frame_state_reg_info::reg to be a std::vector.
This avoids undefined behavior in the copy constructor when the
original object does not have any registers.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * dwarf2-frame.h (dwarf2_frame_state_reg_info)
        <~dwarf2_frame_state_reg_info>: Update.
        <dwarf2_frame_state_reg_info>: Update.
        <alloc_regs>: Add assertion.  Update.
        <reg>: Now a std::vector.
        <num_regs>: Remove.
        <swap>: Update.
        * dwarf2-frame.c (dwarf2_restore_rule, execute_cfa_program)
        (execute_cfa_program_test, dwarf2_frame_cache): Update.
---
 gdb/ChangeLog      | 12 ++++++++++++
 gdb/dwarf2-frame.c | 28 ++++++++++++++--------------
 gdb/dwarf2-frame.h | 31 +++++++++----------------------
 3 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f7dc820f4d..118bc11217 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -204,14 +204,13 @@ dwarf2_restore_rule (struct gdbarch *gdbarch, ULONGEST reg_num,
 {
   ULONGEST reg;
 
-  gdb_assert (fs->initial.reg);
   reg = dwarf2_frame_adjust_regnum (gdbarch, reg_num, eh_frame_p);
   fs->regs.alloc_regs (reg + 1);
 
   /* Check if this register was explicitly initialized in the
   CIE initial instructions.  If not, default the rule to
   UNSPECIFIED.  */
-  if (reg < fs->initial.num_regs)
+  if (reg < fs->initial.reg.size ())
     fs->regs.reg[reg] = fs->initial.reg[reg];
   else
     fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
@@ -602,7 +601,7 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
  }
     }
 
-  if (fs->initial.reg == NULL)
+  if (fs->initial.reg.empty ())
     {
       /* Don't allow remember/restore between CIE and FDE programs.  */
       delete fs->regs.prev;
@@ -653,12 +652,12 @@ execute_cfa_program_test (struct gdbarch *gdbarch)
   auto r1 = dwarf2_frame_adjust_regnum (gdbarch, 1, fde.eh_frame_p);
   auto r2 = dwarf2_frame_adjust_regnum (gdbarch, 2, fde.eh_frame_p);
 
-  SELF_CHECK (fs.regs.num_regs == (std::max (r1, r2) + 1));
+  SELF_CHECK (fs.regs.reg.size () == (std::max (r1, r2) + 1));
 
   SELF_CHECK (fs.regs.reg[r2].how == DWARF2_FRAME_REG_SAVED_OFFSET);
   SELF_CHECK (fs.regs.reg[r2].loc.offset == -4);
 
-  for (auto i = 0; i < fs.regs.num_regs; i++)
+  for (auto i = 0; i < fs.regs.reg.size (); i++)
     if (i != r2)
       SELF_CHECK (fs.regs.reg[i].how == DWARF2_FRAME_REG_UNSPECIFIED);
 
@@ -1097,7 +1096,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   {
     int column; /* CFI speak for "register number".  */
 
-    for (column = 0; column < fs.regs.num_regs; column++)
+    for (column = 0; column < fs.regs.reg.size (); column++)
       {
  /* Use the GDB register number as the destination index.  */
  int regnum = dwarf_reg_to_regnum (gdbarch, column);
@@ -1140,8 +1139,9 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
  if (cache->reg[regnum].how == DWARF2_FRAME_REG_RA
     || cache->reg[regnum].how == DWARF2_FRAME_REG_RA_OFFSET)
   {
-    struct dwarf2_frame_state_reg *retaddr_reg =
-      &fs.regs.reg[fs.retaddr_column];
+    const std::vector<struct dwarf2_frame_state_reg> &regs
+      = fs.regs.reg;
+    ULONGEST retaddr_column = fs.retaddr_column;
 
     /* It seems rather bizarre to specify an "empty" column as
                the return adress column.  However, this is exactly
@@ -1150,14 +1150,14 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
                register corresponding to the return address column.
                Incidentally, that's how we should treat a return
                address column specifying "same value" too.  */
-    if (fs.retaddr_column < fs.regs.num_regs
- && retaddr_reg->how != DWARF2_FRAME_REG_UNSPECIFIED
- && retaddr_reg->how != DWARF2_FRAME_REG_SAME_VALUE)
+    if (fs.retaddr_column < fs.regs.reg.size ()
+ && regs[retaddr_column].how != DWARF2_FRAME_REG_UNSPECIFIED
+ && regs[retaddr_column].how != DWARF2_FRAME_REG_SAME_VALUE)
       {
  if (cache->reg[regnum].how == DWARF2_FRAME_REG_RA)
-  cache->reg[regnum] = *retaddr_reg;
+  cache->reg[regnum] = regs[retaddr_column];
  else
-  cache->retaddr_reg = *retaddr_reg;
+  cache->retaddr_reg = regs[retaddr_column];
       }
     else
       {
@@ -1176,7 +1176,7 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
       }
   }
 
-  if (fs.retaddr_column < fs.regs.num_regs
+  if (fs.retaddr_column < fs.regs.reg.size ()
       && fs.regs.reg[fs.retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
     cache->undefined_retaddr = 1;
 
diff --git a/gdb/dwarf2-frame.h b/gdb/dwarf2-frame.h
index 52316e5e16..b89f931651 100644
--- a/gdb/dwarf2-frame.h
+++ b/gdb/dwarf2-frame.h
@@ -98,19 +98,14 @@ struct dwarf2_frame_state_reg_info
   ~dwarf2_frame_state_reg_info ()
   {
     delete prev;
-    xfree (reg);
   }
 
   /* Copy constructor.  */
   dwarf2_frame_state_reg_info (const dwarf2_frame_state_reg_info &src)
-    : num_regs (src.num_regs), cfa_offset (src.cfa_offset),
+    : reg (src.reg), cfa_offset (src.cfa_offset),
       cfa_reg (src.cfa_reg), cfa_how (src.cfa_how), cfa_exp (src.cfa_exp),
       prev (src.prev)
   {
-    size_t size = src.num_regs * sizeof (struct dwarf2_frame_state_reg);
-
-    reg = (struct dwarf2_frame_state_reg *) xmalloc (size);
-    memcpy (reg, src.reg, size);
   }
 
   /* Assignment operator for both move-assignment and copy-assignment.  */
@@ -123,33 +118,26 @@ struct dwarf2_frame_state_reg_info
 
   /* Move constructor.  */
   dwarf2_frame_state_reg_info (dwarf2_frame_state_reg_info &&rhs) noexcept
-    : reg (rhs.reg), num_regs (rhs.num_regs), cfa_offset (rhs.cfa_offset),
+    : reg (std::move (rhs.reg)), cfa_offset (rhs.cfa_offset),
       cfa_reg (rhs.cfa_reg), cfa_how (rhs.cfa_how), cfa_exp (rhs.cfa_exp),
       prev (rhs.prev)
   {
     rhs.prev = nullptr;
-    rhs.reg = nullptr;
   }
 
-/* Assert that the register set RS is large enough to store gdbarch_num_regs
-   columns.  If necessary, enlarge the register set.  */
+  /* If necessary, enlarge the register set to hold NUM_REGS_REQUESTED
+     registers.  */
   void alloc_regs (int num_regs_requested)
   {
-    if (num_regs_requested <= num_regs)
-      return;
+    gdb_assert (num_regs_requested > 0);
 
-    size_t size = sizeof (struct dwarf2_frame_state_reg);
-
-    reg = (struct dwarf2_frame_state_reg *)
-      xrealloc (reg, num_regs_requested * size);
+    if (num_regs_requested <= reg.size ())
+      return;
 
-    /* Initialize newly allocated registers.  */
-    memset (reg + num_regs, 0, (num_regs_requested - num_regs) * size);
-    num_regs = num_regs_requested;
+    reg.resize (num_regs_requested);
   }
 
-  struct dwarf2_frame_state_reg *reg = NULL;
-  int num_regs = 0;
+  std::vector<struct dwarf2_frame_state_reg> reg;
 
   LONGEST cfa_offset = 0;
   ULONGEST cfa_reg = 0;
@@ -166,7 +154,6 @@ private:
     using std::swap;
 
     swap (lhs.reg, rhs.reg);
-    swap (lhs.num_regs, rhs.num_regs);
     swap (lhs.cfa_offset, rhs.cfa_offset);
     swap (lhs.cfa_reg, rhs.cfa_reg);
     swap (lhs.cfa_how, rhs.cfa_how);
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 03/10] Use unsigned as base type for some enums

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined complains about using operator~ on various enum
types that are used with DEF_ENUM_FLAGS_TYPE.  This patch fixes these
problems by explicitly setting the base type for these enums to
unsigned.  It also adds a static assert to enum_flags to ensure that
future enums used this way have an unsigned underlying type.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * common/enum-flags.h (enum_flags::operator~): Add static assert.
        * symfile-add-flags.h (enum symfile_add_flag): Use unsigned as
        base type.
        * objfile-flags.h (enum objfile_flag): Use unsigned as base type.
        * gdbtypes.h (enum type_instance_flag_value): Use unsigned as base
        type.
        * c-lang.h (enum c_string_type_values): Use unsigned as base
        type.
        * btrace.h (enum btrace_thread_flag): Use unsigned as base type.
---
 gdb/ChangeLog           | 12 ++++++++++++
 gdb/btrace.h            |  2 +-
 gdb/c-lang.h            |  2 +-
 gdb/common/enum-flags.h |  1 +
 gdb/gdbtypes.h          |  2 +-
 gdb/objfile-flags.h     |  2 +-
 gdb/symfile-add-flags.h |  2 +-
 7 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/gdb/btrace.h b/gdb/btrace.h
index bfb0b9f278..0448bd6d49 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -228,7 +228,7 @@ struct btrace_call_history
 };
 
 /* Branch trace thread flags.  */
-enum btrace_thread_flag
+enum btrace_thread_flag : unsigned
 {
   /* The thread is to be stepped forwards.  */
   BTHR_STEP = (1 << 0),
diff --git a/gdb/c-lang.h b/gdb/c-lang.h
index ae17abd20f..f9eab04730 100644
--- a/gdb/c-lang.h
+++ b/gdb/c-lang.h
@@ -35,7 +35,7 @@ struct parser_state;
 /* The various kinds of C string and character.  Note that these
    values are chosen so that they may be or'd together in certain
    ways.  */
-enum c_string_type_values
+enum c_string_type_values : unsigned
   {
     /* An ordinary string: "value".  */
     C_STRING = 0,
diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h
index 82568a5a3d..dad59cec86 100644
--- a/gdb/common/enum-flags.h
+++ b/gdb/common/enum-flags.h
@@ -164,6 +164,7 @@ public:
   }
   enum_flags operator~ () const
   {
+    gdb_static_assert (std::is_unsigned<underlying_type>::value);
     return (enum_type) ~underlying_value ();
   }
 
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 32b58dcc61..a115857c0a 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -193,7 +193,7 @@ enum type_code
 /* * Some bits for the type's instance_flags word.  See the macros
    below for documentation on each bit.  */
 
-enum type_instance_flag_value
+enum type_instance_flag_value : unsigned
 {
   TYPE_INSTANCE_FLAG_CONST = (1 << 0),
   TYPE_INSTANCE_FLAG_VOLATILE = (1 << 1),
diff --git a/gdb/objfile-flags.h b/gdb/objfile-flags.h
index aeaa8fbc04..6f5760d021 100644
--- a/gdb/objfile-flags.h
+++ b/gdb/objfile-flags.h
@@ -25,7 +25,7 @@
 /* Defines for the objfile flags field.  Defined in a separate file to
    break circular header dependencies.  */
 
-enum objfile_flag
+enum objfile_flag : unsigned
   {
     /* When an object file has its functions reordered (currently
        Irix-5.2 shared libraries exhibit this behaviour), we will need
diff --git a/gdb/symfile-add-flags.h b/gdb/symfile-add-flags.h
index 3c07513e39..35241fe4a0 100644
--- a/gdb/symfile-add-flags.h
+++ b/gdb/symfile-add-flags.h
@@ -26,7 +26,7 @@
    symbol_file_add, etc.  Defined in a separate file to break circular
    header dependencies.  */
 
-enum symfile_add_flag
+enum symfile_add_flag : unsigned
   {
     /* Be chatty about what you are doing.  */
     SYMFILE_VERBOSE = 1 << 1,
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 04/10] Avoid undefined behavior in extract_integer

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined showed that extract_integer could left-shift a
negative value, which is undefined.  This patch fixes the problem by
doing all the work in an unsigned type.  This relies on
implementation-defined behavior, but I tend to think we are on safe
ground there.  (Also, if need be, violations of this could probably be
detected, either by configure or by a static_assert.)

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * findvar.c (extract_integer): Do work in an unsigned type.
---
 gdb/ChangeLog | 4 ++++
 gdb/findvar.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 9256833ab6..be6c9d6f60 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -50,7 +50,7 @@ template<typename T, typename>
 T
 extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
 {
-  T retval = 0;
+  typename std::make_unsigned<T>::type retval = 0;
   const unsigned char *p;
   const unsigned char *startaddr = addr;
   const unsigned char *endaddr = startaddr + len;
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 05/10] Avoid undefined behavior in read_subrange_type

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined pointed out an undefined shift of a negative
value in read_subrange_type.  The fix is to do the work in an unsigned
type, where this is defined.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * dwarf2read.c (read_subrange_type): Make "negative_mask"
        unsigned.
---
 gdb/ChangeLog    | 5 +++++
 gdb/dwarf2read.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 4a35e389e9..4013c199da 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17709,7 +17709,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
   int low_default_is_valid;
   int high_bound_is_count = 0;
   const char *name;
-  LONGEST negative_mask;
+  ULONGEST negative_mask;
 
   orig_base_type = die_type (die, cu);
   /* If ORIG_BASE_TYPE is a typedef, it will not be TYPE_UNSIGNED,
@@ -17842,7 +17842,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
      the bounds as signed, and thus sign-extend their values, when
      the base type is signed.  */
   negative_mask =
-    -((LONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));
+    -((ULONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));
   if (low.kind == PROP_CONST
       && !TYPE_UNSIGNED (base_type) && (low.data.const_val & negative_mask))
     low.data.const_val |= negative_mask;
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 06/10] Avoid undefined behavior in parse_number

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined pointed out that c-exp.y relied on undefined
behavior here:

      if (c != 'l' && c != 'u')
        n *= base;

...when a large hex constant "just fit" into a LONGEST, causing the
high bit to be set.

This fixes the problem by having the function work in an unsigned
type.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * c-exp.y (parse_number): Work in unsigned.  Remove casts.
---
 gdb/ChangeLog |  4 ++++
 gdb/c-exp.y   | 10 ++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 0326ee090e..09e31d2283 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1760,10 +1760,8 @@ static int
 parse_number (struct parser_state *par_state,
       const char *buf, int len, int parsed_float, YYSTYPE *putithere)
 {
-  /* FIXME: Shouldn't these be unsigned?  We don't deal with negative values
-     here, and we do kind of silly things like cast to unsigned.  */
-  LONGEST n = 0;
-  LONGEST prevn = 0;
+  ULONGEST n = 0;
+  ULONGEST prevn = 0;
   ULONGEST un;
 
   int i = 0;
@@ -1922,7 +1920,7 @@ parse_number (struct parser_state *par_state,
  on 0x123456789 when LONGEST is 32 bits.  */
       if (c != 'l' && c != 'u' && n != 0)
  {
-  if ((unsigned_p && (ULONGEST) prevn >= (ULONGEST) n))
+  if (unsigned_p && prevn >= n)
     error (_("Numeric constant too large."));
  }
       prevn = n;
@@ -1940,7 +1938,7 @@ parse_number (struct parser_state *par_state,
      the case where it is we just always shift the value more than
      once, with fewer bits each time.  */
 
-  un = (ULONGEST)n >> 2;
+  un = n >> 2;
   if (long_p == 0
       && (un >> (gdbarch_int_bit (parse_gdbarch (par_state)) - 2)) == 0)
     {
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined pointed out that read_signed_leb128 had an
undefined left-shift when processing the final byte of a 64-bit leb:

    runtime error: left shift of 127 by 63 places cannot be represented in type 'long int'

and an undefined negation:

    runtime error: negation of -9223372036854775808 cannot be represented in type 'long int'; cast to an unsigned type to negate this value to itself

Both of these problems are readily avoided by havinng
read_signed_leb128 work in an unsigned type, and then casting to the
signed type at the return.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * dwarf2read.c (read_signed_leb128): Work in ULONGEST.
---
 gdb/ChangeLog    | 4 ++++
 gdb/dwarf2read.c | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 4013c199da..7004299a91 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -19627,7 +19627,7 @@ static LONGEST
 read_signed_leb128 (bfd *abfd, const gdb_byte *buf,
     unsigned int *bytes_read_ptr)
 {
-  LONGEST result;
+  ULONGEST result;
   int shift, num_read;
   unsigned char byte;
 
@@ -19639,7 +19639,7 @@ read_signed_leb128 (bfd *abfd, const gdb_byte *buf,
       byte = bfd_get_8 (abfd, buf);
       buf++;
       num_read++;
-      result |= ((LONGEST) (byte & 127) << shift);
+      result |= ((ULONGEST) (byte & 127) << shift);
       shift += 7;
       if ((byte & 128) == 0)
  {
@@ -19647,7 +19647,7 @@ read_signed_leb128 (bfd *abfd, const gdb_byte *buf,
  }
     }
   if ((shift < 8 * sizeof (result)) && (byte & 0x40))
-    result |= -(((LONGEST) 1) << shift);
+    result |= -(((ULONGEST) 1) << shift);
   *bytes_read_ptr = num_read;
   return result;
 }
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 08/10] Avoid undefined behavior in ada_operator_length

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined pointed out this error:

    runtime error: load of value 2887952, which is not a valid value for type 'exp_opcode'

This happens in gdb.ada/complete.exp when processing "complete p
my_glob".  This does not parse, so the Ada parser throws an exception;
but then the code in parse_exp_in_context_1 accepts the expression
anyway.  However, as no elements have been written to the expression,
undefined behavior results.

The fix is to notice this case in parse_exp_in_context_1.  This patch
also adds an assertion to prefixify_expression to enforce this
pre-existing constraint.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * parse.c (prefixify_expression): Add assert.
        (parse_exp_in_context_1): Throw exception if the expression is
        empty.
---
 gdb/ChangeLog | 8 +++++++-
 gdb/parse.c   | 6 +++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/gdb/parse.c b/gdb/parse.c
index 163852cb84..8a128a74d7 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -792,6 +792,7 @@ copy_name (struct stoken token)
 int
 prefixify_expression (struct expression *expr)
 {
+  gdb_assert (expr->nelts > 0);
   int len = sizeof (struct expression) + EXP_ELEM_TO_BYTES (expr->nelts);
   struct expression *temp;
   int inpos = expr->nelts, outpos = 0;
@@ -1205,7 +1206,10 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
     }
   CATCH (except, RETURN_MASK_ALL)
     {
-      if (! parse_completion)
+      /* If parsing for completion, allow this to succeed; but if no
+ expression elements have been written, then there's nothing
+ to do, so fail.  */
+      if (! parse_completion || ps.expout_ptr == 0)
  throw_exception (except);
     }
   END_CATCH
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 09/10] Avoid undefined behavior in expression dumping

Tom Tromey-2
In reply to this post by Tom Tromey-2
-fsanitize=undefined pointed out undefined behavior in
dump_raw_expression like:

    runtime error: load of value 2887952, which is not a valid value for type 'exp_opcode'

dump_raw_expression will try to print the opcode for each element of
the expression, even when it is not valid.  To allow this, but have it
avoid undefined behavior, this patch sets the underlying type of enum
exp_opcode, and arranges for op_name to handle invalid opcodes more
nicely.

Before this patch, debug-expr.exp shows:

Dump of expression @ 0x60f000007750, before conversion to prefix form:
        Language c, 8 elements, 16 bytes each.
        Index                Opcode         Hex Value  String Value
            0               OP_TYPE  89  Y...............
   <unknown 3851920>  107820862850704  ..:..b..........
            2               OP_TYPE  89  Y...............
            3          OP_VAR_VALUE  40  (...............
            4     <unknown 2807568>  107820861806352  ..*..b..........
            5     <unknown 2806368>  107820861805152  `.*..b..........
            6          OP_VAR_VALUE  40  (...............
            7      UNOP_MEMVAL_TYPE  57  9...............

Afterward, the output is:

Dump of expression @ 0x4820f90, before conversion to prefix form:
        Language c, 8 elements, 16 bytes each.
        Index                Opcode         Hex Value  String Value
            0               OP_TYPE  89  Y...............
            1   unknown opcode: 176  75444400  .0..............
            2               OP_TYPE  89  Y...............
            3          OP_VAR_VALUE  40  (...............
            4               OP_BOOL  74616912  P.r.............
            5   unknown opcode: 128  74615680  ..r.............
            6          OP_VAR_VALUE  40  (...............
            7      UNOP_MEMVAL_TYPE  57  9...............

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * expression.h (enum exp_opcode): Use uint8_t as base type.
        * expprint.c (op_name): Handle invalid opcodes.
---
 gdb/ChangeLog    | 5 +++++
 gdb/expprint.c   | 7 +++++++
 gdb/expression.h | 2 +-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/expprint.c b/gdb/expprint.c
index d6ed41253e..e87b3b709b 100644
--- a/gdb/expprint.c
+++ b/gdb/expprint.c
@@ -687,6 +687,13 @@ static int dump_subexp_body (struct expression *exp, struct ui_file *, int);
 const char *
 op_name (struct expression *exp, enum exp_opcode opcode)
 {
+  if (opcode >= OP_UNUSED_LAST)
+    {
+      char *cell = get_print_cell ();
+      xsnprintf (cell, PRINT_CELL_SIZE, "unknown opcode: %u",
+ unsigned (opcode));
+      return cell;
+    }
   return exp->language_defn->la_exp_desc->op_name (opcode);
 }
 
diff --git a/gdb/expression.h b/gdb/expression.h
index bc7625f984..a5cb4c678e 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -39,7 +39,7 @@
    and skip that many.  Strings, like numbers, are indicated
    by the preceding opcode.  */
 
-enum exp_opcode
+enum exp_opcode : uint8_t
   {
 #define OP(name) name ,
 
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 10/10] Add --enable-ubsan

Tom Tromey-2
In reply to this post by Tom Tromey-2
This adds --enable-ubsan to gdb's configure.  By default it is enabled
in development mode, and disabled otherwise.  This passes both
-fsanitize=undefined and -fno-sanitize-recover=undefined to
compilations, so that undefined behavior violations will be sure to
cause test failures.

gdb/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * README: Mention --enable-ubsan.
        * NEWS: Mention --enable-ubsan.
        * acinclude.m4: Include sanitize.m4.
        * configure: Rebuild.
        * configure.ac: Call AM_GDB_UBSAN.
        * sanitize.m4: New file.

gdb/doc/ChangeLog
2018-10-01  Tom Tromey  <[hidden email]>

        * gdb.texinfo (Configure Options): Document --enable-ubsan.
---
 gdb/ChangeLog       |   9 ++++
 gdb/NEWS            |   8 ++++
 gdb/README          |   7 +++
 gdb/acinclude.m4    |   3 ++
 gdb/configure       | 105 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/configure.ac    |   1 +
 gdb/doc/ChangeLog   |   4 ++
 gdb/doc/gdb.texinfo |   7 +++
 gdb/sanitize.m4     |  46 +++++++++++++++++++
 9 files changed, 190 insertions(+)
 create mode 100644 gdb/sanitize.m4

diff --git a/gdb/NEWS b/gdb/NEWS
index 2669fa91a1..17751bb120 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -103,6 +103,14 @@ CSKY GNU/LINUX csky*-*-linux
   ** The gdb.Progspace type has a new 'objfiles' method, which returns the list
      of objfiles associated to that program space.
 
+* Configure changes
+
+--enable-ubsan
+
+  Enable or disable the undefined behavior sanitizer.  Release
+  versions of gdb disable this by default, but development versions
+  enable it.  Enabling this can cause a performance penalty.
+
 *** Changes in GDB 8.2
 
 * The 'set disassembler-options' command now supports specifying options
diff --git a/gdb/README b/gdb/README
index 5881be23af..69ba0eb8df 100644
--- a/gdb/README
+++ b/gdb/README
@@ -538,6 +538,13 @@ more obscure GDB `configure' options are not listed here.
      the compiler, which will fail the compilation if the compiler
      outputs any warning messages.
 
+`--enable-ubsan'
+     Enable the GCC undefined behavior sanitizer.  By default this is
+     disabled in GDB releases, but enabled when building from git.
+     The undefined behavior sanitizer checks for C++ undefined
+     behavior.  It has a performance cost, so if you are looking at
+     GDB's performance, you should disable it.
+
 `configure' accepts other options, for compatibility with configuring
 other GNU tools recursively.
 
diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 3c2d01015b..52ba3f9ed6 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -15,6 +15,9 @@ sinclude(transform.m4)
 # This gets AM_GDB_WARNINGS.
 sinclude(warning.m4)
 
+# AM_GDB_UBSAN
+sinclude(sanitize.m4)
+
 dnl gdb/configure.in uses BFD_NEED_DECLARATION, so get its definition.
 sinclude(../bfd/bfd.m4)
 
diff --git a/gdb/configure b/gdb/configure
index b7c4ff6f29..4436cc970f 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -886,6 +886,7 @@ with_system_gdbinit
 enable_werror
 enable_build_warnings
 enable_gdb_build_warnings
+enable_ubsan
 with_lzma
 with_liblzma_prefix
 with_tcl
@@ -1556,6 +1557,7 @@ Optional Features:
   --enable-gdb-build-warnings
                           enable GDB specific build-time compiler warnings if
                           gcc is used
+  --enable-ubsan          enable undefined behavior sanitizer (auto/yes/no)
   --enable-sim            link gdb with simulator
   --enable-gdbserver      automatically build gdbserver (yes/no/auto, default
                           is auto)
@@ -2448,6 +2450,52 @@ $as_echo "$ac_res" >&6; }
   eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
 
 } # ac_fn_c_check_decl
+
+# ac_fn_cxx_try_link LINENO
+# -------------------------
+# Try to link conftest.$ac_ext, and return whether this succeeded.
+ac_fn_cxx_try_link ()
+{
+  as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
+  rm -f conftest.$ac_objext conftest$ac_exeext
+  if { { ac_try="$ac_link"
+case "(($ac_try" in
+  *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;;
+  *) ac_try_echo=$ac_try;;
+esac
+eval ac_try_echo="\"\$as_me:${as_lineno-$LINENO}: $ac_try_echo\""
+$as_echo "$ac_try_echo"; } >&5
+  (eval "$ac_link") 2>conftest.err
+  ac_status=$?
+  if test -s conftest.err; then
+    grep -v '^ *+' conftest.err >conftest.er1
+    cat conftest.er1 >&5
+    mv -f conftest.er1 conftest.err
+  fi
+  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
+  test $ac_status = 0; } && {
+ test -z "$ac_cxx_werror_flag" ||
+ test ! -s conftest.err
+       } && test -s conftest$ac_exeext && {
+ test "$cross_compiling" = yes ||
+ test -x conftest$ac_exeext
+       }; then :
+  ac_retval=0
+else
+  $as_echo "$as_me: failed program was:" >&5
+sed 's/^/| /' conftest.$ac_ext >&5
+
+ ac_retval=1
+fi
+  # Delete the IPA/IPO (Inter Procedural Analysis/Optimization) information
+  # created by the PGI compiler (conftest_ipa8_conftest.oo), as it would
+  # interfere with the next link command; also delete a directory that is
+  # left behind by Apple's compiler.  We do this before executing the actions.
+  rm -rf conftest.dSYM conftest_ipa8_conftest.oo
+  eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
+  as_fn_set_status $ac_retval
+
+} # ac_fn_cxx_try_link
 cat >config.log <<_ACEOF
 This file contains any messages produced by compilers while
 running configure, to aid debugging if configure makes a mistake.
@@ -15561,6 +15609,63 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
+# Check whether --enable-ubsan was given.
+if test "${enable_ubsan+set}" = set; then :
+  enableval=$enable_ubsan;
+else
+  enable_ubsan=auto
+fi
+
+if test "x$enable_ubsan" = xauto; then
+  if $development; then
+    enable_ubsan=yes
+  fi
+fi
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+if test "x$enable_ubsan" = xyes; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether -fsanitize=undefined is accepted" >&5
+$as_echo_n "checking whether -fsanitize=undefined is accepted... " >&6; }
+  saved_CXXFLAGS="$CXXFLAGS"
+  CXXFLAGS="$CXXFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+        cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_link "$LINENO"; then :
+  enable_ubsan=yes
+else
+  enable_ubsan=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+  CXXFLAGS="$saved_CXXFLAGS"
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $enable_ubsan" >&5
+$as_echo "$enable_ubsan" >&6; }
+  if test "x$enable_ubsan" = xyes; then
+    WARN_CFLAGS="$WARN_CFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+    CONFIG_LDFLAGS="$CONFIG_LDFLAGS -fsanitize=undefined"
+  fi
+fi
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+
+
 # In the Cygwin environment, we need some additional flags.
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for cygwin" >&5
 $as_echo_n "checking for cygwin... " >&6; }
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 7f6a4033c8..34bfda0907 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1838,6 +1838,7 @@ GDB_AC_WITH_DIR(SYSTEM_GDBINIT, system-gdbinit,
     [])
 
 AM_GDB_WARNINGS
+AM_GDB_UBSAN
 
 # In the Cygwin environment, we need some additional flags.
 AC_CACHE_CHECK([for cygwin], gdb_cv_os_cygwin,
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 49a2cdce9e..913ea21b6e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -35744,6 +35744,13 @@ compiler you are using.
 Treat compiler warnings as werrors.  It adds the @code{-Werror} flag
 to the compiler, which will fail the compilation if the compiler
 outputs any warning messages.
+
+@item --enable-ubsan
+Enable the GCC undefined behavior sanitizer.  By default this is
+disabled in @value{GDBN} releases, but enabled when building from git.
+The undefined behavior sanitizer checks for C@t{++} undefined
+behavior.  It has a performance cost, so if you are looking at
+@value{GDBN}'s performance, you should disable it.
 @end table
 
 @node System-wide configuration
diff --git a/gdb/sanitize.m4 b/gdb/sanitize.m4
new file mode 100644
index 0000000000..3e2207b466
--- /dev/null
+++ b/gdb/sanitize.m4
@@ -0,0 +1,46 @@
+dnl Autoconf configure script for GDB, the GNU debugger.
+dnl Copyright (C) 2018 Free Software Foundation, Inc.
+dnl
+dnl This file is part of GDB.
+dnl
+dnl This program is free software; you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 3 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+dnl GNU General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU General Public License
+dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+AC_DEFUN([AM_GDB_UBSAN],[
+AC_ARG_ENABLE(ubsan,
+  AS_HELP_STRING([--enable-ubsan],
+                 [enable undefined behavior sanitizer (auto/yes/no)]),
+  [],enable_ubsan=auto)
+if test "x$enable_ubsan" = xauto; then
+  if $development; then
+    enable_ubsan=yes
+  fi
+fi
+AC_LANG_PUSH([C++])
+if test "x$enable_ubsan" = xyes; then
+  AC_MSG_CHECKING(whether -fsanitize=undefined is accepted)
+  saved_CXXFLAGS="$CXXFLAGS"
+  CXXFLAGS="$CXXFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+  dnl A link check is required because it is possible to install gcc
+  dnl without libubsan, leading to link failures when compiling with
+  dnl -fsanitize=undefined.
+  AC_TRY_LINK([],[],enable_ubsan=yes,enable_ubsan=no)
+  CXXFLAGS="$saved_CXXFLAGS"
+  AC_MSG_RESULT($enable_ubsan)
+  if test "x$enable_ubsan" = xyes; then
+    WARN_CFLAGS="$WARN_CFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+    CONFIG_LDFLAGS="$CONFIG_LDFLAGS -fsanitize=undefined"
+  fi
+fi
+AC_LANG_POP([C++])
+])
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 10/10] Add --enable-ubsan

Eli Zaretskii
> From: Tom Tromey <[hidden email]>
> Cc: Tom Tromey <[hidden email]>
> Date: Mon,  1 Oct 2018 22:44:20 -0600
>
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -103,6 +103,14 @@ CSKY GNU/LINUX csky*-*-linux
>    ** The gdb.Progspace type has a new 'objfiles' method, which returns the list
>       of objfiles associated to that program space.
>  
> +* Configure changes
> +
> +--enable-ubsan
> +
> +  Enable or disable the undefined behavior sanitizer.  Release
> +  versions of gdb disable this by default, but development versions

"GDB", capitalized, I guess?

> --- a/gdb/README
> +++ b/gdb/README
> @@ -538,6 +538,13 @@ more obscure GDB `configure' options are not listed here.
>       the compiler, which will fail the compilation if the compiler
>       outputs any warning messages.
>  
> +`--enable-ubsan'
> +     Enable the GCC undefined behavior sanitizer.  By default this is
> +     disabled in GDB releases, but enabled when building from git.
> +     The undefined behavior sanitizer checks for C++ undefined
> +     behavior.  It has a performance cost, so if you are looking at
> +     GDB's performance, you should disable it.

Does this require some minimal version of g++?  If so, I think we
should mention that.  And what about testing for this support at
configure time?

> +
> +# ac_fn_cxx_try_link LINENO
> +# -------------------------
> +# Try to link conftest.$ac_ext, and return whether this succeeded.
> +ac_fn_cxx_try_link ()
> +{
> +  as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
> +  rm -f conftest.$ac_objext conftest$ac_exeext
> +  if { { ac_try="$ac_link"
> +case "(($ac_try" in
> +  *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;;
> +  *) ac_try_echo=$ac_try;;
> +esac
> +eval ac_try_echo="\"\$as_me:${as_lineno-$LINENO}: $ac_try_echo\""
> +$as_echo "$ac_try_echo"; } >&5
> +  (eval "$ac_link") 2>conftest.err
> +  ac_status=$?
> +  if test -s conftest.err; then
> +    grep -v '^ *+' conftest.err >conftest.er1
> +    cat conftest.er1 >&5
> +    mv -f conftest.er1 conftest.err
> +  fi
> +  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> +  test $ac_status = 0; } && {
> + test -z "$ac_cxx_werror_flag" ||
> + test ! -s conftest.err
> +       } && test -s conftest$ac_exeext && {
> + test "$cross_compiling" = yes ||
> + test -x conftest$ac_exeext
> +       }; then :
> +  ac_retval=0
> +else
> +  $as_echo "$as_me: failed program was:" >&5
> +sed 's/^/| /' conftest.$ac_ext >&5
> +
> + ac_retval=1
> +fi
> +  # Delete the IPA/IPO (Inter Procedural Analysis/Optimization) information
> +  # created by the PGI compiler (conftest_ipa8_conftest.oo), as it would
> +  # interfere with the next link command; also delete a directory that is
> +  # left behind by Apple's compiler.  We do this before executing the actions.
> +  rm -rf conftest.dSYM conftest_ipa8_conftest.oo
> +  eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
> +  as_fn_set_status $ac_retval
> +
> +} # ac_fn_cxx_try_link
>  cat >config.log <<_ACEOF
>  This file contains any messages produced by compilers while
>  running configure, to aid debugging if configure makes a mistake.

Is this hunk related to the issue at hand?

The documentation parts are approved, with the above nits fixed.

Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 10/10] Add --enable-ubsan

Tom Tromey-2
>>>>> "Eli" == Eli Zaretskii <[hidden email]> writes:

>> +  Enable or disable the undefined behavior sanitizer.  Release
>> +  versions of gdb disable this by default, but development versions

Eli> "GDB", capitalized, I guess?

Done.

>> +`--enable-ubsan'
>> +     Enable the GCC undefined behavior sanitizer.  By default this is
>> +     disabled in GDB releases, but enabled when building from git.
>> +     The undefined behavior sanitizer checks for C++ undefined
>> +     behavior.  It has a performance cost, so if you are looking at
>> +     GDB's performance, you should disable it.

Eli> Does this require some minimal version of g++?  If so, I think we
Eli> should mention that.  And what about testing for this support at
Eli> configure time?

It was in GCC 4.9.  It is tested at configure time.  I've updated the
text & will send a new patch.

>> +# ac_fn_cxx_try_link LINENO
>> +# -------------------------
[..]

Eli> Is this hunk related to the issue at hand?

Yes, I think it's part of configure expansion of the new test.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 10/10] Add --enable-ubsan

Tom Tromey-2
>>>>> "Tom" == Tom Tromey <[hidden email]> writes:

Tom> It was in GCC 4.9.  It is tested at configure time.  I've updated the
Tom> text & will send a new patch.

Let me know what you think.

Tom

commit fcb215b14cfe9e45178659b94fa9978c5b457bb8
Author: Tom Tromey <[hidden email]>
Date:   Sat Aug 18 15:32:46 2018 -0600

    Add --enable-ubsan
   
    This adds --enable-ubsan to gdb's configure.  By default it is enabled
    in development mode, and disabled otherwise.  This passes both
    -fsanitize=undefined and -fno-sanitize-recover=undefined to
    compilations, so that undefined behavior violations will be sure to
    cause test failures.
   
    gdb/ChangeLog
    2018-10-01  Tom Tromey  <[hidden email]>
   
            * README: Mention --enable-ubsan.
            * NEWS: Mention --enable-ubsan.
            * acinclude.m4: Include sanitize.m4.
            * configure: Rebuild.
            * configure.ac: Call AM_GDB_UBSAN.
            * sanitize.m4: New file.
   
    gdb/doc/ChangeLog
    2018-10-01  Tom Tromey  <[hidden email]>
   
            * gdb.texinfo (Configure Options): Document --enable-ubsan.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2776263e86..451f7c4f67 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2018-10-01  Tom Tromey  <[hidden email]>
+
+ * README: Mention --enable-ubsan.
+ * NEWS: Mention --enable-ubsan.
+ * acinclude.m4: Include sanitize.m4.
+ * configure: Rebuild.
+ * configure.ac: Call AM_GDB_UBSAN.
+ * sanitize.m4: New file.
+
 2018-10-01  Tom Tromey  <[hidden email]>
 
  * expression.h (enum exp_opcode): Use uint8_t as base type.
diff --git a/gdb/NEWS b/gdb/NEWS
index 00adcd4d53..b409aa447c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -108,6 +108,16 @@ CSKY GNU/LINUX csky*-*-linux
   ** The gdb.Progspace type has a new 'objfiles' method, which returns the list
      of objfiles associated to that program space.
 
+* Configure changes
+
+--enable-ubsan
+
+  Enable or disable the undefined behavior sanitizer.  Release
+  versions of GDB disable this by default if it is available, but
+  development versions enable it.  Enabling this can cause a
+  performance penalty.  The undefined behavior sanitizer was first
+  introduced in GCC 4.9.
+
 *** Changes in GDB 8.2
 
 * The 'set disassembler-options' command now supports specifying options
diff --git a/gdb/README b/gdb/README
index 5881be23af..69ba0eb8df 100644
--- a/gdb/README
+++ b/gdb/README
@@ -538,6 +538,13 @@ more obscure GDB `configure' options are not listed here.
      the compiler, which will fail the compilation if the compiler
      outputs any warning messages.
 
+`--enable-ubsan'
+     Enable the GCC undefined behavior sanitizer.  By default this is
+     disabled in GDB releases, but enabled when building from git.
+     The undefined behavior sanitizer checks for C++ undefined
+     behavior.  It has a performance cost, so if you are looking at
+     GDB's performance, you should disable it.
+
 `configure' accepts other options, for compatibility with configuring
 other GNU tools recursively.
 
diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 3c2d01015b..52ba3f9ed6 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -15,6 +15,9 @@ sinclude(transform.m4)
 # This gets AM_GDB_WARNINGS.
 sinclude(warning.m4)
 
+# AM_GDB_UBSAN
+sinclude(sanitize.m4)
+
 dnl gdb/configure.in uses BFD_NEED_DECLARATION, so get its definition.
 sinclude(../bfd/bfd.m4)
 
diff --git a/gdb/configure b/gdb/configure
index 931e19d2a4..0ba80147c6 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -886,6 +886,7 @@ with_system_gdbinit
 enable_werror
 enable_build_warnings
 enable_gdb_build_warnings
+enable_ubsan
 with_lzma
 with_liblzma_prefix
 with_tcl
@@ -1556,6 +1557,7 @@ Optional Features:
   --enable-gdb-build-warnings
                           enable GDB specific build-time compiler warnings if
                           gcc is used
+  --enable-ubsan          enable undefined behavior sanitizer (auto/yes/no)
   --enable-sim            link gdb with simulator
   --enable-gdbserver      automatically build gdbserver (yes/no/auto, default
                           is auto)
@@ -2448,6 +2450,52 @@ $as_echo "$ac_res" >&6; }
   eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
 
 } # ac_fn_c_check_decl
+
+# ac_fn_cxx_try_link LINENO
+# -------------------------
+# Try to link conftest.$ac_ext, and return whether this succeeded.
+ac_fn_cxx_try_link ()
+{
+  as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
+  rm -f conftest.$ac_objext conftest$ac_exeext
+  if { { ac_try="$ac_link"
+case "(($ac_try" in
+  *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;;
+  *) ac_try_echo=$ac_try;;
+esac
+eval ac_try_echo="\"\$as_me:${as_lineno-$LINENO}: $ac_try_echo\""
+$as_echo "$ac_try_echo"; } >&5
+  (eval "$ac_link") 2>conftest.err
+  ac_status=$?
+  if test -s conftest.err; then
+    grep -v '^ *+' conftest.err >conftest.er1
+    cat conftest.er1 >&5
+    mv -f conftest.er1 conftest.err
+  fi
+  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
+  test $ac_status = 0; } && {
+ test -z "$ac_cxx_werror_flag" ||
+ test ! -s conftest.err
+       } && test -s conftest$ac_exeext && {
+ test "$cross_compiling" = yes ||
+ test -x conftest$ac_exeext
+       }; then :
+  ac_retval=0
+else
+  $as_echo "$as_me: failed program was:" >&5
+sed 's/^/| /' conftest.$ac_ext >&5
+
+ ac_retval=1
+fi
+  # Delete the IPA/IPO (Inter Procedural Analysis/Optimization) information
+  # created by the PGI compiler (conftest_ipa8_conftest.oo), as it would
+  # interfere with the next link command; also delete a directory that is
+  # left behind by Apple's compiler.  We do this before executing the actions.
+  rm -rf conftest.dSYM conftest_ipa8_conftest.oo
+  eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
+  as_fn_set_status $ac_retval
+
+} # ac_fn_cxx_try_link
 cat >config.log <<_ACEOF
 This file contains any messages produced by compilers while
 running configure, to aid debugging if configure makes a mistake.
@@ -15561,6 +15609,63 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
+# Check whether --enable-ubsan was given.
+if test "${enable_ubsan+set}" = set; then :
+  enableval=$enable_ubsan;
+else
+  enable_ubsan=auto
+fi
+
+if test "x$enable_ubsan" = xauto; then
+  if $development; then
+    enable_ubsan=yes
+  fi
+fi
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+if test "x$enable_ubsan" = xyes; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether -fsanitize=undefined is accepted" >&5
+$as_echo_n "checking whether -fsanitize=undefined is accepted... " >&6; }
+  saved_CXXFLAGS="$CXXFLAGS"
+  CXXFLAGS="$CXXFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+        cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_link "$LINENO"; then :
+  enable_ubsan=yes
+else
+  enable_ubsan=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+  CXXFLAGS="$saved_CXXFLAGS"
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $enable_ubsan" >&5
+$as_echo "$enable_ubsan" >&6; }
+  if test "x$enable_ubsan" = xyes; then
+    WARN_CFLAGS="$WARN_CFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+    CONFIG_LDFLAGS="$CONFIG_LDFLAGS -fsanitize=undefined"
+  fi
+fi
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+
+
 # In the Cygwin environment, we need some additional flags.
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for cygwin" >&5
 $as_echo_n "checking for cygwin... " >&6; }
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 88f2fc47e1..dfb7c87171 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1838,6 +1838,7 @@ GDB_AC_WITH_DIR(SYSTEM_GDBINIT, system-gdbinit,
     [])
 
 AM_GDB_WARNINGS
+AM_GDB_UBSAN
 
 # In the Cygwin environment, we need some additional flags.
 AC_CACHE_CHECK([for cygwin], gdb_cv_os_cygwin,
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index c26b8e6e91..76b654a043 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2018-10-01  Tom Tromey  <[hidden email]>
+
+ * gdb.texinfo (Configure Options): Document --enable-ubsan.
+
 2018-10-02  John Darrington <[hidden email]>
 
         * gdb.texinfo (Remote Connection Commands): Describe
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d37c9e43ca..5653bdcaca 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -35763,6 +35763,14 @@ compiler you are using.
 Treat compiler warnings as werrors.  It adds the @code{-Werror} flag
 to the compiler, which will fail the compilation if the compiler
 outputs any warning messages.
+
+@item --enable-ubsan
+Enable the GCC undefined behavior sanitizer.  By default this is
+disabled in @value{GDBN} releases, but enabled, when available, when
+building from git.  The undefined behavior sanitizer checks for
+C@t{++} undefined behavior.  It has a performance cost, so if you are
+looking at @value{GDBN}'s performance, you should disable it.  The
+undefined behavior sanitizer was first introduced in GCC 4.9.
 @end table
 
 @node System-wide configuration
diff --git a/gdb/sanitize.m4 b/gdb/sanitize.m4
new file mode 100644
index 0000000000..3e2207b466
--- /dev/null
+++ b/gdb/sanitize.m4
@@ -0,0 +1,46 @@
+dnl Autoconf configure script for GDB, the GNU debugger.
+dnl Copyright (C) 2018 Free Software Foundation, Inc.
+dnl
+dnl This file is part of GDB.
+dnl
+dnl This program is free software; you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 3 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+dnl GNU General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU General Public License
+dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+AC_DEFUN([AM_GDB_UBSAN],[
+AC_ARG_ENABLE(ubsan,
+  AS_HELP_STRING([--enable-ubsan],
+                 [enable undefined behavior sanitizer (auto/yes/no)]),
+  [],enable_ubsan=auto)
+if test "x$enable_ubsan" = xauto; then
+  if $development; then
+    enable_ubsan=yes
+  fi
+fi
+AC_LANG_PUSH([C++])
+if test "x$enable_ubsan" = xyes; then
+  AC_MSG_CHECKING(whether -fsanitize=undefined is accepted)
+  saved_CXXFLAGS="$CXXFLAGS"
+  CXXFLAGS="$CXXFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+  dnl A link check is required because it is possible to install gcc
+  dnl without libubsan, leading to link failures when compiling with
+  dnl -fsanitize=undefined.
+  AC_TRY_LINK([],[],enable_ubsan=yes,enable_ubsan=no)
+  CXXFLAGS="$saved_CXXFLAGS"
+  AC_MSG_RESULT($enable_ubsan)
+  if test "x$enable_ubsan" = xyes; then
+    WARN_CFLAGS="$WARN_CFLAGS -fsanitize=undefined -fno-sanitize-recover=undefined"
+    CONFIG_LDFLAGS="$CONFIG_LDFLAGS -fsanitize=undefined"
+  fi
+fi
+AC_LANG_POP([C++])
+])
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 10/02/2018 05:44 AM, Tom Tromey wrote:
> This changes dwarf2_frame_state_reg_info::reg to be a std::vector.
> This avoids undefined behavior in the copy constructor when the
> original object does not have any registers.

I don't have anything to say against the patch, I think it's a nice
cleanup regardless, but could you expand this log a little to point
out exactly what is triggering the undefined behavior.  I guess we're
passing NULL to the memcpy?  LGTM otherwise.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 10/10] Add --enable-ubsan

Eli Zaretskii
In reply to this post by Tom Tromey-2
> From: Tom Tromey <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  [hidden email]
> Date: Tue, 02 Oct 2018 15:27:52 -0600
>
> >>>>> "Tom" == Tom Tromey <[hidden email]> writes:
>
> Tom> It was in GCC 4.9.  It is tested at configure time.  I've updated the
> Tom> text & will send a new patch.
>
> Let me know what you think.

LGTM, thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 03/10] Use unsigned as base type for some enums

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 10/02/2018 05:44 AM, Tom Tromey wrote:
> -fsanitize=undefined complains about using operator~ on various enum
> types that are used with DEF_ENUM_FLAGS_TYPE.  This patch fixes these
> problems by explicitly setting the base type for these enums to
> unsigned.  It also adds a static assert to enum_flags to ensure that
> future enums used this way have an unsigned underlying type.
>
> gdb/ChangeLog

This LGTM, except a nit.  During the discussion around v1, the conclusion
was that we can't add the assertion to the class, but adding it to
operator~ would work.  That information is lost on whoever ends up reading
this code later on.  Could you add a comment?  Or if you prefer, update the
commit log to mention it?

>    enum_flags operator~ () const
>    {
> +    gdb_static_assert (std::is_unsigned<underlying_type>::value);
>      return (enum_type) ~underlying_value ();
>    }

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 09/10] Avoid undefined behavior in expression dumping

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 10/02/2018 05:44 AM, Tom Tromey wrote:

> -fsanitize=undefined pointed out undefined behavior in
> dump_raw_expression like:
>
>     runtime error: load of value 2887952, which is not a valid value for type 'exp_opcode'
>
> dump_raw_expression will try to print the opcode for each element of
> the expression, even when it is not valid.  To allow this, but have it
> avoid undefined behavior, this patch sets the underlying type of enum
> exp_opcode, and arranges for op_name to handle invalid opcodes more
> nicely.
>
> Before this patch, debug-expr.exp shows:
>
> Dump of expression @ 0x60f000007750, before conversion to prefix form:
> Language c, 8 elements, 16 bytes each.
> Index                Opcode         Hex Value  String Value
>    0               OP_TYPE  89  Y...............
>    <unknown 3851920>  107820862850704  ..:..b..........
>    2               OP_TYPE  89  Y...............
>    3          OP_VAR_VALUE  40  (...............
>    4     <unknown 2807568>  107820861806352  ..*..b..........
>    5     <unknown 2806368>  107820861805152  `.*..b..........
>    6          OP_VAR_VALUE  40  (...............
>    7      UNOP_MEMVAL_TYPE  57  9...............
>
> Afterward, the output is:
>
> Dump of expression @ 0x4820f90, before conversion to prefix form:
> Language c, 8 elements, 16 bytes each.
> Index                Opcode         Hex Value  String Value
>    0               OP_TYPE  89  Y...............
>    1   unknown opcode: 176  75444400  .0..............
>    2               OP_TYPE  89  Y...............
>    3          OP_VAR_VALUE  40  (...............
>    4               OP_BOOL  74616912  P.r.............
>    5   unknown opcode: 128  74615680  ..r.............
>    6          OP_VAR_VALUE  40  (...............
>    7      UNOP_MEMVAL_TYPE  57  9...............
>

LGTM.  Thanks for adding the example.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 10/10] Add --enable-ubsan

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 10/02/2018 05:44 AM, Tom Tromey wrote:
> This adds --enable-ubsan to gdb's configure.  By default it is enabled
> in development mode, and disabled otherwise.  This passes both
> -fsanitize=undefined and -fno-sanitize-recover=undefined to
> compilations, so that undefined behavior violations will be sure to
> cause test failures.

Thanks much for all the docs additions, and all the preparatory
work that came with it.

> --- /dev/null
> +++ b/gdb/sanitize.m4
> @@ -0,0 +1,46 @@
> +dnl Autoconf configure script for GDB, the GNU debugger.

Nit: I guess this was copied from warning.m4, but, could you
tweak this intro comment, which seems to have been originally
blindly copied from configure.ac into warning.m4?

Otherwise LGTM.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 10/02/2018 05:44 AM, Tom Tromey wrote:
> This is a new version of the series to add -fsanitize=undefined to the
> build.
>
> It's only added to gdb, though it occurred to me later that it would
> probably be better to add it to all the libraries as well.

Yeah.  I'm fine with starting with gdb only first, though.

Thanks,
Pedro Alves
12