* [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487]
@ 2023-12-08 13:49 Alexander Monakov
2023-12-08 13:49 ` [PATCH 1/1] object lifetime instrumentation for " Alexander Monakov
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Alexander Monakov @ 2023-12-08 13:49 UTC (permalink / raw)
To: gcc-patches
Cc: Arsen Arsenović, Sam James, Daniil Frolov, Alexander Monakov
I would like to propose Valgrind integration previously sent as RFC for trunk.
Arsen and Sam, since you commented on the RFC I wonder if you can have
a look at the proposed configure and documentation changes and let me
know if they look fine for you? For reference, gccinstall.info will say:
‘--enable-valgrind-interop’
Provide wrappers for Valgrind client requests in libgcc, which are
used for ‘-fvalgrind-annotations’. Requires Valgrind header files
for the target (in the build-time sysroot if building a
cross-compiler).
and GCC manual will document the new option as:
-fvalgrind-annotations
Emit Valgrind client requests annotating object lifetime
boundaries. This allows to detect attempts to access fields of a
C++ object after its destructor has completed (but storage was
not deallocated yet), or to initialize it in advance from
"operator new" rather than the constructor.
This instrumentation relies on presence of
"__gcc_vgmc_make_mem_undefined" function that wraps the
corresponding Valgrind client request. It is provided by libgcc
when it is configured with --enable-valgrind-interop. Otherwise,
you can implement it like this:
#include <valgrind/memcheck.h>
void
__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
{
VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
}
Changes since the RFC:
* Add documentation and tests.
* Drop 'emit-' from -fvalgrind-emit-annotations.
* Use --enable-valgrind-interop instead of overloading
--enable-valgrind-annotations.
* Do not build the wrapper unless --enable-valgrind-interop is given and
Valgrind headers are present.
* Clean up libgcc configure changes.
* Reword comments.
Daniil Frolov (1):
object lifetime instrumentation for Valgrind [PR66487]
gcc/Makefile.in | 1 +
gcc/builtins.def | 3 +
gcc/common.opt | 4 +
gcc/doc/install.texi | 5 +
gcc/doc/invoke.texi | 27 +++++
gcc/gimple-valgrind-interop.cc | 112 ++++++++++++++++++
gcc/passes.def | 1 +
gcc/testsuite/g++.dg/valgrind-annotations-1.C | 22 ++++
gcc/testsuite/g++.dg/valgrind-annotations-2.C | 12 ++
gcc/tree-pass.h | 1 +
libgcc/Makefile.in | 3 +
libgcc/config.in | 6 +
libgcc/configure | 22 +++-
libgcc/configure.ac | 15 ++-
libgcc/libgcc2.h | 2 +
libgcc/valgrind-interop.c | 40 +++++++
16 files changed, 274 insertions(+), 2 deletions(-)
create mode 100644 gcc/gimple-valgrind-interop.cc
create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-1.C
create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-2.C
create mode 100644 libgcc/valgrind-interop.c
--
2.39.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1] object lifetime instrumentation for Valgrind [PR66487]
2023-12-08 13:49 [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487] Alexander Monakov
@ 2023-12-08 13:49 ` Alexander Monakov
2023-12-08 14:04 ` [PATCH 0/1] Detecting lifetime-dse issues via " Jakub Jelinek
2023-12-22 14:10 ` [PATCH v2] object lifetime instrumentation for " Alexander Monakov
2 siblings, 0 replies; 13+ messages in thread
From: Alexander Monakov @ 2023-12-08 13:49 UTC (permalink / raw)
To: gcc-patches
Cc: Arsen Arsenović, Sam James, Daniil Frolov, Alexander Monakov
From: Daniil Frolov <exactlywb@ispras.ru>
PR 66487 is asking to provide sanitizer-like detection for C++ object
lifetime violations that are worked around with -fno-lifetime-dse or
-flifetime-dse=1 in Firefox, LLVM (PR 106943), OpenJade (PR 69534).
The discussion in the PR was centered around extending MSan, but MSan
was not ported to GCC (and requires rebuilding everything with
instrumentation).
Instead, allow Valgrind to see lifetime boundaries by emitting client
requests along *this = { CLOBBER }. The client request marks the
"clobbered" memory as undefined for Valgrind; clobbering assignments
mark the beginning of ctor and end of dtor execution for C++ objects.
Hence, attempts to read object storage after the destructor, or
"pre-initialize" its fields prior to the constructor will be caught.
Valgrind client requests are offered as macros that emit inline asm.
For use in code generation, let's wrap them as libgcc builtins.
gcc/ChangeLog:
* Makefile.in (OBJS): Add gimple-valgrind-interop.o.
* builtins.def (BUILT_IN_VALGRIND_MAKE_UNDEFINED): New.
* common.opt (-fvalgrind-annotations): New option.
* doc/install.texi (--enable-valgrind-interop): Document.
* doc/invoke.texi (-fvalgrind-annotations): Document.
* passes.def (pass_instrument_valgrind): Add.
* tree-pass.h (make_pass_instrument_valgrind): Declare.
* gimple-valgrind-interop.cc: New file.
libgcc/ChangeLog:
* Makefile.in (LIB2ADD): Add valgrind-interop.c.
* config.in: Regenerate.
* configure: Regenerate.
* configure.ac (--enable-valgrind-interop): New flag.
* libgcc2.h (__gcc_vgmc_make_mem_undefined): Declare.
* valgrind-interop.c: New file.
gcc/testsuite/ChangeLog:
* g++.dg/valgrind-annotations-1.C: New test.
* g++.dg/valgrind-annotations-2.C: New test.
Co-authored-by: Alexander Monakov <amonakov@ispras.ru>
---
gcc/Makefile.in | 1 +
gcc/builtins.def | 3 +
gcc/common.opt | 4 +
gcc/doc/install.texi | 5 +
gcc/doc/invoke.texi | 27 +++++
gcc/gimple-valgrind-interop.cc | 112 ++++++++++++++++++
gcc/passes.def | 1 +
gcc/testsuite/g++.dg/valgrind-annotations-1.C | 22 ++++
gcc/testsuite/g++.dg/valgrind-annotations-2.C | 12 ++
gcc/tree-pass.h | 1 +
libgcc/Makefile.in | 3 +
libgcc/config.in | 6 +
libgcc/configure | 22 +++-
libgcc/configure.ac | 15 ++-
libgcc/libgcc2.h | 2 +
libgcc/valgrind-interop.c | 40 +++++++
16 files changed, 274 insertions(+), 2 deletions(-)
create mode 100644 gcc/gimple-valgrind-interop.cc
create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-1.C
create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-2.C
create mode 100644 libgcc/valgrind-interop.c
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 68410a86af..4db18387c1 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1506,6 +1506,7 @@ OBJS = \
gimple-ssa-warn-restrict.o \
gimple-streamer-in.o \
gimple-streamer-out.o \
+ gimple-valgrind-interop.o \
gimple-walk.o \
gimple-warn-recursion.o \
gimplify.o \
diff --git a/gcc/builtins.def b/gcc/builtins.def
index f03df32f98..b05e20e062 100644
--- a/gcc/builtins.def
+++ b/gcc/builtins.def
@@ -1194,6 +1194,9 @@ DEF_GCC_BUILTIN (BUILT_IN_LINE, "LINE", BT_FN_INT, ATTR_NOTHROW_LEAF_LIST)
/* Control Flow Redundancy hardening out-of-line checker. */
DEF_BUILTIN_STUB (BUILT_IN___HARDCFR_CHECK, "__builtin___hardcfr_check")
+/* Wrappers for Valgrind client requests. */
+DEF_EXT_LIB_BUILTIN (BUILT_IN_VALGRIND_MAKE_UNDEFINED, "__gcc_vgmc_make_mem_undefined", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
+
/* Synchronization Primitives. */
#include "sync-builtins.def"
diff --git a/gcc/common.opt b/gcc/common.opt
index f070aff8cb..b53565fc1a 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -3372,6 +3372,10 @@ Enum(auto_init_type) String(pattern) Value(AUTO_INIT_PATTERN)
EnumValue
Enum(auto_init_type) String(zero) Value(AUTO_INIT_ZERO)
+fvalgrind-annotations
+Common Var(flag_valgrind_annotations) Optimization
+Annotate lifetime boundaries with Valgrind client requests.
+
; -fverbose-asm causes extra commentary information to be produced in
; the generated assembly code (to make it more readable). This option
; is generally only of use to those who actually need to read the
diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
index c1128d9274..aaf0213bbf 100644
--- a/gcc/doc/install.texi
+++ b/gcc/doc/install.texi
@@ -1567,6 +1567,11 @@ Disable TM clone registry in libgcc. It is enabled in libgcc by default.
This option helps to reduce code size for embedded targets which do
not use transactional memory.
+@item --enable-valgrind-interop
+Provide wrappers for Valgrind client requests in libgcc, which are used for
+@option{-fvalgrind-annotations}. Requires Valgrind header files for the
+target (in the build-time sysroot if building a cross-compiler).
+
@item --with-cpu=@var{cpu}
@itemx --with-cpu-32=@var{cpu}
@itemx --with-cpu-64=@var{cpu}
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 32f535e1ed..6befe7aa22 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -657,6 +657,7 @@ Objective-C and Objective-C++ Dialects}.
-fno-stack-limit -fsplit-stack
-fstrub=disable -fstrub=strict -fstrub=relaxed
-fstrub=all -fstrub=at-calls -fstrub=internal
+-fvalgrind-annotations
-fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
-fvtv-counts -fvtv-debug
-finstrument-functions -finstrument-functions-once
@@ -12920,6 +12921,9 @@ can use @option{-flifetime-dse=1}. The default behavior can be
explicitly selected with @option{-flifetime-dse=2}.
@option{-flifetime-dse=0} is equivalent to @option{-fno-lifetime-dse}.
+See @option{-fvalgrind-annotations} for instrumentation that allows to
+detect violations of standard lifetime semantics using Valgrind.
+
@opindex flive-range-shrinkage
@item -flive-range-shrinkage
Attempt to decrease register pressure through register live range
@@ -17976,6 +17980,29 @@ function attributes that tell which mode was selected for each function.
The primary use of this option is for testing, to exercise thoroughly
the @code{strub} machinery.
+@opindex fvalgrind-annotations
+@item -fvalgrind-annotations
+Emit Valgrind client requests annotating object lifetime boundaries.
+This allows to detect attempts to access fields of a C++ object after
+its destructor has completed (but storage was not deallocated yet), or
+to initialize it in advance from @code{operator new} rather than the
+constructor.
+
+This instrumentation relies on presence of @code{__gcc_vgmc_make_mem_undefined}
+function that wraps the corresponding Valgrind client request. It is provided
+by libgcc when it is configured with @samp{--enable-valgrind-interop}.
+Otherwise, you can implement it like this:
+
+@smallexample
+#include <valgrind/memcheck.h>
+
+void
+__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
+@{
+ VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
+@}
+@end smallexample
+
@opindex fvtable-verify
@item -fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
This option is only available when compiling C++ code.
diff --git a/gcc/gimple-valgrind-interop.cc b/gcc/gimple-valgrind-interop.cc
new file mode 100644
index 0000000000..839b1dd0ba
--- /dev/null
+++ b/gcc/gimple-valgrind-interop.cc
@@ -0,0 +1,112 @@
+/* Annotate lifetime boundaries for Valgrind.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation; either version 3, or (at your option) any
+later version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "cfg.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "gimple-iterator.h"
+#include "tree-pass.h"
+#include "gimplify-me.h"
+#include "fold-const.h"
+
+namespace {
+
+/* Emit VALGRIND_MAKE_MEM_UNDEFINED annotation for the object given by VAR. */
+
+bool
+annotate_undef (gimple_stmt_iterator *gsi, tree var)
+{
+ tree size = arg_size_in_bytes (TREE_TYPE (var));
+ if (size == size_zero_node)
+ return false;
+
+ tree addr = build_fold_addr_expr (var);
+ tree decl = builtin_decl_explicit (BUILT_IN_VALGRIND_MAKE_UNDEFINED);
+ tree call = build_call_expr (decl, 2, addr, size);
+
+ force_gimple_operand_gsi (gsi, call, false, NULL_TREE, false, GSI_NEW_STMT);
+ return true;
+}
+
+const pass_data pass_data_instrument_valgrind = {
+ GIMPLE_PASS, /* type */
+ "valgrind", /* name */
+ OPTGROUP_NONE, /* optinfo_flags */
+ TV_NONE, /* tv_id */
+ PROP_cfg, /* properties_required */
+ 0, /* properties_provided */
+ 0, /* properties_destroyed */
+ 0, /* todo_flags_start */
+ 0, /* todo_flags_finish */
+};
+
+}
+
+class pass_instrument_valgrind : public gimple_opt_pass
+{
+public:
+ pass_instrument_valgrind (gcc::context *ctxt)
+ : gimple_opt_pass (pass_data_instrument_valgrind, ctxt)
+ {
+ }
+
+ unsigned int execute (function *) final override;
+ bool gate (function *) final override;
+};
+
+bool
+pass_instrument_valgrind::gate (function *)
+{
+ return flag_valgrind_annotations;
+}
+
+/* Scan for GIMPLE clobbers that mark object lifetime boundaries and
+ make them known to Valgrind by emitting corresponding client requests. */
+
+unsigned int
+pass_instrument_valgrind::execute (function *fun)
+{
+ bool changed = false;
+ basic_block bb;
+ FOR_EACH_BB_FN (bb, fun)
+ {
+ for (gimple_stmt_iterator gsi = gsi_start_nondebug_bb (bb);
+ !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
+ {
+ gimple *stmt = gsi_stmt (gsi);
+
+ /* Leave CLOBBER_EOLs for ASan. */
+ if (gimple_clobber_p (stmt, CLOBBER_UNDEF))
+ changed |= annotate_undef (&gsi, gimple_assign_lhs (stmt));
+ }
+ }
+
+ return changed ? TODO_update_ssa : 0;
+}
+
+gimple_opt_pass *
+make_pass_instrument_valgrind (gcc::context *ctxt)
+{
+ return new pass_instrument_valgrind (ctxt);
+}
diff --git a/gcc/passes.def b/gcc/passes.def
index d3fccdf0a4..dade93135c 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -57,6 +57,7 @@ along with GCC; see the file COPYING3. If not see
PUSH_INSERT_PASSES_WITHIN (pass_build_ssa_passes)
NEXT_PASS (pass_fixup_cfg);
NEXT_PASS (pass_build_ssa);
+ NEXT_PASS (pass_instrument_valgrind);
NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
NEXT_PASS (pass_warn_printf);
NEXT_PASS (pass_warn_nonnull_compare);
diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-1.C b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
new file mode 100644
index 0000000000..49dc5a9cf1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
@@ -0,0 +1,22 @@
+/* { dg-do compile { target c++11 } } */
+/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
+
+#include <new>
+
+struct Foo
+{
+ int val;
+};
+
+struct Bar : Foo
+{
+ Bar() {}
+};
+
+int main ()
+{
+ alignas(Bar) char buf [sizeof (Bar)];
+ Bar *obj = new(buf) Bar;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-2.C b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
new file mode 100644
index 0000000000..a8b646d076
--- /dev/null
+++ b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target c++11 } } */
+/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
+
+struct Foo
+{
+ char buf [1024];
+ Foo () {}
+};
+
+Foo Obj;
+
+/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index de2820b3a3..2a9bf1f60f 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -440,6 +440,7 @@ extern gimple_opt_pass *make_pass_omp_device_lower (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_object_sizes (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_early_object_sizes (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_access (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_instrument_valgrind (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_printf (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_recursion (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_strlen (gcc::context *ctxt);
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index d8163c5af9..bb1e8d773c 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -436,6 +436,9 @@ LIB2ADD += $(srcdir)/hardcfr.c
# Stack scrubbing infrastructure.
LIB2ADD += $(srcdir)/strub.c
+# Wrappers for Valgrind client requests.
+LIB2ADD += $(srcdir)/valgrind-interop.c
+
# While emutls.c has nothing to do with EH, it is in LIB2ADDEH*
# instead of LIB2ADD because that's the way to be sure on some targets
# (e.g. *-*-darwin*) only one copy of it is linked.
diff --git a/libgcc/config.in b/libgcc/config.in
index f93c64a00c..19ad450680 100644
--- a/libgcc/config.in
+++ b/libgcc/config.in
@@ -3,6 +3,9 @@
/* Define to the .hidden-like directive if it exists. */
#undef AS_HIDDEN_DIRECTIVE
+/* Build Valgrind request wrappers */
+#undef ENABLE_VALGRIND_INTEROP
+
/* Define to 1 if the assembler supports AVX. */
#undef HAVE_AS_AVX
@@ -61,6 +64,9 @@
/* Define to 1 if you have the <unistd.h> header file. */
#undef HAVE_UNISTD_H
+/* Define to 1 if you have the <valgrind/memcheck.h> header file. */
+#undef HAVE_VALGRIND_MEMCHECK_H
+
/* Define to the address where bug reports for this package should be sent. */
#undef PACKAGE_BUGREPORT
diff --git a/libgcc/configure b/libgcc/configure
index cf14920965..580fda0cca 100755
--- a/libgcc/configure
+++ b/libgcc/configure
@@ -715,6 +715,7 @@ with_system_libunwind
enable_cet
enable_explicit_exception_frame_registration
enable_tm_clone_registry
+enable_valgrind_interop
with_glibc_version
enable_tls
with_gcc_major_version_only
@@ -1359,6 +1360,8 @@ Optional Features:
start, for use e.g. for compatibility with
installations without PT_GNU_EH_FRAME support
--disable-tm-clone-registry disable TM clone registry
+ --enable-valgrind-interop
+ build wrappers for Valgrind client requests
--enable-tls Use thread-local storage [default=yes]
Optional Packages:
@@ -4466,7 +4469,8 @@ as_fn_arith $ac_cv_sizeof_long_double \* 8 && long_double_type_size=$as_val
for ac_header in inttypes.h stdint.h stdlib.h ftw.h \
unistd.h sys/stat.h sys/types.h \
- string.h strings.h memory.h sys/auxv.h sys/mman.h
+ string.h strings.h memory.h sys/auxv.h sys/mman.h \
+ valgrind/memcheck.h
do :
as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -5024,6 +5028,22 @@ fi
+# Check whether --enable-valgrind-interop was given.
+if test "${enable_valgrind_interop+set}" = set; then :
+ enableval=$enable_valgrind_interop;
+else
+ enable_valgrind_interop=no
+fi
+
+if test $enable_valgrind_interop != no; then
+ if test $ac_cv_header_valgrind_memcheck_h = no; then
+ as_fn_error $? "--enable-valgrind-interop: valgrind/memcheck.h not found" "$LINENO" 5
+ fi
+
+$as_echo "#define ENABLE_VALGRIND_INTEROP 1" >>confdefs.h
+
+fi
+
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking if the linker ($LD) is GNU ld" >&5
$as_echo_n "checking if the linker ($LD) is GNU ld... " >&6; }
if ${acl_cv_prog_gnu_ld+:} false; then :
diff --git a/libgcc/configure.ac b/libgcc/configure.ac
index 2fc9d5d7c9..23289a81aa 100644
--- a/libgcc/configure.ac
+++ b/libgcc/configure.ac
@@ -231,7 +231,8 @@ AC_SUBST(long_double_type_size)
AC_CHECK_HEADERS(inttypes.h stdint.h stdlib.h ftw.h \
unistd.h sys/stat.h sys/types.h \
- string.h strings.h memory.h sys/auxv.h sys/mman.h)
+ string.h strings.h memory.h sys/auxv.h sys/mman.h \
+ valgrind/memcheck.h)
AC_HEADER_STDC
# Check for decimal float support.
@@ -303,6 +304,18 @@ esac
])
AC_SUBST([use_tm_clone_registry])
+AC_ARG_ENABLE([valgrind-interop],
+ [AC_HELP_STRING([--enable-valgrind-interop],
+ [build wrappers for Valgrind client requests])],
+ [],
+ [enable_valgrind_interop=no])
+if test $enable_valgrind_interop != no; then
+ if test $ac_cv_header_valgrind_memcheck_h = no; then
+ AC_MSG_ERROR([--enable-valgrind-interop: valgrind/memcheck.h not found])
+ fi
+ AC_DEFINE(ENABLE_VALGRIND_INTEROP, 1, [Build Valgrind request wrappers])
+fi
+
AC_LIB_PROG_LD_GNU
AC_MSG_CHECKING([for thread model used by GCC])
diff --git a/libgcc/libgcc2.h b/libgcc/libgcc2.h
index 750670e8ca..ee4500adc1 100644
--- a/libgcc/libgcc2.h
+++ b/libgcc/libgcc2.h
@@ -558,6 +558,8 @@ extern void __strub_enter (void **);
extern void __strub_update (void**);
extern void __strub_leave (void **);
+extern void __gcc_vgmc_make_mem_undefined (void *, size_t);
+
#ifndef HIDE_EXPORTS
#pragma GCC visibility pop
#endif
diff --git a/libgcc/valgrind-interop.c b/libgcc/valgrind-interop.c
new file mode 100644
index 0000000000..fdb8d41bc5
--- /dev/null
+++ b/libgcc/valgrind-interop.c
@@ -0,0 +1,40 @@
+/* Wrappers for Valgrind client requests.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
+<http://www.gnu.org/licenses/>. */
+
+#include "tsystem.h"
+#include "auto-target.h"
+
+#ifdef ENABLE_VALGRIND_INTEROP
+
+#include <valgrind/memcheck.h>
+
+/* Externally callable wrapper for Valgrind Memcheck client request:
+ declare SIZE bytes of memory at address ADDR as uninitialized. */
+
+void
+__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
+{
+ VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
+}
+#endif
--
2.39.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487]
2023-12-08 13:49 [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487] Alexander Monakov
2023-12-08 13:49 ` [PATCH 1/1] object lifetime instrumentation for " Alexander Monakov
@ 2023-12-08 14:04 ` Jakub Jelinek
2023-12-08 15:43 ` Alexander Monakov
2023-12-22 14:10 ` [PATCH v2] object lifetime instrumentation for " Alexander Monakov
2 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2023-12-08 14:04 UTC (permalink / raw)
To: Alexander Monakov
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Fri, Dec 08, 2023 at 04:49:49PM +0300, Alexander Monakov wrote:
> I would like to propose Valgrind integration previously sent as RFC for trunk.
>
> Arsen and Sam, since you commented on the RFC I wonder if you can have
> a look at the proposed configure and documentation changes and let me
> know if they look fine for you? For reference, gccinstall.info will say:
Does VALGRIND_MAKE_MEM_UNDEFINED macro ever change onarches once implemented
there? Wouldn't this be better done by emitting the sequence inline?
Even if it is done in libgcc, it is part of ABI.
So, basically add a new optab, valgrind_request, where each target would
define_insn whatever is needed (it will need to be a single pattern, it
can't be split among multiple) and sorry on -fvalgrind-annotations if the
optab is not defined.
Advantage would be that --enable-valgrind-interop nor building against
valgrind headers is not needed.
In your version, did the new function go just to libgcc.a or to
libgcc_s.so.1? Having a function in there or not dependent on
--enable-valgrind-interop would turn it into an ABI configure option.
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487]
2023-12-08 14:04 ` [PATCH 0/1] Detecting lifetime-dse issues via " Jakub Jelinek
@ 2023-12-08 15:43 ` Alexander Monakov
2023-12-08 16:04 ` Jakub Jelinek
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Monakov @ 2023-12-08 15:43 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Fri, 8 Dec 2023, Jakub Jelinek wrote:
> Does VALGRIND_MAKE_MEM_UNDEFINED macro ever change onarches once implemented
> there?
It seems Valgrind folks take binary compatibility seriously, so that sounds
unlikely.
> Wouldn't this be better done by emitting the sequence inline?
> Even if it is done in libgcc, it is part of ABI.
I'd rather keep it as simple as possible. We could drop the libgcc parts,
users can drop in the wrapper as explained in the manual.
> So, basically add a new optab, valgrind_request, where each target would
> define_insn whatever is needed (it will need to be a single pattern, it
> can't be split among multiple) and sorry on -fvalgrind-annotations if the
> optab is not defined.
There are going to be complications since the request needs a descriptor
structure (on the stack), plus it needs more effort on the GCC side than
Valgrind side (when Valgrind is ported to a new target). I'd rather not
go that way.
> Advantage would be that --enable-valgrind-interop nor building against
> valgrind headers is not needed.
Alternatively, how about synthesizing an auxiliary translation unit with
the wrapper from the driver for -fvalgrind-annotations?
> In your version, did the new function go just to libgcc.a or to
> libgcc_s.so.1?
It participates in libgcc_s link, but it's not listed in the version script,
so it's not exported from libgcc_s (and -gc-sections should eliminate it).
Alexander
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487]
2023-12-08 15:43 ` Alexander Monakov
@ 2023-12-08 16:04 ` Jakub Jelinek
2023-12-08 16:29 ` Alexander Monakov
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2023-12-08 16:04 UTC (permalink / raw)
To: Alexander Monakov
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Fri, Dec 08, 2023 at 06:43:19PM +0300, Alexander Monakov wrote:
> On Fri, 8 Dec 2023, Jakub Jelinek wrote:
> > In your version, did the new function go just to libgcc.a or to
> > libgcc_s.so.1?
>
> It participates in libgcc_s link, but it's not listed in the version script,
> so it's not exported from libgcc_s (and -gc-sections should eliminate it).
Then it at least should not participate in that link.
There are various other objects which are libgcc.a only (e.g. all of dfp
stuff, etc.).
Jakub
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487]
2023-12-08 16:04 ` Jakub Jelinek
@ 2023-12-08 16:29 ` Alexander Monakov
0 siblings, 0 replies; 13+ messages in thread
From: Alexander Monakov @ 2023-12-08 16:29 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Fri, 8 Dec 2023, Jakub Jelinek wrote:
> On Fri, Dec 08, 2023 at 06:43:19PM +0300, Alexander Monakov wrote:
> > On Fri, 8 Dec 2023, Jakub Jelinek wrote:
> > > In your version, did the new function go just to libgcc.a or to
> > > libgcc_s.so.1?
> >
> > It participates in libgcc_s link, but it's not listed in the version script,
> > so it's not exported from libgcc_s (and -gc-sections should eliminate it).
>
> Then it at least should not participate in that link.
> There are various other objects which are libgcc.a only (e.g. all of dfp
> stuff, etc.).
Thanks, changing
LIB2ADD += $(srcdir)/valgrind-interop.c
to
LIB2ADD_ST += $(srcdir)/valgrind-interop.c
in my tree achieved that.
Alexander
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2023-12-08 13:49 [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487] Alexander Monakov
2023-12-08 13:49 ` [PATCH 1/1] object lifetime instrumentation for " Alexander Monakov
2023-12-08 14:04 ` [PATCH 0/1] Detecting lifetime-dse issues via " Jakub Jelinek
@ 2023-12-22 14:10 ` Alexander Monakov
2024-05-15 10:59 ` Alexander Monakov
2 siblings, 1 reply; 13+ messages in thread
From: Alexander Monakov @ 2023-12-22 14:10 UTC (permalink / raw)
To: gcc-patches
Cc: Arsen Arsenović, Sam James, Daniil Frolov, Alexander Monakov
From: Daniil Frolov <exactlywb@ispras.ru>
PR 66487 is asking to provide sanitizer-like detection for C++ object
lifetime violations that are worked around with -fno-lifetime-dse or
-flifetime-dse=1 in Firefox, LLVM (PR 106943), OpenJade (PR 69534).
The discussion in the PR was centered around extending MSan, but MSan
was not ported to GCC (and requires rebuilding everything with
instrumentation).
Instead, allow Valgrind to see lifetime boundaries by emitting client
requests along *this = { CLOBBER }. The client request marks the
"clobbered" memory as undefined for Valgrind; clobbering assignments
mark the beginning of ctor and end of dtor execution for C++ objects.
Hence, attempts to read object storage after the destructor, or
"pre-initialize" its fields prior to the constructor will be caught.
Valgrind client requests are offered as macros that emit inline asm.
For use in code generation, let's wrap them as libgcc builtins.
gcc/ChangeLog:
* Makefile.in (OBJS): Add gimple-valgrind-interop.o.
* builtins.def (BUILT_IN_VALGRIND_MAKE_UNDEFINED): New.
* common.opt (-fvalgrind-annotations): New option.
* doc/install.texi (--enable-valgrind-interop): Document.
* doc/invoke.texi (-fvalgrind-annotations): Document.
* passes.def (pass_instrument_valgrind): Add.
* tree-pass.h (make_pass_instrument_valgrind): Declare.
* gimple-valgrind-interop.cc: New file.
libgcc/ChangeLog:
* Makefile.in (LIB2ADD_ST): Add valgrind-interop.c.
* config.in: Regenerate.
* configure: Regenerate.
* configure.ac (--enable-valgrind-interop): New flag.
* libgcc2.h (__gcc_vgmc_make_mem_undefined): Declare.
* valgrind-interop.c: New file.
gcc/testsuite/ChangeLog:
* g++.dg/valgrind-annotations-1.C: New test.
* g++.dg/valgrind-annotations-2.C: New test.
Co-authored-by: Alexander Monakov <amonakov@ispras.ru>
---
Changes in v2:
* Take new clobber kinds into account.
* Do not link valgrind-interop.o into libgcc_s.so.
gcc/Makefile.in | 1 +
gcc/builtins.def | 3 +
gcc/common.opt | 4 +
gcc/doc/install.texi | 5 +
gcc/doc/invoke.texi | 27 ++++
gcc/gimple-valgrind-interop.cc | 125 ++++++++++++++++++
gcc/passes.def | 1 +
gcc/testsuite/g++.dg/valgrind-annotations-1.C | 22 +++
gcc/testsuite/g++.dg/valgrind-annotations-2.C | 12 ++
gcc/tree-pass.h | 1 +
libgcc/Makefile.in | 3 +
libgcc/config.in | 6 +
libgcc/configure | 22 ++-
libgcc/configure.ac | 15 ++-
libgcc/libgcc2.h | 2 +
libgcc/valgrind-interop.c | 40 ++++++
16 files changed, 287 insertions(+), 2 deletions(-)
create mode 100644 gcc/gimple-valgrind-interop.cc
create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-1.C
create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-2.C
create mode 100644 libgcc/valgrind-interop.c
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 9373800018..d027548203 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1507,6 +1507,7 @@ OBJS = \
gimple-ssa-warn-restrict.o \
gimple-streamer-in.o \
gimple-streamer-out.o \
+ gimple-valgrind-interop.o \
gimple-walk.o \
gimple-warn-recursion.o \
gimplify.o \
diff --git a/gcc/builtins.def b/gcc/builtins.def
index f03df32f98..b05e20e062 100644
--- a/gcc/builtins.def
+++ b/gcc/builtins.def
@@ -1194,6 +1194,9 @@ DEF_GCC_BUILTIN (BUILT_IN_LINE, "LINE", BT_FN_INT, ATTR_NOTHROW_LEAF_LIST)
/* Control Flow Redundancy hardening out-of-line checker. */
DEF_BUILTIN_STUB (BUILT_IN___HARDCFR_CHECK, "__builtin___hardcfr_check")
+/* Wrappers for Valgrind client requests. */
+DEF_EXT_LIB_BUILTIN (BUILT_IN_VALGRIND_MAKE_UNDEFINED, "__gcc_vgmc_make_mem_undefined", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
+
/* Synchronization Primitives. */
#include "sync-builtins.def"
diff --git a/gcc/common.opt b/gcc/common.opt
index d263a959df..2be5b8d0a6 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -3377,6 +3377,10 @@ Enum(auto_init_type) String(pattern) Value(AUTO_INIT_PATTERN)
EnumValue
Enum(auto_init_type) String(zero) Value(AUTO_INIT_ZERO)
+fvalgrind-annotations
+Common Var(flag_valgrind_annotations) Optimization
+Annotate lifetime boundaries with Valgrind client requests.
+
; -fverbose-asm causes extra commentary information to be produced in
; the generated assembly code (to make it more readable). This option
; is generally only of use to those who actually need to read the
diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
index d20b43a5b2..d6e5e5fdaf 100644
--- a/gcc/doc/install.texi
+++ b/gcc/doc/install.texi
@@ -1563,6 +1563,11 @@ Disable TM clone registry in libgcc. It is enabled in libgcc by default.
This option helps to reduce code size for embedded targets which do
not use transactional memory.
+@item --enable-valgrind-interop
+Provide wrappers for Valgrind client requests in libgcc, which are used for
+@option{-fvalgrind-annotations}. Requires Valgrind header files for the
+target (in the build-time sysroot if building a cross-compiler).
+
@item --with-cpu=@var{cpu}
@itemx --with-cpu-32=@var{cpu}
@itemx --with-cpu-64=@var{cpu}
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d272b9228d..bfbe6a8bd9 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -660,6 +660,7 @@ Objective-C and Objective-C++ Dialects}.
-fno-stack-limit -fsplit-stack
-fstrub=disable -fstrub=strict -fstrub=relaxed
-fstrub=all -fstrub=at-calls -fstrub=internal
+-fvalgrind-annotations
-fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
-fvtv-counts -fvtv-debug
-finstrument-functions -finstrument-functions-once
@@ -12975,6 +12976,9 @@ can use @option{-flifetime-dse=1}. The default behavior can be
explicitly selected with @option{-flifetime-dse=2}.
@option{-flifetime-dse=0} is equivalent to @option{-fno-lifetime-dse}.
+See @option{-fvalgrind-annotations} for instrumentation that allows to
+detect violations of standard lifetime semantics using Valgrind.
+
@opindex flive-range-shrinkage
@item -flive-range-shrinkage
Attempt to decrease register pressure through register live range
@@ -18051,6 +18055,29 @@ function attributes that tell which mode was selected for each function.
The primary use of this option is for testing, to exercise thoroughly
the @code{strub} machinery.
+@opindex fvalgrind-annotations
+@item -fvalgrind-annotations
+Emit Valgrind client requests annotating object lifetime boundaries.
+This allows to detect attempts to access fields of a C++ object after
+its destructor has completed (but storage was not deallocated yet), or
+to initialize it in advance from @code{operator new} rather than the
+constructor.
+
+This instrumentation relies on presence of @code{__gcc_vgmc_make_mem_undefined}
+function that wraps the corresponding Valgrind client request. It is provided
+by libgcc when it is configured with @samp{--enable-valgrind-interop}.
+Otherwise, you can implement it like this:
+
+@smallexample
+#include <valgrind/memcheck.h>
+
+void
+__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
+@{
+ VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
+@}
+@end smallexample
+
@opindex fvtable-verify
@item -fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
This option is only available when compiling C++ code.
diff --git a/gcc/gimple-valgrind-interop.cc b/gcc/gimple-valgrind-interop.cc
new file mode 100644
index 0000000000..05d9bdc660
--- /dev/null
+++ b/gcc/gimple-valgrind-interop.cc
@@ -0,0 +1,125 @@
+/* Annotate lifetime boundaries for Valgrind.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation; either version 3, or (at your option) any
+later version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "cfg.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "gimple-iterator.h"
+#include "tree-pass.h"
+#include "gimplify-me.h"
+#include "fold-const.h"
+
+namespace {
+
+/* Emit VALGRIND_MAKE_MEM_UNDEFINED annotation for the object given by VAR. */
+
+bool
+annotate_undef (gimple_stmt_iterator *gsi, tree var)
+{
+ tree size = arg_size_in_bytes (TREE_TYPE (var));
+ if (size == size_zero_node)
+ return false;
+
+ tree addr = build_fold_addr_expr (var);
+ tree decl = builtin_decl_explicit (BUILT_IN_VALGRIND_MAKE_UNDEFINED);
+ tree call = build_call_expr (decl, 2, addr, size);
+
+ force_gimple_operand_gsi (gsi, call, false, NULL_TREE, false, GSI_NEW_STMT);
+ return true;
+}
+
+const pass_data pass_data_instrument_valgrind = {
+ GIMPLE_PASS, /* type */
+ "valgrind", /* name */
+ OPTGROUP_NONE, /* optinfo_flags */
+ TV_NONE, /* tv_id */
+ PROP_cfg, /* properties_required */
+ 0, /* properties_provided */
+ 0, /* properties_destroyed */
+ 0, /* todo_flags_start */
+ 0, /* todo_flags_finish */
+};
+
+}
+
+class pass_instrument_valgrind : public gimple_opt_pass
+{
+public:
+ pass_instrument_valgrind (gcc::context *ctxt)
+ : gimple_opt_pass (pass_data_instrument_valgrind, ctxt)
+ {
+ }
+
+ unsigned int execute (function *) final override;
+ bool gate (function *) final override;
+};
+
+bool
+pass_instrument_valgrind::gate (function *)
+{
+ return flag_valgrind_annotations;
+}
+
+/* Scan for GIMPLE clobbers that mark object lifetime boundaries and
+ make them known to Valgrind by emitting corresponding client requests. */
+
+unsigned int
+pass_instrument_valgrind::execute (function *fun)
+{
+ bool changed = false;
+ basic_block bb;
+ FOR_EACH_BB_FN (bb, fun)
+ {
+ for (gimple_stmt_iterator gsi = gsi_start_nondebug_bb (bb);
+ !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
+ {
+ gimple *stmt = gsi_stmt (gsi);
+
+ if (gimple_clobber_p (stmt))
+ switch (CLOBBER_KIND (gimple_assign_rhs1 (stmt)))
+ {
+ case CLOBBER_UNDEF:
+ case CLOBBER_OBJECT_BEGIN:
+ case CLOBBER_OBJECT_END:
+ changed |= annotate_undef (&gsi, gimple_assign_lhs (stmt));
+ break;
+ case CLOBBER_STORAGE_BEGIN:
+ case CLOBBER_STORAGE_END:
+ /* Leave these for ASan. */
+ break;
+ case CLOBBER_LAST:
+ default:
+ gcc_unreachable ();
+ }
+ }
+ }
+
+ return changed ? TODO_update_ssa : 0;
+}
+
+gimple_opt_pass *
+make_pass_instrument_valgrind (gcc::context *ctxt)
+{
+ return new pass_instrument_valgrind (ctxt);
+}
diff --git a/gcc/passes.def b/gcc/passes.def
index 43b416f98f..2274304321 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -57,6 +57,7 @@ along with GCC; see the file COPYING3. If not see
PUSH_INSERT_PASSES_WITHIN (pass_build_ssa_passes)
NEXT_PASS (pass_fixup_cfg);
NEXT_PASS (pass_build_ssa);
+ NEXT_PASS (pass_instrument_valgrind);
NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
NEXT_PASS (pass_warn_printf);
NEXT_PASS (pass_warn_nonnull_compare);
diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-1.C b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
new file mode 100644
index 0000000000..49dc5a9cf1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
@@ -0,0 +1,22 @@
+/* { dg-do compile { target c++11 } } */
+/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
+
+#include <new>
+
+struct Foo
+{
+ int val;
+};
+
+struct Bar : Foo
+{
+ Bar() {}
+};
+
+int main ()
+{
+ alignas(Bar) char buf [sizeof (Bar)];
+ Bar *obj = new(buf) Bar;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-2.C b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
new file mode 100644
index 0000000000..a8b646d076
--- /dev/null
+++ b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
@@ -0,0 +1,12 @@
+/* { dg-do compile { target c++11 } } */
+/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
+
+struct Foo
+{
+ char buf [1024];
+ Foo () {}
+};
+
+Foo Obj;
+
+/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 52fd57fd4c..49de431bd8 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -441,6 +441,7 @@ extern gimple_opt_pass *make_pass_omp_device_lower (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_object_sizes (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_early_object_sizes (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_access (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_instrument_valgrind (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_printf (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_recursion (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_strlen (gcc::context *ctxt);
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index 3f77283490..ea3ba705e9 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -436,6 +436,9 @@ LIB2ADD += $(srcdir)/hardcfr.c
# Stack scrubbing infrastructure.
@HAVE_STRUB_SUPPORT@LIB2ADD += $(srcdir)/strub.c
+# Wrappers for Valgrind client requests.
+LIB2ADD_ST += $(srcdir)/valgrind-interop.c
+
# While emutls.c has nothing to do with EH, it is in LIB2ADDEH*
# instead of LIB2ADD because that's the way to be sure on some targets
# (e.g. *-*-darwin*) only one copy of it is linked.
diff --git a/libgcc/config.in b/libgcc/config.in
index 8f7dd437b0..a77e9411fc 100644
--- a/libgcc/config.in
+++ b/libgcc/config.in
@@ -3,6 +3,9 @@
/* Define to the .hidden-like directive if it exists. */
#undef AS_HIDDEN_DIRECTIVE
+/* Build Valgrind request wrappers */
+#undef ENABLE_VALGRIND_INTEROP
+
/* Define to 1 if the assembler supports AVX. */
#undef HAVE_AS_AVX
@@ -64,6 +67,9 @@
/* Define to 1 if you have the <unistd.h> header file. */
#undef HAVE_UNISTD_H
+/* Define to 1 if you have the <valgrind/memcheck.h> header file. */
+#undef HAVE_VALGRIND_MEMCHECK_H
+
/* Define to 1 if __getauxval is available. */
#undef HAVE___GETAUXVAL
diff --git a/libgcc/configure b/libgcc/configure
index 3671d9b1a1..431c028af9 100755
--- a/libgcc/configure
+++ b/libgcc/configure
@@ -716,6 +716,7 @@ with_system_libunwind
enable_cet
enable_explicit_exception_frame_registration
enable_tm_clone_registry
+enable_valgrind_interop
with_glibc_version
enable_tls
with_gcc_major_version_only
@@ -1360,6 +1361,8 @@ Optional Features:
start, for use e.g. for compatibility with
installations without PT_GNU_EH_FRAME support
--disable-tm-clone-registry disable TM clone registry
+ --enable-valgrind-interop
+ build wrappers for Valgrind client requests
--enable-tls Use thread-local storage [default=yes]
Optional Packages:
@@ -4467,7 +4470,8 @@ as_fn_arith $ac_cv_sizeof_long_double \* 8 && long_double_type_size=$as_val
for ac_header in inttypes.h stdint.h stdlib.h ftw.h \
unistd.h sys/stat.h sys/types.h \
- string.h strings.h memory.h sys/auxv.h sys/mman.h
+ string.h strings.h memory.h sys/auxv.h sys/mman.h \
+ valgrind/memcheck.h
do :
as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -5025,6 +5029,22 @@ fi
+# Check whether --enable-valgrind-interop was given.
+if test "${enable_valgrind_interop+set}" = set; then :
+ enableval=$enable_valgrind_interop;
+else
+ enable_valgrind_interop=no
+fi
+
+if test $enable_valgrind_interop != no; then
+ if test $ac_cv_header_valgrind_memcheck_h = no; then
+ as_fn_error $? "--enable-valgrind-interop: valgrind/memcheck.h not found" "$LINENO" 5
+ fi
+
+$as_echo "#define ENABLE_VALGRIND_INTEROP 1" >>confdefs.h
+
+fi
+
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking if the linker ($LD) is GNU ld" >&5
$as_echo_n "checking if the linker ($LD) is GNU ld... " >&6; }
if ${acl_cv_prog_gnu_ld+:} false; then :
diff --git a/libgcc/configure.ac b/libgcc/configure.ac
index 467f5e63ef..d19f5e7e84 100644
--- a/libgcc/configure.ac
+++ b/libgcc/configure.ac
@@ -231,7 +231,8 @@ AC_SUBST(long_double_type_size)
AC_CHECK_HEADERS(inttypes.h stdint.h stdlib.h ftw.h \
unistd.h sys/stat.h sys/types.h \
- string.h strings.h memory.h sys/auxv.h sys/mman.h)
+ string.h strings.h memory.h sys/auxv.h sys/mman.h \
+ valgrind/memcheck.h)
AC_HEADER_STDC
# Check for decimal float support.
@@ -303,6 +304,18 @@ esac
])
AC_SUBST([use_tm_clone_registry])
+AC_ARG_ENABLE([valgrind-interop],
+ [AC_HELP_STRING([--enable-valgrind-interop],
+ [build wrappers for Valgrind client requests])],
+ [],
+ [enable_valgrind_interop=no])
+if test $enable_valgrind_interop != no; then
+ if test $ac_cv_header_valgrind_memcheck_h = no; then
+ AC_MSG_ERROR([--enable-valgrind-interop: valgrind/memcheck.h not found])
+ fi
+ AC_DEFINE(ENABLE_VALGRIND_INTEROP, 1, [Build Valgrind request wrappers])
+fi
+
AC_LIB_PROG_LD_GNU
AC_MSG_CHECKING([for thread model used by GCC])
diff --git a/libgcc/libgcc2.h b/libgcc/libgcc2.h
index 750670e8ca..ee4500adc1 100644
--- a/libgcc/libgcc2.h
+++ b/libgcc/libgcc2.h
@@ -558,6 +558,8 @@ extern void __strub_enter (void **);
extern void __strub_update (void**);
extern void __strub_leave (void **);
+extern void __gcc_vgmc_make_mem_undefined (void *, size_t);
+
#ifndef HIDE_EXPORTS
#pragma GCC visibility pop
#endif
diff --git a/libgcc/valgrind-interop.c b/libgcc/valgrind-interop.c
new file mode 100644
index 0000000000..fdb8d41bc5
--- /dev/null
+++ b/libgcc/valgrind-interop.c
@@ -0,0 +1,40 @@
+/* Wrappers for Valgrind client requests.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
+<http://www.gnu.org/licenses/>. */
+
+#include "tsystem.h"
+#include "auto-target.h"
+
+#ifdef ENABLE_VALGRIND_INTEROP
+
+#include <valgrind/memcheck.h>
+
+/* Externally callable wrapper for Valgrind Memcheck client request:
+ declare SIZE bytes of memory at address ADDR as uninitialized. */
+
+void
+__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
+{
+ VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
+}
+#endif
--
2.39.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2023-12-22 14:10 ` [PATCH v2] object lifetime instrumentation for " Alexander Monakov
@ 2024-05-15 10:59 ` Alexander Monakov
2024-05-28 7:26 ` Richard Biener
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Monakov @ 2024-05-15 10:59 UTC (permalink / raw)
To: gcc-patches; +Cc: Arsen Arsenović, Sam James, Daniil Frolov
Hello,
I'd like to ask if anyone has any new thoughts on this patch.
Let me also point out that valgrind/memcheck.h is permissively
licensed (BSD-style, rest of Valgrind is GPLv2), with the intention
to allow importing into projects that are interested in using
client requests without build-time dependency on installed headers.
So maybe we have that as an option too.
Alexander
On Fri, 22 Dec 2023, Alexander Monakov wrote:
> From: Daniil Frolov <exactlywb@ispras.ru>
>
> PR 66487 is asking to provide sanitizer-like detection for C++ object
> lifetime violations that are worked around with -fno-lifetime-dse or
> -flifetime-dse=1 in Firefox, LLVM (PR 106943), OpenJade (PR 69534).
>
> The discussion in the PR was centered around extending MSan, but MSan
> was not ported to GCC (and requires rebuilding everything with
> instrumentation).
>
> Instead, allow Valgrind to see lifetime boundaries by emitting client
> requests along *this = { CLOBBER }. The client request marks the
> "clobbered" memory as undefined for Valgrind; clobbering assignments
> mark the beginning of ctor and end of dtor execution for C++ objects.
> Hence, attempts to read object storage after the destructor, or
> "pre-initialize" its fields prior to the constructor will be caught.
>
> Valgrind client requests are offered as macros that emit inline asm.
> For use in code generation, let's wrap them as libgcc builtins.
>
> gcc/ChangeLog:
>
> * Makefile.in (OBJS): Add gimple-valgrind-interop.o.
> * builtins.def (BUILT_IN_VALGRIND_MAKE_UNDEFINED): New.
> * common.opt (-fvalgrind-annotations): New option.
> * doc/install.texi (--enable-valgrind-interop): Document.
> * doc/invoke.texi (-fvalgrind-annotations): Document.
> * passes.def (pass_instrument_valgrind): Add.
> * tree-pass.h (make_pass_instrument_valgrind): Declare.
> * gimple-valgrind-interop.cc: New file.
>
> libgcc/ChangeLog:
>
> * Makefile.in (LIB2ADD_ST): Add valgrind-interop.c.
> * config.in: Regenerate.
> * configure: Regenerate.
> * configure.ac (--enable-valgrind-interop): New flag.
> * libgcc2.h (__gcc_vgmc_make_mem_undefined): Declare.
> * valgrind-interop.c: New file.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/valgrind-annotations-1.C: New test.
> * g++.dg/valgrind-annotations-2.C: New test.
>
> Co-authored-by: Alexander Monakov <amonakov@ispras.ru>
> ---
> Changes in v2:
>
> * Take new clobber kinds into account.
> * Do not link valgrind-interop.o into libgcc_s.so.
>
> gcc/Makefile.in | 1 +
> gcc/builtins.def | 3 +
> gcc/common.opt | 4 +
> gcc/doc/install.texi | 5 +
> gcc/doc/invoke.texi | 27 ++++
> gcc/gimple-valgrind-interop.cc | 125 ++++++++++++++++++
> gcc/passes.def | 1 +
> gcc/testsuite/g++.dg/valgrind-annotations-1.C | 22 +++
> gcc/testsuite/g++.dg/valgrind-annotations-2.C | 12 ++
> gcc/tree-pass.h | 1 +
> libgcc/Makefile.in | 3 +
> libgcc/config.in | 6 +
> libgcc/configure | 22 ++-
> libgcc/configure.ac | 15 ++-
> libgcc/libgcc2.h | 2 +
> libgcc/valgrind-interop.c | 40 ++++++
> 16 files changed, 287 insertions(+), 2 deletions(-)
> create mode 100644 gcc/gimple-valgrind-interop.cc
> create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-1.C
> create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-2.C
> create mode 100644 libgcc/valgrind-interop.c
>
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 9373800018..d027548203 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1507,6 +1507,7 @@ OBJS = \
> gimple-ssa-warn-restrict.o \
> gimple-streamer-in.o \
> gimple-streamer-out.o \
> + gimple-valgrind-interop.o \
> gimple-walk.o \
> gimple-warn-recursion.o \
> gimplify.o \
> diff --git a/gcc/builtins.def b/gcc/builtins.def
> index f03df32f98..b05e20e062 100644
> --- a/gcc/builtins.def
> +++ b/gcc/builtins.def
> @@ -1194,6 +1194,9 @@ DEF_GCC_BUILTIN (BUILT_IN_LINE, "LINE", BT_FN_INT, ATTR_NOTHROW_LEAF_LIST)
> /* Control Flow Redundancy hardening out-of-line checker. */
> DEF_BUILTIN_STUB (BUILT_IN___HARDCFR_CHECK, "__builtin___hardcfr_check")
>
> +/* Wrappers for Valgrind client requests. */
> +DEF_EXT_LIB_BUILTIN (BUILT_IN_VALGRIND_MAKE_UNDEFINED, "__gcc_vgmc_make_mem_undefined", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
> +
> /* Synchronization Primitives. */
> #include "sync-builtins.def"
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index d263a959df..2be5b8d0a6 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -3377,6 +3377,10 @@ Enum(auto_init_type) String(pattern) Value(AUTO_INIT_PATTERN)
> EnumValue
> Enum(auto_init_type) String(zero) Value(AUTO_INIT_ZERO)
>
> +fvalgrind-annotations
> +Common Var(flag_valgrind_annotations) Optimization
> +Annotate lifetime boundaries with Valgrind client requests.
> +
> ; -fverbose-asm causes extra commentary information to be produced in
> ; the generated assembly code (to make it more readable). This option
> ; is generally only of use to those who actually need to read the
> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> index d20b43a5b2..d6e5e5fdaf 100644
> --- a/gcc/doc/install.texi
> +++ b/gcc/doc/install.texi
> @@ -1563,6 +1563,11 @@ Disable TM clone registry in libgcc. It is enabled in libgcc by default.
> This option helps to reduce code size for embedded targets which do
> not use transactional memory.
>
> +@item --enable-valgrind-interop
> +Provide wrappers for Valgrind client requests in libgcc, which are used for
> +@option{-fvalgrind-annotations}. Requires Valgrind header files for the
> +target (in the build-time sysroot if building a cross-compiler).
> +
> @item --with-cpu=@var{cpu}
> @itemx --with-cpu-32=@var{cpu}
> @itemx --with-cpu-64=@var{cpu}
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index d272b9228d..bfbe6a8bd9 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -660,6 +660,7 @@ Objective-C and Objective-C++ Dialects}.
> -fno-stack-limit -fsplit-stack
> -fstrub=disable -fstrub=strict -fstrub=relaxed
> -fstrub=all -fstrub=at-calls -fstrub=internal
> +-fvalgrind-annotations
> -fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
> -fvtv-counts -fvtv-debug
> -finstrument-functions -finstrument-functions-once
> @@ -12975,6 +12976,9 @@ can use @option{-flifetime-dse=1}. The default behavior can be
> explicitly selected with @option{-flifetime-dse=2}.
> @option{-flifetime-dse=0} is equivalent to @option{-fno-lifetime-dse}.
>
> +See @option{-fvalgrind-annotations} for instrumentation that allows to
> +detect violations of standard lifetime semantics using Valgrind.
> +
> @opindex flive-range-shrinkage
> @item -flive-range-shrinkage
> Attempt to decrease register pressure through register live range
> @@ -18051,6 +18055,29 @@ function attributes that tell which mode was selected for each function.
> The primary use of this option is for testing, to exercise thoroughly
> the @code{strub} machinery.
>
> +@opindex fvalgrind-annotations
> +@item -fvalgrind-annotations
> +Emit Valgrind client requests annotating object lifetime boundaries.
> +This allows to detect attempts to access fields of a C++ object after
> +its destructor has completed (but storage was not deallocated yet), or
> +to initialize it in advance from @code{operator new} rather than the
> +constructor.
> +
> +This instrumentation relies on presence of @code{__gcc_vgmc_make_mem_undefined}
> +function that wraps the corresponding Valgrind client request. It is provided
> +by libgcc when it is configured with @samp{--enable-valgrind-interop}.
> +Otherwise, you can implement it like this:
> +
> +@smallexample
> +#include <valgrind/memcheck.h>
> +
> +void
> +__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
> +@{
> + VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
> +@}
> +@end smallexample
> +
> @opindex fvtable-verify
> @item -fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
> This option is only available when compiling C++ code.
> diff --git a/gcc/gimple-valgrind-interop.cc b/gcc/gimple-valgrind-interop.cc
> new file mode 100644
> index 0000000000..05d9bdc660
> --- /dev/null
> +++ b/gcc/gimple-valgrind-interop.cc
> @@ -0,0 +1,125 @@
> +/* Annotate lifetime boundaries for Valgrind.
> + Copyright (C) 2023 Free Software Foundation, Inc.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it
> +under the terms of the GNU General Public License as published by the
> +Free Software Foundation; either version 3, or (at your option) any
> +later version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT
> +ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> +for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3. If not see
> +<http://www.gnu.org/licenses/>. */
> +
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tree.h"
> +#include "function.h"
> +#include "cfg.h"
> +#include "basic-block.h"
> +#include "gimple.h"
> +#include "gimple-iterator.h"
> +#include "tree-pass.h"
> +#include "gimplify-me.h"
> +#include "fold-const.h"
> +
> +namespace {
> +
> +/* Emit VALGRIND_MAKE_MEM_UNDEFINED annotation for the object given by VAR. */
> +
> +bool
> +annotate_undef (gimple_stmt_iterator *gsi, tree var)
> +{
> + tree size = arg_size_in_bytes (TREE_TYPE (var));
> + if (size == size_zero_node)
> + return false;
> +
> + tree addr = build_fold_addr_expr (var);
> + tree decl = builtin_decl_explicit (BUILT_IN_VALGRIND_MAKE_UNDEFINED);
> + tree call = build_call_expr (decl, 2, addr, size);
> +
> + force_gimple_operand_gsi (gsi, call, false, NULL_TREE, false, GSI_NEW_STMT);
> + return true;
> +}
> +
> +const pass_data pass_data_instrument_valgrind = {
> + GIMPLE_PASS, /* type */
> + "valgrind", /* name */
> + OPTGROUP_NONE, /* optinfo_flags */
> + TV_NONE, /* tv_id */
> + PROP_cfg, /* properties_required */
> + 0, /* properties_provided */
> + 0, /* properties_destroyed */
> + 0, /* todo_flags_start */
> + 0, /* todo_flags_finish */
> +};
> +
> +}
> +
> +class pass_instrument_valgrind : public gimple_opt_pass
> +{
> +public:
> + pass_instrument_valgrind (gcc::context *ctxt)
> + : gimple_opt_pass (pass_data_instrument_valgrind, ctxt)
> + {
> + }
> +
> + unsigned int execute (function *) final override;
> + bool gate (function *) final override;
> +};
> +
> +bool
> +pass_instrument_valgrind::gate (function *)
> +{
> + return flag_valgrind_annotations;
> +}
> +
> +/* Scan for GIMPLE clobbers that mark object lifetime boundaries and
> + make them known to Valgrind by emitting corresponding client requests. */
> +
> +unsigned int
> +pass_instrument_valgrind::execute (function *fun)
> +{
> + bool changed = false;
> + basic_block bb;
> + FOR_EACH_BB_FN (bb, fun)
> + {
> + for (gimple_stmt_iterator gsi = gsi_start_nondebug_bb (bb);
> + !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
> + {
> + gimple *stmt = gsi_stmt (gsi);
> +
> + if (gimple_clobber_p (stmt))
> + switch (CLOBBER_KIND (gimple_assign_rhs1 (stmt)))
> + {
> + case CLOBBER_UNDEF:
> + case CLOBBER_OBJECT_BEGIN:
> + case CLOBBER_OBJECT_END:
> + changed |= annotate_undef (&gsi, gimple_assign_lhs (stmt));
> + break;
> + case CLOBBER_STORAGE_BEGIN:
> + case CLOBBER_STORAGE_END:
> + /* Leave these for ASan. */
> + break;
> + case CLOBBER_LAST:
> + default:
> + gcc_unreachable ();
> + }
> + }
> + }
> +
> + return changed ? TODO_update_ssa : 0;
> +}
> +
> +gimple_opt_pass *
> +make_pass_instrument_valgrind (gcc::context *ctxt)
> +{
> + return new pass_instrument_valgrind (ctxt);
> +}
> diff --git a/gcc/passes.def b/gcc/passes.def
> index 43b416f98f..2274304321 100644
> --- a/gcc/passes.def
> +++ b/gcc/passes.def
> @@ -57,6 +57,7 @@ along with GCC; see the file COPYING3. If not see
> PUSH_INSERT_PASSES_WITHIN (pass_build_ssa_passes)
> NEXT_PASS (pass_fixup_cfg);
> NEXT_PASS (pass_build_ssa);
> + NEXT_PASS (pass_instrument_valgrind);
> NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
> NEXT_PASS (pass_warn_printf);
> NEXT_PASS (pass_warn_nonnull_compare);
> diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-1.C b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
> new file mode 100644
> index 0000000000..49dc5a9cf1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
> @@ -0,0 +1,22 @@
> +/* { dg-do compile { target c++11 } } */
> +/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
> +
> +#include <new>
> +
> +struct Foo
> +{
> + int val;
> +};
> +
> +struct Bar : Foo
> +{
> + Bar() {}
> +};
> +
> +int main ()
> +{
> + alignas(Bar) char buf [sizeof (Bar)];
> + Bar *obj = new(buf) Bar;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
> diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-2.C b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
> new file mode 100644
> index 0000000000..a8b646d076
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
> @@ -0,0 +1,12 @@
> +/* { dg-do compile { target c++11 } } */
> +/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
> +
> +struct Foo
> +{
> + char buf [1024];
> + Foo () {}
> +};
> +
> +Foo Obj;
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> index 52fd57fd4c..49de431bd8 100644
> --- a/gcc/tree-pass.h
> +++ b/gcc/tree-pass.h
> @@ -441,6 +441,7 @@ extern gimple_opt_pass *make_pass_omp_device_lower (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_object_sizes (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_early_object_sizes (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_warn_access (gcc::context *ctxt);
> +extern gimple_opt_pass *make_pass_instrument_valgrind (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_warn_printf (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_warn_recursion (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_strlen (gcc::context *ctxt);
> diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
> index 3f77283490..ea3ba705e9 100644
> --- a/libgcc/Makefile.in
> +++ b/libgcc/Makefile.in
> @@ -436,6 +436,9 @@ LIB2ADD += $(srcdir)/hardcfr.c
> # Stack scrubbing infrastructure.
> @HAVE_STRUB_SUPPORT@LIB2ADD += $(srcdir)/strub.c
>
> +# Wrappers for Valgrind client requests.
> +LIB2ADD_ST += $(srcdir)/valgrind-interop.c
> +
> # While emutls.c has nothing to do with EH, it is in LIB2ADDEH*
> # instead of LIB2ADD because that's the way to be sure on some targets
> # (e.g. *-*-darwin*) only one copy of it is linked.
> diff --git a/libgcc/config.in b/libgcc/config.in
> index 8f7dd437b0..a77e9411fc 100644
> --- a/libgcc/config.in
> +++ b/libgcc/config.in
> @@ -3,6 +3,9 @@
> /* Define to the .hidden-like directive if it exists. */
> #undef AS_HIDDEN_DIRECTIVE
>
> +/* Build Valgrind request wrappers */
> +#undef ENABLE_VALGRIND_INTEROP
> +
> /* Define to 1 if the assembler supports AVX. */
> #undef HAVE_AS_AVX
>
> @@ -64,6 +67,9 @@
> /* Define to 1 if you have the <unistd.h> header file. */
> #undef HAVE_UNISTD_H
>
> +/* Define to 1 if you have the <valgrind/memcheck.h> header file. */
> +#undef HAVE_VALGRIND_MEMCHECK_H
> +
> /* Define to 1 if __getauxval is available. */
> #undef HAVE___GETAUXVAL
>
> diff --git a/libgcc/configure b/libgcc/configure
> index 3671d9b1a1..431c028af9 100755
> --- a/libgcc/configure
> +++ b/libgcc/configure
> @@ -716,6 +716,7 @@ with_system_libunwind
> enable_cet
> enable_explicit_exception_frame_registration
> enable_tm_clone_registry
> +enable_valgrind_interop
> with_glibc_version
> enable_tls
> with_gcc_major_version_only
> @@ -1360,6 +1361,8 @@ Optional Features:
> start, for use e.g. for compatibility with
> installations without PT_GNU_EH_FRAME support
> --disable-tm-clone-registry disable TM clone registry
> + --enable-valgrind-interop
> + build wrappers for Valgrind client requests
> --enable-tls Use thread-local storage [default=yes]
>
> Optional Packages:
> @@ -4467,7 +4470,8 @@ as_fn_arith $ac_cv_sizeof_long_double \* 8 && long_double_type_size=$as_val
>
> for ac_header in inttypes.h stdint.h stdlib.h ftw.h \
> unistd.h sys/stat.h sys/types.h \
> - string.h strings.h memory.h sys/auxv.h sys/mman.h
> + string.h strings.h memory.h sys/auxv.h sys/mman.h \
> + valgrind/memcheck.h
> do :
> as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
> ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
> @@ -5025,6 +5029,22 @@ fi
>
>
>
> +# Check whether --enable-valgrind-interop was given.
> +if test "${enable_valgrind_interop+set}" = set; then :
> + enableval=$enable_valgrind_interop;
> +else
> + enable_valgrind_interop=no
> +fi
> +
> +if test $enable_valgrind_interop != no; then
> + if test $ac_cv_header_valgrind_memcheck_h = no; then
> + as_fn_error $? "--enable-valgrind-interop: valgrind/memcheck.h not found" "$LINENO" 5
> + fi
> +
> +$as_echo "#define ENABLE_VALGRIND_INTEROP 1" >>confdefs.h
> +
> +fi
> +
> { $as_echo "$as_me:${as_lineno-$LINENO}: checking if the linker ($LD) is GNU ld" >&5
> $as_echo_n "checking if the linker ($LD) is GNU ld... " >&6; }
> if ${acl_cv_prog_gnu_ld+:} false; then :
> diff --git a/libgcc/configure.ac b/libgcc/configure.ac
> index 467f5e63ef..d19f5e7e84 100644
> --- a/libgcc/configure.ac
> +++ b/libgcc/configure.ac
> @@ -231,7 +231,8 @@ AC_SUBST(long_double_type_size)
>
> AC_CHECK_HEADERS(inttypes.h stdint.h stdlib.h ftw.h \
> unistd.h sys/stat.h sys/types.h \
> - string.h strings.h memory.h sys/auxv.h sys/mman.h)
> + string.h strings.h memory.h sys/auxv.h sys/mman.h \
> + valgrind/memcheck.h)
> AC_HEADER_STDC
>
> # Check for decimal float support.
> @@ -303,6 +304,18 @@ esac
> ])
> AC_SUBST([use_tm_clone_registry])
>
> +AC_ARG_ENABLE([valgrind-interop],
> + [AC_HELP_STRING([--enable-valgrind-interop],
> + [build wrappers for Valgrind client requests])],
> + [],
> + [enable_valgrind_interop=no])
> +if test $enable_valgrind_interop != no; then
> + if test $ac_cv_header_valgrind_memcheck_h = no; then
> + AC_MSG_ERROR([--enable-valgrind-interop: valgrind/memcheck.h not found])
> + fi
> + AC_DEFINE(ENABLE_VALGRIND_INTEROP, 1, [Build Valgrind request wrappers])
> +fi
> +
> AC_LIB_PROG_LD_GNU
>
> AC_MSG_CHECKING([for thread model used by GCC])
> diff --git a/libgcc/libgcc2.h b/libgcc/libgcc2.h
> index 750670e8ca..ee4500adc1 100644
> --- a/libgcc/libgcc2.h
> +++ b/libgcc/libgcc2.h
> @@ -558,6 +558,8 @@ extern void __strub_enter (void **);
> extern void __strub_update (void**);
> extern void __strub_leave (void **);
>
> +extern void __gcc_vgmc_make_mem_undefined (void *, size_t);
> +
> #ifndef HIDE_EXPORTS
> #pragma GCC visibility pop
> #endif
> diff --git a/libgcc/valgrind-interop.c b/libgcc/valgrind-interop.c
> new file mode 100644
> index 0000000000..fdb8d41bc5
> --- /dev/null
> +++ b/libgcc/valgrind-interop.c
> @@ -0,0 +1,40 @@
> +/* Wrappers for Valgrind client requests.
> + Copyright (C) 2023 Free Software Foundation, Inc.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> +for more details.
> +
> +Under Section 7 of GPL version 3, you are granted additional
> +permissions described in the GCC Runtime Library Exception, version
> +3.1, as published by the Free Software Foundation.
> +
> +You should have received a copy of the GNU General Public License and
> +a copy of the GCC Runtime Library Exception along with this program;
> +see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
> +<http://www.gnu.org/licenses/>. */
> +
> +#include "tsystem.h"
> +#include "auto-target.h"
> +
> +#ifdef ENABLE_VALGRIND_INTEROP
> +
> +#include <valgrind/memcheck.h>
> +
> +/* Externally callable wrapper for Valgrind Memcheck client request:
> + declare SIZE bytes of memory at address ADDR as uninitialized. */
> +
> +void
> +__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
> +{
> + VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
> +}
> +#endif
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2024-05-15 10:59 ` Alexander Monakov
@ 2024-05-28 7:26 ` Richard Biener
2024-05-28 9:46 ` Alexander Monakov
0 siblings, 1 reply; 13+ messages in thread
From: Richard Biener @ 2024-05-28 7:26 UTC (permalink / raw)
To: Alexander Monakov
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Wed, May 15, 2024 at 12:59 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> Hello,
>
> I'd like to ask if anyone has any new thoughts on this patch.
>
> Let me also point out that valgrind/memcheck.h is permissively
> licensed (BSD-style, rest of Valgrind is GPLv2), with the intention
> to allow importing into projects that are interested in using
> client requests without build-time dependency on installed headers.
> So maybe we have that as an option too.
Inlining the VALGRIND_DO_CLIENT_REQUEST_EXPR would be a lot
cheaper and would not add to the libgcc ABI. I would guess the
valgrind "ABI" for these is practically fixed but of course architecture
dependent.
I do like the feature in general.
> Alexander
>
> On Fri, 22 Dec 2023, Alexander Monakov wrote:
>
> > From: Daniil Frolov <exactlywb@ispras.ru>
> >
> > PR 66487 is asking to provide sanitizer-like detection for C++ object
> > lifetime violations that are worked around with -fno-lifetime-dse or
> > -flifetime-dse=1 in Firefox, LLVM (PR 106943), OpenJade (PR 69534).
> >
> > The discussion in the PR was centered around extending MSan, but MSan
> > was not ported to GCC (and requires rebuilding everything with
> > instrumentation).
> >
> > Instead, allow Valgrind to see lifetime boundaries by emitting client
> > requests along *this = { CLOBBER }. The client request marks the
> > "clobbered" memory as undefined for Valgrind; clobbering assignments
> > mark the beginning of ctor and end of dtor execution for C++ objects.
> > Hence, attempts to read object storage after the destructor, or
> > "pre-initialize" its fields prior to the constructor will be caught.
> >
> > Valgrind client requests are offered as macros that emit inline asm.
> > For use in code generation, let's wrap them as libgcc builtins.
> >
> > gcc/ChangeLog:
> >
> > * Makefile.in (OBJS): Add gimple-valgrind-interop.o.
> > * builtins.def (BUILT_IN_VALGRIND_MAKE_UNDEFINED): New.
> > * common.opt (-fvalgrind-annotations): New option.
> > * doc/install.texi (--enable-valgrind-interop): Document.
> > * doc/invoke.texi (-fvalgrind-annotations): Document.
> > * passes.def (pass_instrument_valgrind): Add.
> > * tree-pass.h (make_pass_instrument_valgrind): Declare.
> > * gimple-valgrind-interop.cc: New file.
> >
> > libgcc/ChangeLog:
> >
> > * Makefile.in (LIB2ADD_ST): Add valgrind-interop.c.
> > * config.in: Regenerate.
> > * configure: Regenerate.
> > * configure.ac (--enable-valgrind-interop): New flag.
> > * libgcc2.h (__gcc_vgmc_make_mem_undefined): Declare.
> > * valgrind-interop.c: New file.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/valgrind-annotations-1.C: New test.
> > * g++.dg/valgrind-annotations-2.C: New test.
> >
> > Co-authored-by: Alexander Monakov <amonakov@ispras.ru>
> > ---
> > Changes in v2:
> >
> > * Take new clobber kinds into account.
> > * Do not link valgrind-interop.o into libgcc_s.so.
> >
> > gcc/Makefile.in | 1 +
> > gcc/builtins.def | 3 +
> > gcc/common.opt | 4 +
> > gcc/doc/install.texi | 5 +
> > gcc/doc/invoke.texi | 27 ++++
> > gcc/gimple-valgrind-interop.cc | 125 ++++++++++++++++++
> > gcc/passes.def | 1 +
> > gcc/testsuite/g++.dg/valgrind-annotations-1.C | 22 +++
> > gcc/testsuite/g++.dg/valgrind-annotations-2.C | 12 ++
> > gcc/tree-pass.h | 1 +
> > libgcc/Makefile.in | 3 +
> > libgcc/config.in | 6 +
> > libgcc/configure | 22 ++-
> > libgcc/configure.ac | 15 ++-
> > libgcc/libgcc2.h | 2 +
> > libgcc/valgrind-interop.c | 40 ++++++
> > 16 files changed, 287 insertions(+), 2 deletions(-)
> > create mode 100644 gcc/gimple-valgrind-interop.cc
> > create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-1.C
> > create mode 100644 gcc/testsuite/g++.dg/valgrind-annotations-2.C
> > create mode 100644 libgcc/valgrind-interop.c
> >
> > diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> > index 9373800018..d027548203 100644
> > --- a/gcc/Makefile.in
> > +++ b/gcc/Makefile.in
> > @@ -1507,6 +1507,7 @@ OBJS = \
> > gimple-ssa-warn-restrict.o \
> > gimple-streamer-in.o \
> > gimple-streamer-out.o \
> > + gimple-valgrind-interop.o \
> > gimple-walk.o \
> > gimple-warn-recursion.o \
> > gimplify.o \
> > diff --git a/gcc/builtins.def b/gcc/builtins.def
> > index f03df32f98..b05e20e062 100644
> > --- a/gcc/builtins.def
> > +++ b/gcc/builtins.def
> > @@ -1194,6 +1194,9 @@ DEF_GCC_BUILTIN (BUILT_IN_LINE, "LINE", BT_FN_INT, ATTR_NOTHROW_LEAF_LIST)
> > /* Control Flow Redundancy hardening out-of-line checker. */
> > DEF_BUILTIN_STUB (BUILT_IN___HARDCFR_CHECK, "__builtin___hardcfr_check")
> >
> > +/* Wrappers for Valgrind client requests. */
> > +DEF_EXT_LIB_BUILTIN (BUILT_IN_VALGRIND_MAKE_UNDEFINED, "__gcc_vgmc_make_mem_undefined", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
> > +
> > /* Synchronization Primitives. */
> > #include "sync-builtins.def"
> >
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index d263a959df..2be5b8d0a6 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -3377,6 +3377,10 @@ Enum(auto_init_type) String(pattern) Value(AUTO_INIT_PATTERN)
> > EnumValue
> > Enum(auto_init_type) String(zero) Value(AUTO_INIT_ZERO)
> >
> > +fvalgrind-annotations
> > +Common Var(flag_valgrind_annotations) Optimization
> > +Annotate lifetime boundaries with Valgrind client requests.
> > +
> > ; -fverbose-asm causes extra commentary information to be produced in
> > ; the generated assembly code (to make it more readable). This option
> > ; is generally only of use to those who actually need to read the
> > diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> > index d20b43a5b2..d6e5e5fdaf 100644
> > --- a/gcc/doc/install.texi
> > +++ b/gcc/doc/install.texi
> > @@ -1563,6 +1563,11 @@ Disable TM clone registry in libgcc. It is enabled in libgcc by default.
> > This option helps to reduce code size for embedded targets which do
> > not use transactional memory.
> >
> > +@item --enable-valgrind-interop
> > +Provide wrappers for Valgrind client requests in libgcc, which are used for
> > +@option{-fvalgrind-annotations}. Requires Valgrind header files for the
> > +target (in the build-time sysroot if building a cross-compiler).
> > +
> > @item --with-cpu=@var{cpu}
> > @itemx --with-cpu-32=@var{cpu}
> > @itemx --with-cpu-64=@var{cpu}
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index d272b9228d..bfbe6a8bd9 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -660,6 +660,7 @@ Objective-C and Objective-C++ Dialects}.
> > -fno-stack-limit -fsplit-stack
> > -fstrub=disable -fstrub=strict -fstrub=relaxed
> > -fstrub=all -fstrub=at-calls -fstrub=internal
> > +-fvalgrind-annotations
> > -fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
> > -fvtv-counts -fvtv-debug
> > -finstrument-functions -finstrument-functions-once
> > @@ -12975,6 +12976,9 @@ can use @option{-flifetime-dse=1}. The default behavior can be
> > explicitly selected with @option{-flifetime-dse=2}.
> > @option{-flifetime-dse=0} is equivalent to @option{-fno-lifetime-dse}.
> >
> > +See @option{-fvalgrind-annotations} for instrumentation that allows to
> > +detect violations of standard lifetime semantics using Valgrind.
> > +
> > @opindex flive-range-shrinkage
> > @item -flive-range-shrinkage
> > Attempt to decrease register pressure through register live range
> > @@ -18051,6 +18055,29 @@ function attributes that tell which mode was selected for each function.
> > The primary use of this option is for testing, to exercise thoroughly
> > the @code{strub} machinery.
> >
> > +@opindex fvalgrind-annotations
> > +@item -fvalgrind-annotations
> > +Emit Valgrind client requests annotating object lifetime boundaries.
> > +This allows to detect attempts to access fields of a C++ object after
> > +its destructor has completed (but storage was not deallocated yet), or
> > +to initialize it in advance from @code{operator new} rather than the
> > +constructor.
> > +
> > +This instrumentation relies on presence of @code{__gcc_vgmc_make_mem_undefined}
> > +function that wraps the corresponding Valgrind client request. It is provided
> > +by libgcc when it is configured with @samp{--enable-valgrind-interop}.
> > +Otherwise, you can implement it like this:
> > +
> > +@smallexample
> > +#include <valgrind/memcheck.h>
> > +
> > +void
> > +__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
> > +@{
> > + VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
> > +@}
> > +@end smallexample
> > +
> > @opindex fvtable-verify
> > @item -fvtable-verify=@r{[}std@r{|}preinit@r{|}none@r{]}
> > This option is only available when compiling C++ code.
> > diff --git a/gcc/gimple-valgrind-interop.cc b/gcc/gimple-valgrind-interop.cc
> > new file mode 100644
> > index 0000000000..05d9bdc660
> > --- /dev/null
> > +++ b/gcc/gimple-valgrind-interop.cc
> > @@ -0,0 +1,125 @@
> > +/* Annotate lifetime boundaries for Valgrind.
> > + Copyright (C) 2023 Free Software Foundation, Inc.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it
> > +under the terms of the GNU General Public License as published by the
> > +Free Software Foundation; either version 3, or (at your option) any
> > +later version.
> > +
> > +GCC is distributed in the hope that it will be useful, but WITHOUT
> > +ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > +for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with GCC; see the file COPYING3. If not see
> > +<http://www.gnu.org/licenses/>. */
> > +
> > +#include "config.h"
> > +#include "system.h"
> > +#include "coretypes.h"
> > +#include "tree.h"
> > +#include "function.h"
> > +#include "cfg.h"
> > +#include "basic-block.h"
> > +#include "gimple.h"
> > +#include "gimple-iterator.h"
> > +#include "tree-pass.h"
> > +#include "gimplify-me.h"
> > +#include "fold-const.h"
> > +
> > +namespace {
> > +
> > +/* Emit VALGRIND_MAKE_MEM_UNDEFINED annotation for the object given by VAR. */
> > +
> > +bool
> > +annotate_undef (gimple_stmt_iterator *gsi, tree var)
> > +{
> > + tree size = arg_size_in_bytes (TREE_TYPE (var));
> > + if (size == size_zero_node)
> > + return false;
> > +
> > + tree addr = build_fold_addr_expr (var);
> > + tree decl = builtin_decl_explicit (BUILT_IN_VALGRIND_MAKE_UNDEFINED);
> > + tree call = build_call_expr (decl, 2, addr, size);
> > +
> > + force_gimple_operand_gsi (gsi, call, false, NULL_TREE, false, GSI_NEW_STMT);
> > + return true;
> > +}
> > +
> > +const pass_data pass_data_instrument_valgrind = {
> > + GIMPLE_PASS, /* type */
> > + "valgrind", /* name */
> > + OPTGROUP_NONE, /* optinfo_flags */
> > + TV_NONE, /* tv_id */
> > + PROP_cfg, /* properties_required */
> > + 0, /* properties_provided */
> > + 0, /* properties_destroyed */
> > + 0, /* todo_flags_start */
> > + 0, /* todo_flags_finish */
> > +};
> > +
> > +}
> > +
> > +class pass_instrument_valgrind : public gimple_opt_pass
> > +{
> > +public:
> > + pass_instrument_valgrind (gcc::context *ctxt)
> > + : gimple_opt_pass (pass_data_instrument_valgrind, ctxt)
> > + {
> > + }
> > +
> > + unsigned int execute (function *) final override;
> > + bool gate (function *) final override;
> > +};
> > +
> > +bool
> > +pass_instrument_valgrind::gate (function *)
> > +{
> > + return flag_valgrind_annotations;
> > +}
> > +
> > +/* Scan for GIMPLE clobbers that mark object lifetime boundaries and
> > + make them known to Valgrind by emitting corresponding client requests. */
> > +
> > +unsigned int
> > +pass_instrument_valgrind::execute (function *fun)
> > +{
> > + bool changed = false;
> > + basic_block bb;
> > + FOR_EACH_BB_FN (bb, fun)
> > + {
> > + for (gimple_stmt_iterator gsi = gsi_start_nondebug_bb (bb);
> > + !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
> > + {
> > + gimple *stmt = gsi_stmt (gsi);
> > +
> > + if (gimple_clobber_p (stmt))
> > + switch (CLOBBER_KIND (gimple_assign_rhs1 (stmt)))
> > + {
> > + case CLOBBER_UNDEF:
> > + case CLOBBER_OBJECT_BEGIN:
> > + case CLOBBER_OBJECT_END:
> > + changed |= annotate_undef (&gsi, gimple_assign_lhs (stmt));
> > + break;
> > + case CLOBBER_STORAGE_BEGIN:
> > + case CLOBBER_STORAGE_END:
> > + /* Leave these for ASan. */
> > + break;
> > + case CLOBBER_LAST:
> > + default:
> > + gcc_unreachable ();
> > + }
> > + }
> > + }
> > +
> > + return changed ? TODO_update_ssa : 0;
> > +}
> > +
> > +gimple_opt_pass *
> > +make_pass_instrument_valgrind (gcc::context *ctxt)
> > +{
> > + return new pass_instrument_valgrind (ctxt);
> > +}
> > diff --git a/gcc/passes.def b/gcc/passes.def
> > index 43b416f98f..2274304321 100644
> > --- a/gcc/passes.def
> > +++ b/gcc/passes.def
> > @@ -57,6 +57,7 @@ along with GCC; see the file COPYING3. If not see
> > PUSH_INSERT_PASSES_WITHIN (pass_build_ssa_passes)
> > NEXT_PASS (pass_fixup_cfg);
> > NEXT_PASS (pass_build_ssa);
> > + NEXT_PASS (pass_instrument_valgrind);
> > NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
> > NEXT_PASS (pass_warn_printf);
> > NEXT_PASS (pass_warn_nonnull_compare);
> > diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-1.C b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
> > new file mode 100644
> > index 0000000000..49dc5a9cf1
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/valgrind-annotations-1.C
> > @@ -0,0 +1,22 @@
> > +/* { dg-do compile { target c++11 } } */
> > +/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
> > +
> > +#include <new>
> > +
> > +struct Foo
> > +{
> > + int val;
> > +};
> > +
> > +struct Bar : Foo
> > +{
> > + Bar() {}
> > +};
> > +
> > +int main ()
> > +{
> > + alignas(Bar) char buf [sizeof (Bar)];
> > + Bar *obj = new(buf) Bar;
> > +}
> > +
> > +/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
> > diff --git a/gcc/testsuite/g++.dg/valgrind-annotations-2.C b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
> > new file mode 100644
> > index 0000000000..a8b646d076
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/valgrind-annotations-2.C
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile { target c++11 } } */
> > +/* { dg-options "-O2 -fvalgrind-annotations -fdump-tree-optimized" } */
> > +
> > +struct Foo
> > +{
> > + char buf [1024];
> > + Foo () {}
> > +};
> > +
> > +Foo Obj;
> > +
> > +/* { dg-final { scan-tree-dump-times "__builtin___gcc_vgmc_make_mem_undefined" 1 "optimized" } } */
> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> > index 52fd57fd4c..49de431bd8 100644
> > --- a/gcc/tree-pass.h
> > +++ b/gcc/tree-pass.h
> > @@ -441,6 +441,7 @@ extern gimple_opt_pass *make_pass_omp_device_lower (gcc::context *ctxt);
> > extern gimple_opt_pass *make_pass_object_sizes (gcc::context *ctxt);
> > extern gimple_opt_pass *make_pass_early_object_sizes (gcc::context *ctxt);
> > extern gimple_opt_pass *make_pass_warn_access (gcc::context *ctxt);
> > +extern gimple_opt_pass *make_pass_instrument_valgrind (gcc::context *ctxt);
> > extern gimple_opt_pass *make_pass_warn_printf (gcc::context *ctxt);
> > extern gimple_opt_pass *make_pass_warn_recursion (gcc::context *ctxt);
> > extern gimple_opt_pass *make_pass_strlen (gcc::context *ctxt);
> > diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
> > index 3f77283490..ea3ba705e9 100644
> > --- a/libgcc/Makefile.in
> > +++ b/libgcc/Makefile.in
> > @@ -436,6 +436,9 @@ LIB2ADD += $(srcdir)/hardcfr.c
> > # Stack scrubbing infrastructure.
> > @HAVE_STRUB_SUPPORT@LIB2ADD += $(srcdir)/strub.c
> >
> > +# Wrappers for Valgrind client requests.
> > +LIB2ADD_ST += $(srcdir)/valgrind-interop.c
> > +
> > # While emutls.c has nothing to do with EH, it is in LIB2ADDEH*
> > # instead of LIB2ADD because that's the way to be sure on some targets
> > # (e.g. *-*-darwin*) only one copy of it is linked.
> > diff --git a/libgcc/config.in b/libgcc/config.in
> > index 8f7dd437b0..a77e9411fc 100644
> > --- a/libgcc/config.in
> > +++ b/libgcc/config.in
> > @@ -3,6 +3,9 @@
> > /* Define to the .hidden-like directive if it exists. */
> > #undef AS_HIDDEN_DIRECTIVE
> >
> > +/* Build Valgrind request wrappers */
> > +#undef ENABLE_VALGRIND_INTEROP
> > +
> > /* Define to 1 if the assembler supports AVX. */
> > #undef HAVE_AS_AVX
> >
> > @@ -64,6 +67,9 @@
> > /* Define to 1 if you have the <unistd.h> header file. */
> > #undef HAVE_UNISTD_H
> >
> > +/* Define to 1 if you have the <valgrind/memcheck.h> header file. */
> > +#undef HAVE_VALGRIND_MEMCHECK_H
> > +
> > /* Define to 1 if __getauxval is available. */
> > #undef HAVE___GETAUXVAL
> >
> > diff --git a/libgcc/configure b/libgcc/configure
> > index 3671d9b1a1..431c028af9 100755
> > --- a/libgcc/configure
> > +++ b/libgcc/configure
> > @@ -716,6 +716,7 @@ with_system_libunwind
> > enable_cet
> > enable_explicit_exception_frame_registration
> > enable_tm_clone_registry
> > +enable_valgrind_interop
> > with_glibc_version
> > enable_tls
> > with_gcc_major_version_only
> > @@ -1360,6 +1361,8 @@ Optional Features:
> > start, for use e.g. for compatibility with
> > installations without PT_GNU_EH_FRAME support
> > --disable-tm-clone-registry disable TM clone registry
> > + --enable-valgrind-interop
> > + build wrappers for Valgrind client requests
> > --enable-tls Use thread-local storage [default=yes]
> >
> > Optional Packages:
> > @@ -4467,7 +4470,8 @@ as_fn_arith $ac_cv_sizeof_long_double \* 8 && long_double_type_size=$as_val
> >
> > for ac_header in inttypes.h stdint.h stdlib.h ftw.h \
> > unistd.h sys/stat.h sys/types.h \
> > - string.h strings.h memory.h sys/auxv.h sys/mman.h
> > + string.h strings.h memory.h sys/auxv.h sys/mman.h \
> > + valgrind/memcheck.h
> > do :
> > as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
> > ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
> > @@ -5025,6 +5029,22 @@ fi
> >
> >
> >
> > +# Check whether --enable-valgrind-interop was given.
> > +if test "${enable_valgrind_interop+set}" = set; then :
> > + enableval=$enable_valgrind_interop;
> > +else
> > + enable_valgrind_interop=no
> > +fi
> > +
> > +if test $enable_valgrind_interop != no; then
> > + if test $ac_cv_header_valgrind_memcheck_h = no; then
> > + as_fn_error $? "--enable-valgrind-interop: valgrind/memcheck.h not found" "$LINENO" 5
> > + fi
> > +
> > +$as_echo "#define ENABLE_VALGRIND_INTEROP 1" >>confdefs.h
> > +
> > +fi
> > +
> > { $as_echo "$as_me:${as_lineno-$LINENO}: checking if the linker ($LD) is GNU ld" >&5
> > $as_echo_n "checking if the linker ($LD) is GNU ld... " >&6; }
> > if ${acl_cv_prog_gnu_ld+:} false; then :
> > diff --git a/libgcc/configure.ac b/libgcc/configure.ac
> > index 467f5e63ef..d19f5e7e84 100644
> > --- a/libgcc/configure.ac
> > +++ b/libgcc/configure.ac
> > @@ -231,7 +231,8 @@ AC_SUBST(long_double_type_size)
> >
> > AC_CHECK_HEADERS(inttypes.h stdint.h stdlib.h ftw.h \
> > unistd.h sys/stat.h sys/types.h \
> > - string.h strings.h memory.h sys/auxv.h sys/mman.h)
> > + string.h strings.h memory.h sys/auxv.h sys/mman.h \
> > + valgrind/memcheck.h)
> > AC_HEADER_STDC
> >
> > # Check for decimal float support.
> > @@ -303,6 +304,18 @@ esac
> > ])
> > AC_SUBST([use_tm_clone_registry])
> >
> > +AC_ARG_ENABLE([valgrind-interop],
> > + [AC_HELP_STRING([--enable-valgrind-interop],
> > + [build wrappers for Valgrind client requests])],
> > + [],
> > + [enable_valgrind_interop=no])
> > +if test $enable_valgrind_interop != no; then
> > + if test $ac_cv_header_valgrind_memcheck_h = no; then
> > + AC_MSG_ERROR([--enable-valgrind-interop: valgrind/memcheck.h not found])
> > + fi
> > + AC_DEFINE(ENABLE_VALGRIND_INTEROP, 1, [Build Valgrind request wrappers])
> > +fi
> > +
> > AC_LIB_PROG_LD_GNU
> >
> > AC_MSG_CHECKING([for thread model used by GCC])
> > diff --git a/libgcc/libgcc2.h b/libgcc/libgcc2.h
> > index 750670e8ca..ee4500adc1 100644
> > --- a/libgcc/libgcc2.h
> > +++ b/libgcc/libgcc2.h
> > @@ -558,6 +558,8 @@ extern void __strub_enter (void **);
> > extern void __strub_update (void**);
> > extern void __strub_leave (void **);
> >
> > +extern void __gcc_vgmc_make_mem_undefined (void *, size_t);
> > +
> > #ifndef HIDE_EXPORTS
> > #pragma GCC visibility pop
> > #endif
> > diff --git a/libgcc/valgrind-interop.c b/libgcc/valgrind-interop.c
> > new file mode 100644
> > index 0000000000..fdb8d41bc5
> > --- /dev/null
> > +++ b/libgcc/valgrind-interop.c
> > @@ -0,0 +1,40 @@
> > +/* Wrappers for Valgrind client requests.
> > + Copyright (C) 2023 Free Software Foundation, Inc.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it under
> > +the terms of the GNU General Public License as published by the Free
> > +Software Foundation; either version 3, or (at your option) any later
> > +version.
> > +
> > +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> > +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > +for more details.
> > +
> > +Under Section 7 of GPL version 3, you are granted additional
> > +permissions described in the GCC Runtime Library Exception, version
> > +3.1, as published by the Free Software Foundation.
> > +
> > +You should have received a copy of the GNU General Public License and
> > +a copy of the GCC Runtime Library Exception along with this program;
> > +see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
> > +<http://www.gnu.org/licenses/>. */
> > +
> > +#include "tsystem.h"
> > +#include "auto-target.h"
> > +
> > +#ifdef ENABLE_VALGRIND_INTEROP
> > +
> > +#include <valgrind/memcheck.h>
> > +
> > +/* Externally callable wrapper for Valgrind Memcheck client request:
> > + declare SIZE bytes of memory at address ADDR as uninitialized. */
> > +
> > +void
> > +__gcc_vgmc_make_mem_undefined (void *addr, size_t size)
> > +{
> > + VALGRIND_MAKE_MEM_UNDEFINED (addr, size);
> > +}
> > +#endif
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2024-05-28 7:26 ` Richard Biener
@ 2024-05-28 9:46 ` Alexander Monakov
2024-05-28 11:09 ` Richard Biener
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Monakov @ 2024-05-28 9:46 UTC (permalink / raw)
To: Richard Biener
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
[-- Attachment #1: Type: text/plain, Size: 2121 bytes --]
On Tue, 28 May 2024, Richard Biener wrote:
> On Wed, May 15, 2024 at 12:59 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> >
> > Hello,
> >
> > I'd like to ask if anyone has any new thoughts on this patch.
> >
> > Let me also point out that valgrind/memcheck.h is permissively
> > licensed (BSD-style, rest of Valgrind is GPLv2), with the intention
> > to allow importing into projects that are interested in using
> > client requests without build-time dependency on installed headers.
> > So maybe we have that as an option too.
>
> Inlining the VALGRIND_DO_CLIENT_REQUEST_EXPR would be a lot
> cheaper
I am a bit confused what you mean by "cheaper". Could it be that we are not
on the same page regarding the machine code behind client requests?
Here's how the proposed libgcc helper compiles on amd64:
; Prepare the descriptor structure on the stack
movq $1296236545, -56(%rsp)
leaq -56(%rsp), %rax
xorl %edx, %edx
movq %rdi, -48(%rsp)
movq %rsi, -40(%rsp)
movq $0, -32(%rsp)
movq $0, -24(%rsp)
movq $0, -16(%rsp)
#APP
; Request preamble: Valgrind detects the useless
; rotate sequence. Other architectures use different
; preambles.
rolq $3, %rdi ; rolq $13, %rdi
rolq $61, %rdi ; rolq $51, %rdi
; Request trigger (also varies by target).
xchgq %rbx,%rbx
#NO_APP
; The (ignored) result is a volatile automatic variable
movq %rdx, -64(%rsp)
movq -64(%rsp), %rax
I think this is not well-suited for inlining, and to expand it you need to know
the layout of the descriptor struct and machine-specific preamble+trigger.
Also I am not sure where that would leave us with target support. What if
folks will be eventually interested in using this to hunt down mysterious
miscompilations on, say, PowerPC?
> and would not add to the libgcc ABI.
What about linking a new library with that helper?
> I would guess the valgrind "ABI" for these is practically fixed but of course
> architecture dependent.
>
> I do like the feature in general.
Thank you.
Alexander
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2024-05-28 9:46 ` Alexander Monakov
@ 2024-05-28 11:09 ` Richard Biener
2024-05-28 11:38 ` Alexander Monakov
0 siblings, 1 reply; 13+ messages in thread
From: Richard Biener @ 2024-05-28 11:09 UTC (permalink / raw)
To: Alexander Monakov
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Tue, May 28, 2024 at 11:46 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> On Tue, 28 May 2024, Richard Biener wrote:
>
> > On Wed, May 15, 2024 at 12:59 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> > >
> > >
> > > Hello,
> > >
> > > I'd like to ask if anyone has any new thoughts on this patch.
> > >
> > > Let me also point out that valgrind/memcheck.h is permissively
> > > licensed (BSD-style, rest of Valgrind is GPLv2), with the intention
> > > to allow importing into projects that are interested in using
> > > client requests without build-time dependency on installed headers.
> > > So maybe we have that as an option too.
> >
> > Inlining the VALGRIND_DO_CLIENT_REQUEST_EXPR would be a lot
> > cheaper
>
> I am a bit confused what you mean by "cheaper". Could it be that we are not
> on the same page regarding the machine code behind client requests?
Probably "cheaper" in register usage. I also wondered if valgrind is happy
with these when applied to stack space allocated in the caller? Is there
means to verify valgrind picks them up appropriately (as opposed to
simply ignore them)?
> Here's how the proposed libgcc helper compiles on amd64:
>
> ; Prepare the descriptor structure on the stack
> movq $1296236545, -56(%rsp)
> leaq -56(%rsp), %rax
> xorl %edx, %edx
> movq %rdi, -48(%rsp)
> movq %rsi, -40(%rsp)
> movq $0, -32(%rsp)
> movq $0, -24(%rsp)
> movq $0, -16(%rsp)
> #APP
> ; Request preamble: Valgrind detects the useless
> ; rotate sequence. Other architectures use different
> ; preambles.
> rolq $3, %rdi ; rolq $13, %rdi
> rolq $61, %rdi ; rolq $51, %rdi
> ; Request trigger (also varies by target).
> xchgq %rbx,%rbx
> #NO_APP
> ; The (ignored) result is a volatile automatic variable
> movq %rdx, -64(%rsp)
> movq -64(%rsp), %rax
>
> I think this is not well-suited for inlining, and to expand it you need to know
> the layout of the descriptor struct and machine-specific preamble+trigger.
>
> Also I am not sure where that would leave us with target support. What if
> folks will be eventually interested in using this to hunt down mysterious
> miscompilations on, say, PowerPC?
No idea ;) But the same argument applies when libgcc from newer compilers
suddenly change that "ABI" because the valgrind version built against changes?
> > and would not add to the libgcc ABI.
>
> What about linking a new library with that helper?
I guess that would work for me (a static library, that is). Ideally
valgrind itself
would provide it so it's clear its tied to the valgrind version rather than to a
GCC version.
> > I would guess the valgrind "ABI" for these is practically fixed but of course
> > architecture dependent.
> >
> > I do like the feature in general.
>
> Thank you.
> Alexander
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2024-05-28 11:09 ` Richard Biener
@ 2024-05-28 11:38 ` Alexander Monakov
2024-05-28 12:08 ` Richard Biener
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Monakov @ 2024-05-28 11:38 UTC (permalink / raw)
To: Richard Biener
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Tue, 28 May 2024, Richard Biener wrote:
> > I am a bit confused what you mean by "cheaper". Could it be that we are not
> > on the same page regarding the machine code behind client requests?
>
> Probably "cheaper" in register usage.
But it doesn't matter considering that execution under Valgrind is about 40x
slower than native. The intended use is that the project is rebuilt with
this instrumentation, run under Valgrind, then discarded.
Here's an argument against inlining: it makes breakpointing on the helper
possible. And it may be actually necessary.
> I also wondered if valgrind is happy with these when applied to stack space
> allocated in the caller? Is there means to verify valgrind picks them up
> appropriately (as opposed to simply ignore them)?
Yes, it works. Exercising this scenario under gcc.dg does not seem easy, though.
> No idea ;) But the same argument applies when libgcc from newer compilers
> suddenly change that "ABI" because the valgrind version built against changes?
This was raised previously with Jakub. I find it implausible that Valgrind
folks will make incompatible changes to the client request ABI (they know to
keep old requests working when ehnancing the interface).
> > What about linking a new library with that helper?
>
> I guess that would work for me (a static library, that is). Ideally valgrind
> itself would provide it so it's clear its tied to the valgrind version rather
> than to a GCC version.
How about packaging all of this separately as a plugin?
Thanks.
Alexander
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] object lifetime instrumentation for Valgrind [PR66487]
2024-05-28 11:38 ` Alexander Monakov
@ 2024-05-28 12:08 ` Richard Biener
0 siblings, 0 replies; 13+ messages in thread
From: Richard Biener @ 2024-05-28 12:08 UTC (permalink / raw)
To: Alexander Monakov
Cc: gcc-patches, Arsen Arsenović, Sam James, Daniil Frolov
On Tue, May 28, 2024 at 1:38 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> On Tue, 28 May 2024, Richard Biener wrote:
>
> > > I am a bit confused what you mean by "cheaper". Could it be that we are not
> > > on the same page regarding the machine code behind client requests?
> >
> > Probably "cheaper" in register usage.
>
> But it doesn't matter considering that execution under Valgrind is about 40x
> slower than native. The intended use is that the project is rebuilt with
> this instrumentation, run under Valgrind, then discarded.
>
> Here's an argument against inlining: it makes breakpointing on the helper
> possible. And it may be actually necessary.
>
> > I also wondered if valgrind is happy with these when applied to stack space
> > allocated in the caller? Is there means to verify valgrind picks them up
> > appropriately (as opposed to simply ignore them)?
>
> Yes, it works. Exercising this scenario under gcc.dg does not seem easy, though.
>
> > No idea ;) But the same argument applies when libgcc from newer compilers
> > suddenly change that "ABI" because the valgrind version built against changes?
>
> This was raised previously with Jakub. I find it implausible that Valgrind
> folks will make incompatible changes to the client request ABI (they know to
> keep old requests working when ehnancing the interface).
OK, I see.
> > > What about linking a new library with that helper?
> >
> > I guess that would work for me (a static library, that is). Ideally valgrind
> > itself would provide it so it's clear its tied to the valgrind version rather
> > than to a GCC version.
>
> How about packaging all of this separately as a plugin?
Well, sure - but of course I think our plugin API is broken and I rather have
such feature in-tree. It possibly makes sense for _valgrind_ to host such
a plugin, not so much for GCC itself (because then, just build it in-tree).
As said, I'm nervous about libgcc, everything else is OK I think (didn't look
into the pass in detail yet, but I trust you here).
Richard.
> Thanks.
> Alexander
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-05-28 12:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-08 13:49 [PATCH 0/1] Detecting lifetime-dse issues via Valgrind [PR66487] Alexander Monakov
2023-12-08 13:49 ` [PATCH 1/1] object lifetime instrumentation for " Alexander Monakov
2023-12-08 14:04 ` [PATCH 0/1] Detecting lifetime-dse issues via " Jakub Jelinek
2023-12-08 15:43 ` Alexander Monakov
2023-12-08 16:04 ` Jakub Jelinek
2023-12-08 16:29 ` Alexander Monakov
2023-12-22 14:10 ` [PATCH v2] object lifetime instrumentation for " Alexander Monakov
2024-05-15 10:59 ` Alexander Monakov
2024-05-28 7:26 ` Richard Biener
2024-05-28 9:46 ` Alexander Monakov
2024-05-28 11:09 ` Richard Biener
2024-05-28 11:38 ` Alexander Monakov
2024-05-28 12:08 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).