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
