* [PATCH v2 08/10] Avoid undefined behavior in ada_operator_length
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-02 4:44 ` [PATCH v2 04/10] Avoid undefined behavior in extract_integer Tom Tromey
` (10 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v2 04/10] Avoid undefined behavior in extract_integer
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
2018-10-02 4:44 ` [PATCH v2 08/10] Avoid undefined behavior in ada_operator_length Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-02 4:44 ` [PATCH v2 09/10] Avoid undefined behavior in expression dumping Tom Tromey
` (9 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v2 09/10] Avoid undefined behavior in expression dumping
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
2018-10-02 4:44 ` [PATCH v2 08/10] Avoid undefined behavior in ada_operator_length Tom Tromey
2018-10-02 4:44 ` [PATCH v2 04/10] Avoid undefined behavior in extract_integer Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-03 17:48 ` Pedro Alves
2018-10-02 4:44 ` [PATCH v2 10/10] Add --enable-ubsan Tom Tromey
` (8 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 09/10] Avoid undefined behavior in expression dumping
2018-10-02 4:44 ` [PATCH v2 09/10] Avoid undefined behavior in expression dumping Tom Tromey
@ 2018-10-03 17:48 ` Pedro Alves
0 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2018-10-03 17:48 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 10/10] Add --enable-ubsan
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (2 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 09/10] Avoid undefined behavior in expression dumping Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-02 13:53 ` Eli Zaretskii
2018-10-03 17:54 ` Pedro Alves
2018-10-02 4:44 ` [PATCH v2 06/10] Avoid undefined behavior in parse_number Tom Tromey
` (7 subsequent siblings)
11 siblings, 2 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 Tom Tromey <tom@tromey.com>
* 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 <tom@tromey.com>
* 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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 10/10] Add --enable-ubsan
2018-10-02 4:44 ` [PATCH v2 10/10] Add --enable-ubsan Tom Tromey
@ 2018-10-02 13:53 ` Eli Zaretskii
2018-10-02 21:26 ` Tom Tromey
2018-10-03 17:54 ` Pedro Alves
1 sibling, 1 reply; 29+ messages in thread
From: Eli Zaretskii @ 2018-10-02 13:53 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 10/10] Add --enable-ubsan
2018-10-02 13:53 ` Eli Zaretskii
@ 2018-10-02 21:26 ` Tom Tromey
2018-10-02 21:28 ` Tom Tromey
0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 21:26 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Tom Tromey, gdb-patches
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 10/10] Add --enable-ubsan
2018-10-02 21:26 ` Tom Tromey
@ 2018-10-02 21:28 ` Tom Tromey
2018-10-03 17:31 ` Eli Zaretskii
0 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 21:28 UTC (permalink / raw)
To: Tom Tromey; +Cc: Eli Zaretskii, gdb-patches
>>>>> "Tom" == Tom Tromey <tom@tromey.com> 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 <tom@tromey.com>
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 <tom@tromey.com>
* 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 <tom@tromey.com>
* 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 <tom@tromey.com>
+
+ * 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 <tom@tromey.com>
* 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 <tom@tromey.com>
+
+ * gdb.texinfo (Configure Options): Document --enable-ubsan.
+
2018-10-02 John Darrington <john@darrington.wattle.id.au>
* 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++])
+])
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 10/10] Add --enable-ubsan
2018-10-02 21:28 ` Tom Tromey
@ 2018-10-03 17:31 ` Eli Zaretskii
0 siblings, 0 replies; 29+ messages in thread
From: Eli Zaretskii @ 2018-10-03 17:31 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
> From: Tom Tromey <tom@tromey.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> Date: Tue, 02 Oct 2018 15:27:52 -0600
>
> >>>>> "Tom" == Tom Tromey <tom@tromey.com> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 10/10] Add --enable-ubsan
2018-10-02 4:44 ` [PATCH v2 10/10] Add --enable-ubsan Tom Tromey
2018-10-02 13:53 ` Eli Zaretskii
@ 2018-10-03 17:54 ` Pedro Alves
2018-10-03 21:09 ` Tom Tromey
1 sibling, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2018-10-03 17:54 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 10/10] Add --enable-ubsan
2018-10-03 17:54 ` Pedro Alves
@ 2018-10-03 21:09 ` Tom Tromey
0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-03 21:09 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> --- /dev/null
>> +++ b/gdb/sanitize.m4
>> @@ -0,0 +1,46 @@
>> +dnl Autoconf configure script for GDB, the GNU debugger.
Pedro> Nit: I guess this was copied from warning.m4, but, could you
Pedro> tweak this intro comment, which seems to have been originally
Pedro> blindly copied from configure.ac into warning.m4?
I've tweaked it.
Tom
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 06/10] Avoid undefined behavior in parse_number
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (3 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 10/10] Add --enable-ubsan Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-02 4:44 ` [PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128 Tom Tromey
` (6 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (4 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 06/10] Avoid undefined behavior in parse_number Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-02 4:44 ` [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector Tom Tromey
` (5 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (5 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 07/10] Avoid undefined behavior in read_signed_leb128 Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-03 17:28 ` Pedro Alves
2018-10-02 4:44 ` [PATCH v2 01/10] Do not pass NULL to memcpy Tom Tromey
` (4 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 | 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> ®s
+ = 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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector
2018-10-02 4:44 ` [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector Tom Tromey
@ 2018-10-03 17:28 ` Pedro Alves
2018-10-03 21:05 ` Tom Tromey
0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2018-10-03 17:28 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector
2018-10-03 17:28 ` Pedro Alves
@ 2018-10-03 21:05 ` Tom Tromey
0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-03 21:05 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> 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.
Pedro> I don't have anything to say against the patch, I think it's a nice
Pedro> cleanup regardless, but could you expand this log a little to point
Pedro> out exactly what is triggering the undefined behavior. I guess we're
Pedro> passing NULL to the memcpy?
Yes, that was the issue. Simon asked for this cleanup IIRC.
I've added a note about this to the commit message.
Tom
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 01/10] Do not pass NULL to memcpy
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (6 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-02 4:44 ` [PATCH v2 03/10] Use unsigned as base type for some enums Tom Tromey
` (3 subsequent siblings)
11 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v2 03/10] Use unsigned as base type for some enums
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (7 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 01/10] Do not pass NULL to memcpy Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-03 17:33 ` Pedro Alves
2018-10-02 4:44 ` [PATCH v2 05/10] Avoid undefined behavior in read_subrange_type Tom Tromey
` (2 subsequent siblings)
11 siblings, 1 reply; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 03/10] Use unsigned as base type for some enums
2018-10-02 4:44 ` [PATCH v2 03/10] Use unsigned as base type for some enums Tom Tromey
@ 2018-10-03 17:33 ` Pedro Alves
2018-10-03 21:07 ` Tom Tromey
0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2018-10-03 17:33 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 03/10] Use unsigned as base type for some enums
2018-10-03 17:33 ` Pedro Alves
@ 2018-10-03 21:07 ` Tom Tromey
0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-03 21:07 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> This LGTM, except a nit. During the discussion around v1, the conclusion
Pedro> was that we can't add the assertion to the class, but adding it to
Pedro> operator~ would work. That information is lost on whoever ends up reading
Pedro> this code later on. Could you add a comment? Or if you prefer, update the
Pedro> commit log to mention it?
I've added a comment.
Tom
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 05/10] Avoid undefined behavior in read_subrange_type
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (8 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 03/10] Use unsigned as base type for some enums Tom Tromey
@ 2018-10-02 4:44 ` Tom Tromey
2018-10-03 17:57 ` [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Pedro Alves
2018-10-08 19:14 ` John Baldwin
11 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-02 4: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-10-01 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 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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (9 preceding siblings ...)
2018-10-02 4:44 ` [PATCH v2 05/10] Avoid undefined behavior in read_subrange_type Tom Tromey
@ 2018-10-03 17:57 ` Pedro Alves
2018-10-03 21:09 ` Tom Tromey
2018-10-08 19:14 ` John Baldwin
11 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2018-10-03 17:57 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs
2018-10-03 17:57 ` [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Pedro Alves
@ 2018-10-03 21:09 ` Tom Tromey
0 siblings, 0 replies; 29+ messages in thread
From: Tom Tromey @ 2018-10-03 21:09 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> 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.
Pedro> Yeah. I'm fine with starting with gdb only first, though.
Thanks everyone for the reviews.
I'm going to check this in now.
Tom
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs
2018-10-02 4:44 [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Tom Tromey
` (10 preceding siblings ...)
2018-10-03 17:57 ` [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs Pedro Alves
@ 2018-10-08 19:14 ` John Baldwin
2018-10-08 20:22 ` Joel Brobecker
11 siblings, 1 reply; 29+ messages in thread
From: John Baldwin @ 2018-10-08 19:14 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
On 10/1/18 9:44 PM, 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.
>
> 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.
FWIW, I built GDB master today and ubsan (from LLVM, not GCC) flagged a
use of obstack_blank_fast() in minsyms.c with a negative offset (used to
shrink an obstack) when trying to do 'start' on /bin/ls:
(gdb) start
Temporary breakpoint 1 at 0x402674: file /usr/src/bin/ls/ls.c, line 161.
Starting program: /bin/ls
../../gdb/minsyms.c:1378:7: runtime error: addition of unsigned offset to 0x00080907cd10 overflowed to 0x00080907cc38
This corresponds to the invocation of obstack_blank_fast here:
/* Compact out any duplicates, and free up whatever space we are
no longer using. */
mcount = compact_minimal_symbols (msymbols, mcount, m_objfile);
obstack_blank_fast (&m_objfile->per_bfd->storage_obstack,
(mcount + 1 - alloc_count) * sizeof (struct minimal_symbol));
The case that triggered the failure for me had these values that resulted
in a negative offset:
(top-gdb) p mcount
$23 = 5740
(top-gdb) p alloc_count
$24 = 5744
...
(top-gdb) p mcount + 1 - alloc_count
$26 = -3
I guess since sizeof's return type is size_t, then the promotion rules on
LP64 mean that the resulting value is unsigned? Anyway, we might consider
making ubsan only default to on for GCC for now?
--
John Baldwin
                                                                           Â
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs
2018-10-08 19:14 ` John Baldwin
@ 2018-10-08 20:22 ` Joel Brobecker
2018-10-09 10:44 ` Pedro Alves
0 siblings, 1 reply; 29+ messages in thread
From: Joel Brobecker @ 2018-10-08 20:22 UTC (permalink / raw)
To: John Baldwin; +Cc: Tom Tromey, gdb-patches
> FWIW, I built GDB master today and ubsan (from LLVM, not GCC) flagged a
> use of obstack_blank_fast() in minsyms.c with a negative offset (used to
> shrink an obstack) when trying to do 'start' on /bin/ls:
On my end, I am working on a resync of AdaCore's "head" version,
which would include this change as well. My testing shows that
it revealed some issues; I haven't had a chance to look into them yet
(next on my list today, hopefully), but this is making me think
there might be bona fide issues. Getting the location where things
are happening is pretty nice :).
--
Joel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs
2018-10-08 20:22 ` Joel Brobecker
@ 2018-10-09 10:44 ` Pedro Alves
2018-10-12 21:07 ` Joel Brobecker
0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2018-10-09 10:44 UTC (permalink / raw)
To: Joel Brobecker, John Baldwin; +Cc: Tom Tromey, gdb-patches
On 10/08/2018 09:22 PM, Joel Brobecker wrote:
>> FWIW, I built GDB master today and ubsan (from LLVM, not GCC) flagged a
>> use of obstack_blank_fast() in minsyms.c with a negative offset (used to
>> shrink an obstack) when trying to do 'start' on /bin/ls:
>
> On my end, I am working on a resync of AdaCore's "head" version,
> which would include this change as well. My testing shows that
> it revealed some issues; I haven't had a chance to look into them yet
> (next on my list today, hopefully), but this is making me think
> there might be bona fide issues. Getting the location where things
> are happening is pretty nice :).
This also revealed PR 23743:
https://sourceware.org/bugzilla/show_bug.cgi?id=23743
I think the side-effects are blocking/disturbing enough (GDB dies) that I
think the best course of action is to temporarily disable the ubsan by
default until these issues are fixed. Then try re-enabling again, rinse/repeat.
Otherwise, as is, this is sort of imposing that the community prioritizes
fixing these ubsan-exposed bugs, which I think is unreasonable.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs
2018-10-09 10:44 ` Pedro Alves
@ 2018-10-12 21:07 ` Joel Brobecker
0 siblings, 0 replies; 29+ messages in thread
From: Joel Brobecker @ 2018-10-12 21:07 UTC (permalink / raw)
To: Pedro Alves; +Cc: John Baldwin, Tom Tromey, gdb-patches
> This also revealed PR 23743:
> https://sourceware.org/bugzilla/show_bug.cgi?id=23743
For the record, I've also discovered something I didn't realize
about struct memory allocations, which is a bit disturbing, because
I think we have a lot of areas where we malloc struct objects...
I reported it under https://sourceware.org/bugzilla/show_bug.cgi?id=23768
For now, it seems like the lessons learned is that it seems we should
be very careful when using XNEW/XCNEW with struct types?
----------------------------------------------------------------------
on ppc-linux, a GDB built with --enable-ubsan=yes crashes immediately as soon as one tries to enter anything at the prompt:
$ /path/to/gdb
GNU gdb (GDB) 8.2.50.20181012-git (with AdaCore local changes)
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
See your support agreement for details of warranty and support.
If you do not have a current support agreement, then there is absolutely
no warranty for this version of GDB.
Type "show copying" and "show warranty" for details.
This GDB was configured as "powerpc-generic-linux-gnu".
Type "show configuration" for configuration details.
For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) /homes/brobecke/act/gdb/gdb-head/gdb/common/common-exceptions.c:84:26: runtime error: member access within misaligned address 0x124af518 for type 'struct catcher', which requires 16 byte alignment
The code that triggers the error is fairly straightforward:
| jmp_buf *
| exceptions_state_mc_init (void)
| {
| struct catcher *new_catcher = XCNEW (struct catcher);
|
| /* Start with no exception. */
| new_catcher->exception = exception_none;
The problem happens because "struct catcher" is a structure which
requires a 16 bytes alignment, due to one of its members (the jmpbuf)
having itself a 16bytes alignment:
From bits/setjmp.h on ppc-linux:
| /* The current powerpc 32-bit Altivec ABI specifies for SVR4 ABI and EABI
| the vrsave must be at byte 248 & v20 at byte 256. So we must pad this
| correctly on 32 bit. It also insists that vecregs are only gauranteed
| 4 byte alignment so we need to use vperm in the setjmp/longjmp routines.
| We have to version the code because members like int __mask_was_saved
| in the jmp_buf will move as jmp_buf is now larger than 248 bytes. We
| cannot keep the altivec jmp_buf backward compatible with the jmp_buf. */
| #ifndef _ASM
| # if __WORDSIZE == 64
| typedef long int __jmp_buf[64] __attribute__ ((__aligned__ (16)));
| # else
| /* The alignment is not essential, i.e.the buffer can be copied to a 4 byte
| aligned buffer as per the ABI it is just added for performance reasons. */
| typedef long int __jmp_buf[64 + (12 * 4)] __attribute__ ((__aligned__ (16)));
| # endif
XCNEW performs calls to malloc without worrying about alignment,
so there is indeed no guaranty that the returned address is
aligned on 16 bytes. We looked at the GNU libc documentation,
for instance, and they guaranty 8 bytes only on 32bit machines
| https://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html
|
| 3.2.3.6 Allocating Aligned Memory Blocks
|
| The address of a block returned by malloc or realloc in GNU systems is
| always a multiple of eight (or sixteen on 64-bit systems). If you need a
| block whose address is a multiple of a higher power of two than that,
| use aligned_alloc or posix_memalign. aligned_alloc and posix_memalign
| are declared in stdlib.h.
We can certainly start by plugging the hole in this particular case,
and swap the call to XCNEW with a call to an aligned malloc. But
this made me realize we have a general issue, because potentially,
all struct allocations are potentially problematic, if they happen
to have alignment requirements that are stronger than what malloc
itself follows. I hope that, in practice, aligment requirements
beyond 4 or 8 bytes are rare.
Perhaps one possible idea was to have XNEW/XCNEW et all take the
alignment into account when performing the memory allocation
(we pass the type, so it could use alignof); however, that is
a major change affecting everyone. And looking closer at those
macros, one notices the following important comment...
/* Scalar allocators. */
... which tells me the macros may not have been meant to be used
in the context of allocating structures.
It's unclear to me what we would want to do...
--
Joel
^ permalink raw reply [flat|nested] 29+ messages in thread