* [PATCH v2 03/10] Use unsigned as base type for some enums
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 09/10] Avoid undefined behavior in expression dumping Tom Tromey
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 bfb0b9f2787..0448bd6d498 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 ae17abd20f9..f9eab04730c 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 82568a5a3d9..dad59cec867 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 14059ab3dc5..f7ea2ac6df5 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -192,7 +192,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 aeaa8fbc04d..6f5760d021d 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 3c07513e395..35241fe4a04 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 09/10] Avoid undefined behavior in expression dumping
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
2018-08-30 2:44 ` [PATCH v2 03/10] Use unsigned as base type for some enums Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 10/10] Add --enable-ubsan Tom Tromey
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 d6ed41253ed..e87b3b709b9 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 9f26bb8d60b..db572efe2a3 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 10/10] Add --enable-ubsan
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
2018-08-30 2:44 ` [PATCH v2 03/10] Use unsigned as base type for some enums Tom Tromey
2018-08-30 2:44 ` [PATCH v2 09/10] Avoid undefined behavior in expression dumping Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 05/10] Avoid undefined behavior in read_subrange_type Tom Tromey
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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-08-29 Tom Tromey <tom@tromey.com>
* acinclude.m4: Include sanitize.m4.
* configure: Rebuild.
* configure.ac: Call AM_GDB_UBSAN.
* sanitize.m4: New file.
---
gdb/ChangeLog | 7 ++++
gdb/acinclude.m4 | 3 ++
gdb/configure | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
gdb/configure.ac | 1 +
gdb/sanitize.m4 | 46 ++++++++++++++++++++++++
5 files changed, 162 insertions(+)
create mode 100644 gdb/sanitize.m4
diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 3c2d01015bf..52ba3f9ed6e 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 d207c2baf1b..51fef3266e4 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
@@ -1558,6 +1559,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-multi-ice build the multi-ice-gdb-server
--enable-gdbserver automatically build gdbserver (yes/no/auto, default
@@ -2451,6 +2453,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.
@@ -15555,6 +15603,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 13bc5f9a8f2..a87a5ff43ac 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1860,6 +1860,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/sanitize.m4 b/gdb/sanitize.m4
new file mode 100644
index 00000000000..3e2207b466c
--- /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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 05/10] Avoid undefined behavior in read_subrange_type
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (2 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 10/10] Add --enable-ubsan Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128 Tom Tromey
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 8834d08a1c6..86ef1c4040b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17682,7 +17682,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,
@@ -17815,7 +17815,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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (3 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 05/10] Avoid undefined behavior in read_subrange_type Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector Tom Tromey
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 86ef1c4040b..e61d6e04cb4 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -19600,7 +19600,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;
@@ -19612,7 +19612,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)
{
@@ -19620,7 +19620,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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (4 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128 Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 04/10] Avoid undefined behavior in extract_integer Tom Tromey
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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-08-29 Tom Tromey <tom@tromey.com>
* 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 | 15 +++++++--------
gdb/dwarf2-frame.h | 31 +++++++++----------------------
3 files changed, 28 insertions(+), 30 deletions(-)
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f7dc820f4db..a594d223aa9 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);
@@ -1150,7 +1149,7 @@ 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
+ if (fs.retaddr_column < fs.regs.reg.size ()
&& retaddr_reg->how != DWARF2_FRAME_REG_UNSPECIFIED
&& retaddr_reg->how != DWARF2_FRAME_REG_SAME_VALUE)
{
@@ -1176,7 +1175,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 52316e5e168..b89f931651a 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 04/10] Avoid undefined behavior in extract_integer
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (5 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 06/10] Avoid undefined behavior in parse_number Tom Tromey
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 9256833ab60..be6c9d6f60b 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 06/10] Avoid undefined behavior in parse_number
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (6 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 04/10] Avoid undefined behavior in extract_integer Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 08/10] Avoid undefined behavior in ada_operator_length Tom Tromey
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 0326ee090e1..09e31d2283a 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 08/10] Avoid undefined behavior in ada_operator_length
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (7 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 06/10] Avoid undefined behavior in parse_number Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-08-30 2:44 ` [PATCH v2 01/10] Do not pass NULL to memcpy Tom Tromey
2018-09-12 11:49 ` [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-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-08-29 Tom Tromey <tom@tromey.com>
* 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 163852cb84c..8a128a74d77 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 01/10] Do not pass NULL to memcpy
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (8 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 08/10] Avoid undefined behavior in ada_operator_length Tom Tromey
@ 2018-08-30 2:44 ` Tom Tromey
2018-09-12 11:49 ` [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-08-30 2:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
-fsanitize=undefined pointed out a spot that passes NULL to memcpy,
which is undefined behavior according to the C standard.
gdb/ChangeLog
2018-08-29 Tom Tromey <tom@tromey.com>
* 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 be998d9d491..85c0c4b14d7 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.13.6
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 00/10] Enable undefined behavior sanitizer
2018-08-30 2:44 [PATCH v2 00/10] Enable undefined behavior sanitizer Tom Tromey
` (9 preceding siblings ...)
2018-08-30 2:44 ` [PATCH v2 01/10] Do not pass NULL to memcpy Tom Tromey
@ 2018-09-12 11:49 ` Tom Tromey
10 siblings, 0 replies; 12+ messages in thread
From: Tom Tromey @ 2018-09-12 11:49 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> This is v2 of the series to enable the undefined behavior sanitizer
Tom> for gdb. This is done by default in development mode, but can be
Tom> disabled.
Update on this series...
Tom> I believe this version addresses all the review comments.
For the record, subsequently, Pedro and I discussed the need to document
this a bit more. I'll include a patch for this in the next revision of
the series.
Tom> Tested (*) by the buildbot.
Tom> (*) However, as I mentioned in another thread, the s390 builders do
Tom> not like this series. I get many, many failures. Looking at the
Tom> gdb.log, though, the failures seem to be coming from libstdc++ debug
Tom> mode -- which this series does not enable.
This is all explained now.
Sergio pointed out that the builders are enabling -D_GLIBCXX_DEBUG. I'm
glad that mystery is solved.
A failure with -D_GLIBCXX_DEBUG enabled just results in a crash, without
much useful information to go by. This is often fine, but in this case
I could only reproduce on s390, where I couldn't log in to reproduce for
myself.
So, I hacked the testsuite Makefile to run just a single test .exp that
I knew to fail, and then arranged to get a real stack trace. See the
appended patch.
This pointed out that there was a bug in one of the patches in the series.
Namely, this code in dwarf2-frame.c:
struct dwarf2_frame_state_reg *retaddr_reg =
&fs.regs.reg[fs.retaddr_column];
... will reference past the end of the array in some cases. When reg is
just an ordinary pointer this is "ok" -- possibly undefined behavior
depending on the index in use, but not likely to cause a crash. But the
series changes reg to a std::vector, where this is invalid.
Tom
diff --git a/gdb/testsuite/Makefile.in b/gdb/testsuite/Makefile.in
index 07e0928a9f8..3bfc4d9f847 100644
--- a/gdb/testsuite/Makefile.in
+++ b/gdb/testsuite/Makefile.in
@@ -48,7 +48,7 @@ EXPECT = `if [ "$${READ1}" != "" ] ; then \
RUNTEST = $(RUNTEST_FOR_TARGET)
-RUNTESTFLAGS =
+override RUNTESTFLAGS := gdb.cp/extern-c.exp
FORCE_PARALLEL =
@@ -153,7 +153,11 @@ CHECK_TARGET = $(if $(RACY_ITER),$(addsuffix -racy,$(CHECK_TARGET_TMP)),$(CHECK_
# because GNU make 3.82 has a bug preventing MAKEFLAGS from being used
# in conditions.
check: all $(abs_builddir)/site.exp
- $(MAKE) $(CHECK_TARGET)
+ -$(MAKE) $(CHECK_TARGET)
+ cat gdb.log
+ /bin/gdb -batch -ex 'run' -ex 'bt' \
+ --args ../gdb -batch -ex 'file outputs/gdb.cp/extern-c/extern-c' \
+ -ex 'break c_func' -ex 'run' -ex 'rbreak c_funcs'
check-read1:
$(MAKE) READ1="1" check
^ permalink raw reply [flat|nested] 12+ messages in thread