[RFC] Detecting lifetime-dse issues via Valgrind

  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 in Firefox, LLVM,
OpenJade.

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, we need to wrap it in a built-in.  We know that implementing
such a built-in in libgcc is undesirable, ideally contents of libgcc should not
depend on availability of external headers.  Suggestion for cleaner solutions
would be welcome.

gcc/ChangeLog:

	* Makefile.in: Add gimple-valgrind.o.
	* builtins.def (BUILT_IN_VALGRIND_MEM_UNDEF): Add new built-in.
	* common.opt: Add new option.
	* passes.def: Add new pass.
	* tree-pass.h (make_pass_emit_valgrind): New function.
	* gimple-valgrind.cc: New file.

libgcc/ChangeLog:

	* Makefile.in: Add valgrind.o.
	* config.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Add option --enable-valgrind-annotations into libgcc
	config.
	* libgcc2.h (__valgrind_make_mem_undefined): New function.
	* valgrind.c: New file.
---
 gcc/Makefile.in        |   1 +
 gcc/builtins.def       |   1 +
 gcc/common.opt         |   4 ++
 gcc/gimple-valgrind.cc | 124 +++++++++++++++++++++++++++++++++++++++++
 gcc/passes.def         |   1 +
 gcc/tree-pass.h        |   1 +
 libgcc/Makefile.in     |   2 +-
 libgcc/config.in       |   9 +++
 libgcc/configure       |  79 ++++++++++++++++++++++++++
 libgcc/configure.ac    |  48 ++++++++++++++++
 libgcc/libgcc2.h       |   1 +
 libgcc/valgrind.c      |  50 +++++++++++++++++
 12 files changed, 320 insertions(+), 1 deletion(-)
 create mode 100644 gcc/gimple-valgrind.cc
 create mode 100644 libgcc/valgrind.c
  

exactlywb@ispras.ru writes:

> 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 in Firefox, LLVM,
> OpenJade.
>
> 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, we need to wrap it in a built-in.  We know that implementing
> such a built-in in libgcc is undesirable, [...].

Perhaps less objectionable than you think, at least given the new CFR
stuff from oliva from the other week that landed.

> ideally contents of libgcc should not
> depend on availability of external headers.  Suggestion for cleaner solutions
> would be welcome.

This is a really neat idea (it also makes me wonder if there's any other
opportunities for Valgrind integration like this?). It provides a good
alternative to some of the problems from sanitizers too. Sanitizers are
great but they're really not perfect, at least as they stand.

We've "wasted" a lot of time debugging lifetime issues when building
with GCC, especially with LTO. As noted, sanitizers don't help for this
apart from MSan and MSan has its own substantial problems (even if you're on a
source-based distro where you're willing to try rebuild everything).

LLVM was the most recent example but it wasn't the first, and this has
come up with LLVM in a few other places too (same root cause, wasn't
obvious at all).

I can't formally review but the implementation appears straightforward,
just a few comments below.

>
> gcc/ChangeLog:
>
> 	* Makefile.in: Add gimple-valgrind.o.
> 	* builtins.def (BUILT_IN_VALGRIND_MEM_UNDEF): Add new built-in.
> 	* common.opt: Add new option.
> 	* passes.def: Add new pass.
> 	* tree-pass.h (make_pass_emit_valgrind): New function.
> 	* gimple-valgrind.cc: New file.
>
> libgcc/ChangeLog:
>
> 	* Makefile.in: Add valgrind.o.
> 	* config.in: Regenerate.
> 	* configure: Regenerate.
> 	* configure.ac: Add option --enable-valgrind-annotations into libgcc
> 	config.
> 	* libgcc2.h (__valgrind_make_mem_undefined): New function.
> 	* valgrind.c: New file.
> ---
>  gcc/Makefile.in        |   1 +
>  gcc/builtins.def       |   1 +
>  gcc/common.opt         |   4 ++
>  gcc/gimple-valgrind.cc | 124 +++++++++++++++++++++++++++++++++++++++++
>  gcc/passes.def         |   1 +
>  gcc/tree-pass.h        |   1 +
>  libgcc/Makefile.in     |   2 +-
>  libgcc/config.in       |   9 +++
>  libgcc/configure       |  79 ++++++++++++++++++++++++++
>  libgcc/configure.ac    |  48 ++++++++++++++++
>  libgcc/libgcc2.h       |   1 +
>  libgcc/valgrind.c      |  50 +++++++++++++++++
>  12 files changed, 320 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/gimple-valgrind.cc
>  create mode 100644 libgcc/valgrind.c
>
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 9cc16268abf..ded6bdf1673 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1487,6 +1487,7 @@ OBJS = \
>  	gimple-ssa-warn-access.o \
>  	gimple-ssa-warn-alloca.o \
>  	gimple-ssa-warn-restrict.o \
> +	gimple-valgrind.o \
>  	gimple-streamer-in.o \
>  	gimple-streamer-out.o \
>  	gimple-walk.o \
> diff --git a/gcc/builtins.def b/gcc/builtins.def
> index 5953266acba..42d34189f1e 100644
> --- a/gcc/builtins.def
> +++ b/gcc/builtins.def
> @@ -1064,6 +1064,7 @@ DEF_GCC_BUILTIN        (BUILT_IN_VA_END, "va_end", BT_FN_VOID_VALIST_REF, ATTR_N
>  DEF_GCC_BUILTIN        (BUILT_IN_VA_START, "va_start", BT_FN_VOID_VALIST_REF_VAR, ATTR_NOTHROW_LEAF_LIST)
>  DEF_GCC_BUILTIN        (BUILT_IN_VA_ARG_PACK, "va_arg_pack", BT_FN_INT, ATTR_PURE_NOTHROW_LEAF_LIST)
>  DEF_GCC_BUILTIN        (BUILT_IN_VA_ARG_PACK_LEN, "va_arg_pack_len", BT_FN_INT, ATTR_PURE_NOTHROW_LEAF_LIST)
> +DEF_EXT_LIB_BUILTIN    (BUILT_IN_VALGRIND_MEM_UNDEF, "__valgrind_make_mem_undefined", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
>  DEF_EXT_LIB_BUILTIN    (BUILT_IN__EXIT, "_exit", BT_FN_VOID_INT, ATTR_NORETURN_NOTHROW_LEAF_LIST)
>  DEF_C99_BUILTIN        (BUILT_IN__EXIT2, "_Exit", BT_FN_VOID_INT, ATTR_NORETURN_NOTHROW_LEAF_LIST)
>  
> diff --git a/gcc/common.opt b/gcc/common.opt
> index f137a1f81ac..c9040386956 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2515,6 +2515,10 @@ starts and when the destructor finishes.
>  flifetime-dse=
>  Common Joined RejectNegative UInteger Var(flag_lifetime_dse) Optimization IntegerRange(0, 2)
>  
> +fvalgrind-emit-annotations
> +Common Var(flag_valgrind_annotations,1)
> +Emit Valgrind annotations with respect to object's lifetime.
> +
>  flive-patching
>  Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
>  
> diff --git a/gcc/gimple-valgrind.cc b/gcc/gimple-valgrind.cc
> new file mode 100644
> index 00000000000..8075e6404d4
> --- /dev/null
> +++ b/gcc/gimple-valgrind.cc
> @@ -0,0 +1,124 @@
> +/* Emit 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.
> +
> +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 "backend.h"
> +#include "tree.h"
> +#include "gimple.h"
> +#include "gimple-iterator.h"
> +#include "tree-pass.h"
> +#include "gimplify-me.h"
> +#include "fold-const.h"
> +
> +namespace {
> +
> +/* Interface for emitting a call for __valgrind_make_mem_undefined.  This
> +   function returns true if the call was successfully emitted and false
> +   otherwise.  */
> +
> +bool
> +emit_builtin_memory_undefined (gimple_stmt_iterator *gsi, gimple *stmt)
> +{
> +  tree var_ptr, call;
> +  tree lhs = gimple_assign_lhs (stmt);
> +  tree sizet = arg_size_in_bytes (TREE_TYPE (lhs));
> +  if (sizet == size_zero_node)
> +    return false;
> +
> +  var_ptr = build_fold_addr_expr (lhs);
> +  call = build_call_expr (builtin_decl_explicit (BUILT_IN_VALGRIND_MEM_UNDEF),
> +			  2, var_ptr, sizet);
> +  force_gimple_operand_gsi (gsi, call, false, NULL_TREE, false, GSI_NEW_STMT);
> +
> +  return true;
> +}
> +
> +const pass_data pass_data_emit_valgrind = {
> +  GIMPLE_PASS, /* type */
> +  "emit_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_emit_valgrind : public gimple_opt_pass
> +{
> +public:
> +  pass_emit_valgrind (gcc::context *ctxt)
> +    : gimple_opt_pass (pass_data_emit_valgrind, ctxt)
> +  {
> +  }
> +
> +  unsigned int execute (function *) final override;
> +  bool gate (function *) final override;
> +};
> +
> +bool
> +pass_emit_valgrind::gate (function *)
> +{
> +  return flag_valgrind_annotations;
> +}
> +
> +/* Valgrind does not know about lifetime violations because of it is working in
> +   machine code level.  There are special Valgrind client requests like
> +   VALGRIND_MAKE_MEM_UNDEFINED, VALGRIND_MAKE_MEM_NOACCESS etc. to mark memory
> +   especially.
> +
> +   This pass emits such annotations automatically.  */
> +
> +unsigned int
> +pass_emit_valgrind::execute (function *fun)
> +{
> +  bool todo = 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);
> +	  /* GCC emits special clobber gimple instructions with respect to
> +	     lifetime of memory like:
> +
> +	       x ={v} {CLOBBER};
> +
> +	     Insert __builtin_valgrind_make_mem_undefined after such statement
> +	     to trigger lifetime violations in Valgrind.  */
> +	  if (gimple_clobber_p (stmt))
> +	    todo |= emit_builtin_memory_undefined (&gsi, stmt);
> +	}
> +    }
> +
> +  return todo ? TODO_update_ssa : 0;
> +}
> +
> +gimple_opt_pass *
> +make_pass_emit_valgrind (gcc::context *ctxt)
> +{
> +  return new pass_emit_valgrind (ctxt);
> +}
> +
> diff --git a/gcc/passes.def b/gcc/passes.def
> index 4110a472914..c7fb47db864 100644
> --- a/gcc/passes.def
> +++ b/gcc/passes.def
> @@ -56,6 +56,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_emit_valgrind);
>        NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
>        NEXT_PASS (pass_warn_printf);
>        NEXT_PASS (pass_warn_nonnull_compare);
> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> index eba2d54ac76..b8797b465e3 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_emit_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 7ee8b5f9bcb..56cb8a9d903 100644
> --- a/libgcc/Makefile.in
> +++ b/libgcc/Makefile.in
> @@ -428,7 +428,7 @@ ifneq ($(enable_shared),yes)
>  iterator = $(patsubst %,$(srcdir)/static-object.mk,$(iter-items))
>  endif
>  
> -LIB2ADD += enable-execute-stack.c
> +LIB2ADD += enable-execute-stack.c $(srcdir)/valgrind.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
> diff --git a/libgcc/config.in b/libgcc/config.in
> index f93c64a00c3..278d6e94817 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
>  
> +/* Define to get calls to the valgrind runtime enabled. */
> +#undef ENABLE_VALGRIND_ANNOTATIONS
> +
>  /* Define to 1 if the assembler supports AVX. */
>  #undef HAVE_AS_AVX
>  
> @@ -28,6 +31,9 @@
>  /* Define to 1 if you have the <inttypes.h> header file. */
>  #undef HAVE_INTTYPES_H
>  
> +/* Define if valgrind's memcheck.h header is installed. */
> +#undef HAVE_MEMCHECK_H
> +
>  /* Define to 1 if you have the <memory.h> header file. */
>  #undef HAVE_MEMORY_H
>  
> @@ -61,6 +67,9 @@
>  /* Define to 1 if you have the <unistd.h> header file. */
>  #undef HAVE_UNISTD_H
>  
> +/* Define if valgrind's valgrind/memcheck.h header is installed. */
> +#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 cf149209652..98f5bffb293 100755
> --- a/libgcc/configure
> +++ b/libgcc/configure
> @@ -713,6 +713,7 @@ enable_largefile
>  enable_decimal_float
>  with_system_libunwind
>  enable_cet
> +enable_valgrind_annotations
>  enable_explicit_exception_frame_registration
>  enable_tm_clone_registry
>  with_glibc_version
> @@ -1354,6 +1355,8 @@ Optional Features:
>  			or 'dpd' choses which decimal floating point format
>  			to use
>    --enable-cet            enable Intel CET in target libraries [default=auto]
> +  --enable-valgrind-annotations
> +                          enable valgrind runtime interaction
>    --enable-explicit-exception-frame-registration
>                            register exception tables explicitly at module
>                            start, for use e.g. for compatibility with
> @@ -4987,6 +4990,82 @@ fi
>  
>  
>  
> +ac_fn_c_check_header_preproc "$LINENO" "valgrind.h" "ac_cv_header_valgrind_h"
> +if test "x$ac_cv_header_valgrind_h" = xyes; then :
> +  have_valgrind_h=yes
> +else
> +  have_valgrind_h=no
> +fi
> +
> +
> +# It is certainly possible that there's valgrind but no valgrind.h.
> +# GCC relies on making annotations so we must have both.
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for VALGRIND_DISCARD in <valgrind/memcheck.h>" >&5
> +$as_echo_n "checking for VALGRIND_DISCARD in <valgrind/memcheck.h>... " >&6; }
> +cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +#include <valgrind/memcheck.h>
> +#ifndef VALGRIND_DISCARD
> +#error VALGRIND_DISCARD not defined
> +#endif
> +_ACEOF
> +if ac_fn_c_try_cpp "$LINENO"; then :
> +  gcc_cv_header_valgrind_memcheck_h=yes
> +else
> +  gcc_cv_header_valgrind_memcheck_h=no
> +fi
> +rm -f conftest.err conftest.i conftest.$ac_ext
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_header_valgrind_memcheck_h" >&5
> +$as_echo "$gcc_cv_header_valgrind_memcheck_h" >&6; }
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for VALGRIND_DISCARD in <memcheck.h>" >&5
> +$as_echo_n "checking for VALGRIND_DISCARD in <memcheck.h>... " >&6; }
> +cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +#include <memcheck.h>
> +#ifndef VALGRIND_DISCARD
> +#error VALGRIND_DISCARD not defined
> +#endif
> +_ACEOF
> +if ac_fn_c_try_cpp "$LINENO"; then :
> +  gcc_cv_header_memcheck_h=yes
> +else
> +  gcc_cv_header_memcheck_h=no
> +fi
> +rm -f conftest.err conftest.i conftest.$ac_ext
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_header_memcheck_h" >&5
> +$as_echo "$gcc_cv_header_memcheck_h" >&6; }
> +if test $gcc_cv_header_valgrind_memcheck_h = yes; then
> +
> +$as_echo "#define HAVE_VALGRIND_MEMCHECK_H 1" >>confdefs.h
> +
> +fi
> +if test $gcc_cv_header_memcheck_h = yes; then
> +
> +$as_echo "#define HAVE_MEMCHECK_H 1" >>confdefs.h
> +
> +fi
> +
> +# Check whether --enable-valgrind-annotations was given.
> +if test "${enable_valgrind_annotations+set}" = set; then :
> +  enableval=$enable_valgrind_annotations;
> +else
> +  enable_valgrind_annotations=no
> +fi
> +
> +if test x$enable_valgrind_annotations != xno \
> +    || test x$ac_valgrind_checking != x; then
> +  if (test $have_valgrind_h = no \
> +      && test $gcc_cv_header_memcheck_h = no \
> +      && test $gcc_cv_header_valgrind_memcheck_h = no); then
> +    as_fn_error $? "*** valgrind annotations requested, but" "$LINENO" 5
> +    as_fn_error $? "*** Can't find valgrind/memcheck.h, memcheck.h or valgrind.h" "$LINENO" 5
> +  fi
> +
> +$as_echo "#define ENABLE_VALGRIND_ANNOTATIONS 1" >>confdefs.h
> +
> +fi
> +
> +
>  # Check whether --enable-explicit-exception-frame-registration was given.
>  if test "${enable_explicit_exception_frame_registration+set}" = set; then :
>    enableval=$enable_explicit_exception_frame_registration;
> diff --git a/libgcc/configure.ac b/libgcc/configure.ac
> index 2fc9d5d7c93..6054581f41f 100644
> --- a/libgcc/configure.ac
> +++ b/libgcc/configure.ac
> @@ -269,6 +269,54 @@ GCC_CHECK_SJLJ_EXCEPTIONS
>  GCC_CET_FLAGS(CET_FLAGS)
>  AC_SUBST(CET_FLAGS)
>  
> +AC_CHECK_HEADER(valgrind.h, have_valgrind_h=yes, have_valgrind_h=no)

Consider using PKG_CHECK_MODULES and falling back to a manual search.

> +
> +# It is certainly possible that there's valgrind but no valgrind.h.
> +# GCC relies on making annotations so we must have both.
> +AC_MSG_CHECKING(for VALGRIND_DISCARD in <valgrind/memcheck.h>)
> +AC_PREPROC_IFELSE([AC_LANG_SOURCE(
> +  [[#include <valgrind/memcheck.h>
> +#ifndef VALGRIND_DISCARD
> +#error VALGRIND_DISCARD not defined
> +#endif]])],
> +  [gcc_cv_header_valgrind_memcheck_h=yes],
> +  [gcc_cv_header_valgrind_memcheck_h=no])
> +AC_MSG_RESULT($gcc_cv_header_valgrind_memcheck_h)
> +AC_MSG_CHECKING(for VALGRIND_DISCARD in <memcheck.h>)
> +AC_PREPROC_IFELSE([AC_LANG_SOURCE(
> +  [[#include <memcheck.h>
> +#ifndef VALGRIND_DISCARD
> +#error VALGRIND_DISCARD not defined
> +#endif]])],
> +  [gcc_cv_header_memcheck_h=yes],
> +  [gcc_cv_header_memcheck_h=no])
> +AC_MSG_RESULT($gcc_cv_header_memcheck_h)
> +if test $gcc_cv_header_valgrind_memcheck_h = yes; then

Consider AS_IF, as more recent autoconf now recommends - see
https://git.savannah.gnu.org/cgit/autoconf.git/commit/?id=beb6d826338fb854b5c73458a1d52662b04c171c.

> +  AC_DEFINE(HAVE_VALGRIND_MEMCHECK_H, 1,
> +	[Define if valgrind's valgrind/memcheck.h header is installed.])
> +fi
> +if test $gcc_cv_header_memcheck_h = yes; then
> +  AC_DEFINE(HAVE_MEMCHECK_H, 1,
> +	[Define if valgrind's memcheck.h header is installed.])
> +fi
> +
> +AC_ARG_ENABLE(valgrind-annotations,
> +[AS_HELP_STRING([--enable-valgrind-annotations],
> +		[enable valgrind runtime interaction])], [],
> +[enable_valgrind_annotations=no])
> +if test x$enable_valgrind_annotations != xno \
> +    || test x$ac_valgrind_checking != x; then
> +  if (test $have_valgrind_h = no \
> +      && test $gcc_cv_header_memcheck_h = no \
> +      && test $gcc_cv_header_valgrind_memcheck_h = no); then
> +    AC_MSG_ERROR([*** valgrind annotations requested, but])
> +    AC_MSG_ERROR([*** Can't find valgrind/memcheck.h, memcheck.h or valgrind.h])

Nit: ", or"

> +  fi
> +  AC_DEFINE(ENABLE_VALGRIND_ANNOTATIONS, 1,
> +[Define to get calls to the valgrind runtime enabled.])
> +fi
> +
> +
>  AC_ARG_ENABLE([explicit-exception-frame-registration],
>    [AC_HELP_STRING([--enable-explicit-exception-frame-registration],
>       [register exception tables explicitly at module start, for use
> diff --git a/libgcc/libgcc2.h b/libgcc/libgcc2.h
> index 06e26cad884..af37ae7d816 100644
> --- a/libgcc/libgcc2.h
> +++ b/libgcc/libgcc2.h
> @@ -31,6 +31,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  
>  extern int __gcc_bcmp (const unsigned char *, const unsigned char *, size_t);
>  extern void __clear_cache (void *, void *);
> +extern void __valgrind_make_mem_undefined (void *ptr, unsigned long sz);
>  extern void __eprintf (const char *, const char *, unsigned int, const char *)
>    __attribute__ ((__noreturn__));
>  
> diff --git a/libgcc/valgrind.c b/libgcc/valgrind.c
> new file mode 100644
> index 00000000000..a64e22d0cb3
> --- /dev/null
> +++ b/libgcc/valgrind.c
> @@ -0,0 +1,50 @@
> +/* Implement interface 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 "tconfig.h"
> +#include "tsystem.h"
> +#include "auto-target.h"
> +
> +#ifdef ENABLE_VALGRIND_ANNOTATIONS
> +# ifdef HAVE_VALGRIND_MEMCHECK_H
> +#  include <valgrind/memcheck.h>
> +# elif defined HAVE_MEMCHECK_H
> +#  include <memcheck.h>
> +# else
> +#  include <valgrind.h>
> +# endif
> +/* Compatibility macros to let valgrind 3.1 work.  */
> +# ifndef VALGRIND_MAKE_MEM_UNDEFINED
> +#  define VALGRIND_MAKE_MEM_UNDEFINED VALGRIND_MAKE_WRITABLE
> +# endif
> +/* Abort if ENABLE_VALGRIND_ANNOTATIONS is not defined.  */
> +#else
> +# define VALGRIND_MAKE_MEM_UNDEFINED(ptr, sz) __builtin_trap ()
> +#endif
> +
> +void __valgrind_make_mem_undefined (void *ptr, unsigned long sz)
> +{
> +  VALGRIND_MAKE_MEM_UNDEFINED (ptr, sz);
> +}
> +
  

On Sat, 11 Nov 2023, Sam James wrote:

> > Valgrind client requests are offered as macros that emit inline asm.  For use
> > in code generation, we need to wrap it in a built-in.  We know that implementing
> > such a built-in in libgcc is undesirable, [...].
> 
> Perhaps less objectionable than you think, at least given the new CFR
> stuff from oliva from the other week that landed.

Yeah; we haven't found any better solution anyway.

> This is a really neat idea (it also makes me wonder if there's any other
> opportunities for Valgrind integration like this?).

To (attempt to) answer the parenthetical question, note that the patch is not
limited to instrumenting C++ cdtors, it annotates all lifetime CLOBBER marks,
so Valgrind should see lifetime boundaries of various on-stack arrays too.

(I hope positioning the new pass after build_ssa is sufficient to avoid
annotating too much, like non-address-taken local scalars)

> LLVM was the most recent example but it wasn't the first, and this has
> come up with LLVM in a few other places too (same root cause, wasn't
> obvious at all).

I'm very curious what you mean by "this has come up with LLVM [] too": ttbomk,
LLVM doesn't do such lifetime-based optimization yet, which is why compiling
LLVM with LLVM doesn't break it. Can you share some examples? Or do you mean
instances when libLLVM-miscompiled-with-GCC was linked elsewhere, and that
program crashed mysteriously as a result?

Indeed this work is inspired by the LLVM incident in PR 106943. Unforunately
we don't see many other instances with -flifetime-dse workarounds in public.
Grepping Gentoo Portage reveals only openjade. Arch applies the workaround to
a jvm package too, and we know that Firefox and LLVM apply it on their own.

This patch finds the issue in LLVM and openjade; testing it on Spidermonkey
is TODO. Suggestions for other interesting tests would be welcome.

> > --- a/libgcc/configure.ac
> > +++ b/libgcc/configure.ac
> > @@ -269,6 +269,54 @@ GCC_CHECK_SJLJ_EXCEPTIONS
> >  GCC_CET_FLAGS(CET_FLAGS)
> >  AC_SUBST(CET_FLAGS)
> >  
> > +AC_CHECK_HEADER(valgrind.h, have_valgrind_h=yes, have_valgrind_h=no)
> 
> Consider using PKG_CHECK_MODULES and falling back to a manual search.

Thanks. autotools bits in this patch are one-to-one copy of the pre-existing
Valgrind detection in the 'gcc' subdirectory where it's necessary for
running the compiler under Valgrind without false positives.

I guess the right solution is to move Valgrind detection into the top-level
'config' directory (and apply the cleanups you mention), but as we are not
familiar with autotools we just made the copy-paste for this RFC.

With the patch, --enable-valgrind-annotations becomes "overloaded" to
simultaneously instrument the compiler and enhance libgcc to support
-fvalgrind-emit-annotations, but those are independent and in practice
people may need the latter without the former.

Alexander
  

Alexander Monakov <amonakov@ispras.ru> writes:

> On Sat, 11 Nov 2023, Sam James wrote:
>
>> > Valgrind client requests are offered as macros that emit inline asm.  For use
>> > in code generation, we need to wrap it in a built-in.  We know that implementing
>> > such a built-in in libgcc is undesirable, [...].
>> 
>> Perhaps less objectionable than you think, at least given the new CFR
>> stuff from oliva from the other week that landed.
>
> Yeah; we haven't found any better solution anyway.
>
>> This is a really neat idea (it also makes me wonder if there's any other
>> opportunities for Valgrind integration like this?).
>
> To (attempt to) answer the parenthetical question, note that the patch is not
> limited to instrumenting C++ cdtors, it annotates all lifetime CLOBBER marks,
> so Valgrind should see lifetime boundaries of various on-stack arrays too.

Oh, right!

>
> (I hope positioning the new pass after build_ssa is sufficient to avoid
> annotating too much, like non-address-taken local scalars)
>
>> LLVM was the most recent example but it wasn't the first, and this has
>> come up with LLVM in a few other places too (same root cause, wasn't
>> obvious at all).
>
> I'm very curious what you mean by "this has come up with LLVM [] too": ttbomk,
> LLVM doesn't do such lifetime-based optimization yet, which is why compiling
> LLVM with LLVM doesn't break it. Can you share some examples? Or do you mean
> instances when libLLVM-miscompiled-with-GCC was linked elsewhere, and that
> program crashed mysteriously as a result?
>
> Indeed this work is inspired by the LLVM incident in PR 106943.

For that part, I meant that we had a _lot_ of different variations of
the LLVM miscompilation over the years where people would report issues
when building LLVM with GCC but trying to investigate it didn't get very
far.

i.e. It was very hard to debug and something like this would've made
things substantially easier (it takes us from the realm of "uhh maybe
compiler bug" to provable UB, which we're very used to handling).

It doesn't help that the fact that ubsan and friends can't find this
means it's often not-determined and may be worked around with either
disabling LTO or disabling various passes as a heavy hammer.

I didn't think to try toggling it when I hit issues until PR 106943.

> we don't see many other instances with -flifetime-dse workarounds in public.
> Grepping Gentoo Portage reveals only openjade. Arch applies the workaround to
> a jvm package too, and we know that Firefox and LLVM apply it on their own.
>

I had some vague memories in the back of my head so I went digging
because I enjoy this:

* Qt has hit this in the past, and I actually wonder if it's the cause
of a few other nebulous LTO issues (which we've never had a solid report
for) as well:
https://codereview.qt-project.org/c/qt/qtdeclarative/+/176272
https://bugs.gentoo.org/584818
https://bugs.gentoo.org/626070

We've given up for Qt 5 and I plan on revisiting any possible issues
with LTO with Qt 6 instead.

* Firefox as you noted: https://bugzilla.mozilla.org/show_bug.cgi?id=1232696.
* TBB (no real details available, unfortunately): https://github.com/oneapi-src/oneTBB/commit/51c0b2f742920535178560f31c6e91065bf87b41
* crypto++ has had a series of mysterious miscompilations and I suspect
  it may be a victim here. See https://github.com/weidai11/cryptopp/issues/1141#issuecomment-1208169530
  onwards but it's not the first bug like this crypto++ has had.
* codeblocks: https://bugs.gentoo.org/625696
* coin: https://bugs.gentoo.org/619378

(I would not be surprised if other wxwidgets applications are victims,
e.g. older kicad or audacity.)

Some of the results for GCC 6 at
https://bugs.gentoo.org/showdependencytree.cgi?id=582084&hide_resolved=0
were from other optimisation changes, but some are DSE (and some might
have been DSE but masked by another flag as I mentioned earlier).

If someone is extremely bored, they may want to look through our LTO tracker /
various -fno-lto/filter-lto grep results in gentoo.git to see if any
of them seem fishy. In the past, people would filter LTO without
examining the real problem. I am trying to fix this but you can't do that overnight.

Another good heuristic, I bet, is anything passing -O1, filtering -O3,
or if you want a general sense of crustiness, where the ebuild has to pass
-std=c++98 or so. But I'm just trying to give you fodder if you want
more examples with that last suggestion.

With less detail, I have mentions for the following in some git checkouts
from other distributions. I could not find any bug references:
* injeqt (Void Linux)
* opencollada (Void Linux)

> This patch finds the issue in LLVM and openjade; testing it on Spidermonkey
> is TODO. Suggestions for other interesting tests would be welcome.
>
>> > --- a/libgcc/configure.ac
>> > +++ b/libgcc/configure.ac
>> > @@ -269,6 +269,54 @@ GCC_CHECK_SJLJ_EXCEPTIONS
>> >  GCC_CET_FLAGS(CET_FLAGS)
>> >  AC_SUBST(CET_FLAGS)
>> >  
>> > +AC_CHECK_HEADER(valgrind.h, have_valgrind_h=yes, have_valgrind_h=no)
>> 
>> Consider using PKG_CHECK_MODULES and falling back to a manual search.
>
> Thanks. autotools bits in this patch are one-to-one copy of the pre-existing
> Valgrind detection in the 'gcc' subdirectory where it's necessary for
> running the compiler under Valgrind without false positives.
>
> I guess the right solution is to move Valgrind detection into the top-level
> 'config' directory (and apply the cleanups you mention), but as we are not
> familiar with autotools we just made the copy-paste for this RFC.

ack

>
> With the patch, --enable-valgrind-annotations becomes "overloaded" to
> simultaneously instrument the compiler and enhance libgcc to support
> -fvalgrind-emit-annotations, but those are independent and in practice
> people may need the latter without the former.

Yeah, I did notice this and wasn't sure how I felt about it, but I
didn't want to bikeshed at this point. I don't think it matters that
much given you need to build with a flag for them to be emitted anyway.

>
> Alexander

thanks,
sam
  

Sam James <sam@gentoo.org> writes:

> Alexander Monakov <amonakov@ispras.ru> writes:
> [...]
>>
>> I'm very curious what you mean by "this has come up with LLVM [] too": ttbomk,
>> LLVM doesn't do such lifetime-based optimization yet, which is why compiling
>> LLVM with LLVM doesn't break it. Can you share some examples? Or do you mean
>> instances when libLLVM-miscompiled-with-GCC was linked elsewhere, and that
>> program crashed mysteriously as a result?
>>
>> Indeed this work is inspired by the LLVM incident in PR 106943.
>
> [...]
> I had some vague memories in the back of my head so I went digging
> because I enjoy this:
> [...]

I ended up stumbling on two more:

* charm (https://github.com/UIUC-PPL/charm/issues/1045)
* firebird (https://github.com/FirebirdSQL/firebird/issues/5384, starring richi)

Now I'm really done :)

> [...]
>>
>> Alexander
>
> thanks,
> sam
  

On 2023-11-13 02:53, Sam James wrote:
> Sam James <sam@gentoo.org> writes:
> 
>> Alexander Monakov <amonakov@ispras.ru> writes:
>> [...]
>>> 
>>> I'm very curious what you mean by "this has come up with LLVM [] 
>>> too": ttbomk,
>>> LLVM doesn't do such lifetime-based optimization yet, which is why 
>>> compiling
>>> LLVM with LLVM doesn't break it. Can you share some examples? Or do 
>>> you mean
>>> instances when libLLVM-miscompiled-with-GCC was linked elsewhere, and 
>>> that
>>> program crashed mysteriously as a result?
>>> 
>>> Indeed this work is inspired by the LLVM incident in PR 106943.
>> 
>> [...]
>> I had some vague memories in the back of my head so I went digging
>> because I enjoy this:
>> [...]
> 
> I ended up stumbling on two more:
> 
> * charm (https://github.com/UIUC-PPL/charm/issues/1045)
> * firebird (https://github.com/FirebirdSQL/firebird/issues/5384, 
> starring richi)
> 
> Now I'm really done :)
> 
>> [...]
>>> 
>>> Alexander
>> 
>> thanks,
>> sam

Thanks for your prompt response; it is greatly appreciated.

We conducted tests on two packages from your provided list and obtained
some preliminary results:

* crypto++: No violations were detected during their own tests, which
were executed using 'make valgrind' and our custom option
--fvalgrind-emit-annotations.

* firebird: An issue was identified with the global object isqlGlobal.  
It
appears that developers are assuming the global object will be
zero-initialized, but the C++ standard guarantees this only for static
initialization.  The presence of a non-trivial constructor, 
IsqlGlobals(),
means that isqlGlobal has formally uninitialized fields.  A snippet from
the Valgrind dump is as follows:

==106087== Conditional jump or move depends on uninitialised value(s)
==106087==    at 0x4378F0: create_db(char const*, char*) (isql.cpp:5838)
==106087==    by 0x44CBE3: frontend(char const*) (isql.cpp:6699)
==106087==    by 0x44EBC1: get_statement (isql.cpp:7638)
==106087==    by 0x44EBC1: do_isql() (isql.cpp:6008)
==106087==    by 0x45039C: ISQL_main(int, char**) (isql.cpp:1854)
==106087==    by 0x4EE1082: (below main) (libc-start.c:308)
==106087==  Uninitialised value was created by a client request
==106087==    at 0x5A617C: __valgrind_make_mem_undefined (valgrind.c:48)
==106087==    by 0x42E365: IsqlGlobals::IsqlGlobals() (isql.cpp:1378)
==106087==    by 0x418388: __static_initialization_and_destruction_0 
(isql.cpp:1098)
==106087==    by 0x418388: _GLOBAL__sub_I_isql.cpp (isql.cpp:11536)
==106087==    by 0x5A61DC: __libc_csu_init (in ...)
==106087==    by 0x4EE100F: (below main) (libc-start.c:264)

---
With best regards,
Daniil