[0/1] Detecting lifetime-dse issues via Valgrind [PR66487]

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

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

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

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

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