Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
> From: > Date: Tue, 24 Oct 2023 17:11:24 +0300 > From: Daniil Frolov > > 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. A long time ago, I hacked the first version of valgrind-testing support into gcc and I think yours is a great idea! A natural follow-up then would be making -fvalgrind-emit-annotations the default when building gcc *and* --enable-checking=valgrind is in effect (i.e. not just expanding the in-source annotations but the explicit testing mode). No need to cram it into the first version though. Also, a preprocessor macro when -fvalgrind-emit-annotations is in effect, may help dealing with quirky corner cases. I agree with Arsen that if no valgrind headers are found, just don't build __valgrind_make_mem_undefined and let its absense be a linker error rather than trapping in __valgrind_make_mem_undefined. I think building libgcc __valgrind_make_mem_undefined should actually be the *default* when valgrind headers are found, i.e. in absense of --disable-valgrind-annotations. A hard configure-time error is suitable when an --enable-valgrind-annotations is explicitly specified in the absence of valgrind headers. BTW, if you use AS_IF, beware of its quirks. See end of this email-thread with conclusion when libstdc++ was hit: https://gcc.gnu.org/pipermail/libstdc++/2023-June/056113.html brgds, H-P
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Tue, Nov 21, 2023 at 9:56 AM Alexander Monakov wrote: > > > On Tue, 21 Nov 2023, Richard Biener wrote: > > > and this, too, btw. - the DSE actually happens, the latter transform not. > > We specifically "opt out" of doing that for QOI to not make undefined > > behavior worse. The more correct transform would be to replace the > > load with a __builtin_trap () during path isolation (or wire in path > > isolation > > to value-numbering where we actually figure out there's no valid definition > > to reach for the load). > > > > So yes, if you want to avoid these kind of transforms earlier > > instrumentation > > is better. > > And then attempting to schedule it immediately after pass_ccp in the > early-opts > pipeline is already too late, right? Well, yes. CCP won't do many things but it might for example rewrite a stack variable to a register. > Thanks! > Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Tue, 21 Nov 2023, Richard Biener wrote: > and this, too, btw. - the DSE actually happens, the latter transform not. > We specifically "opt out" of doing that for QOI to not make undefined > behavior worse. The more correct transform would be to replace the > load with a __builtin_trap () during path isolation (or wire in path isolation > to value-numbering where we actually figure out there's no valid definition > to reach for the load). > > So yes, if you want to avoid these kind of transforms earlier instrumentation > is better. And then attempting to schedule it immediately after pass_ccp in the early-opts pipeline is already too late, right? Thanks! Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Tue, Nov 21, 2023 at 8:59 AM Alexander Monakov wrote: > > > On Tue, 21 Nov 2023, Alexander Monakov wrote: > > > I am concerned that if GCC ever learns to leave out the following access > > to 'this->foo', leaving tmp uninitialized, we will end up with: > > > > this->foo = 42; > > Sorry, this store will be DSE'd out, of course, but my question stands. I think that would be a reasonable transform, yes. > Alexander > > > *this = { CLOBBER }; > > __valgrind_make_mem_undefined(this, sizeof *this); > > int tmp(D); > > return tmp(D); // uninitialized and this, too, btw. - the DSE actually happens, the latter transform not. We specifically "opt out" of doing that for QOI to not make undefined behavior worse. The more correct transform would be to replace the load with a __builtin_trap () during path isolation (or wire in path isolation to value-numbering where we actually figure out there's no valid definition to reach for the load). So yes, if you want to avoid these kind of transforms earlier instrumentation is better. Richard. > > > > and Valgrind will not report anything since the invalid load is optimized > > out. > > > > With early instrumentation such optimization is not going to happen, since > > the > > builtin may modify *this. > > > > Is my concern reasonable? > > > > Thanks. > > Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Tue, 21 Nov 2023, Alexander Monakov wrote: > I am concerned that if GCC ever learns to leave out the following access > to 'this->foo', leaving tmp uninitialized, we will end up with: > > this->foo = 42; Sorry, this store will be DSE'd out, of course, but my question stands. Alexander > *this = { CLOBBER }; > __valgrind_make_mem_undefined(this, sizeof *this); > int tmp(D); > return tmp(D); // uninitialized > > and Valgrind will not report anything since the invalid load is optimized out. > > With early instrumentation such optimization is not going to happen, since the > builtin may modify *this. > > Is my concern reasonable? > > Thanks. > Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Mon, 13 Nov 2023, Richard Biener wrote: > > Ideally we'd position it such that more locals are put in SSA form, > > but not too late to miss some UB, right? Perhaps after first pass_ccp? > > I guess it’s worth experimenting. Even doing it right before RTL expansion > might work. Note if you pick ccp you have to use a separate place for -O0 While Daniil is experimenting with this, I want to raise my concern about attempting this instrumentation too late. Consider the main thing we are trying to catch: // inlined operator new: this->foo = 42; // inlined constructor: *this = { CLOBBER }; // caller: int tmp = this->foo; return tmp; Our instrumentation adds __valgrind_make_mem_undefined(this, sizeof *this); immediately after the clobber. I am concerned that if GCC ever learns to leave out the following access to 'this->foo', leaving tmp uninitialized, we will end up with: this->foo = 42; *this = { CLOBBER }; __valgrind_make_mem_undefined(this, sizeof *this); int tmp(D); return tmp(D); // uninitialized and Valgrind will not report anything since the invalid load is optimized out. With early instrumentation such optimization is not going to happen, since the builtin may modify *this. Is my concern reasonable? Thanks. Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind [PR66487]
On 2023-11-13 02:53, Sam James wrote: Sam James writes: Alexander Monakov 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
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
> Am 13.11.2023 um 15:52 schrieb Alexander Monakov : > > >> On Mon, 13 Nov 2023, Richard Biener wrote: >> >> Another generic comment - placing a built-in call probably pessimizes code >> generation unless we handle it specially during alias analysis (or in >> builtin_fnspec). > > But considering the resulting code is intended to be run under Valgrind, > isn't a bit worse quality acceptable? Note that we don't want loads > following the built-in to be optimized out, they are necessary as they > will be flagged by Valgrind as attempts to read uninitialized memory. > > I suspect positioning the pass immediately after build_ssa as we do now > is quite imperfect because we will then instrument 'x' in > > void f() > { >int x, *p; >p = > } > > Ideally we'd position it such that more locals are put in SSA form, > but not too late to miss some UB, right? Perhaps after first pass_ccp? I guess it’s worth experimenting. Even doing it right before RTL expansion might work. Note if you pick ccp you have to use a separate place for -O0 >> I also don't like having another pass for this - did you >> investigate to do the instrumentation at the point the CLOBBERs are >> introduced? > > I don't see a better approach, some CLOBBERs are emitted by the C++ > front-end via build_clobber_this, some by omp-expand, some during > gimplification. I'm not a fan of useless IR rescans either, but > this pass is supposed to run very rarely, not by default. > >> Another possibility would be to make this more generic >> and emit the instrumentation when we lower GIMPLE_BIND during >> the GIMPLE lowering pass, you wouldn't then rely on the CLOBBERs >> some of which only appear when -fstack-reuse=none is not used. > > The CLOBBERs that trigger on Firefox and LLVM are emitted not during > gimplification, but via build_clobber_this in the front-end. > > Alexander >
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Mon, 13 Nov 2023, Richard Biener wrote: > Another generic comment - placing a built-in call probably pessimizes code > generation unless we handle it specially during alias analysis (or in > builtin_fnspec). But considering the resulting code is intended to be run under Valgrind, isn't a bit worse quality acceptable? Note that we don't want loads following the built-in to be optimized out, they are necessary as they will be flagged by Valgrind as attempts to read uninitialized memory. I suspect positioning the pass immediately after build_ssa as we do now is quite imperfect because we will then instrument 'x' in void f() { int x, *p; p = } Ideally we'd position it such that more locals are put in SSA form, but not too late to miss some UB, right? Perhaps after first pass_ccp? > I also don't like having another pass for this - did you > investigate to do the instrumentation at the point the CLOBBERs are > introduced? I don't see a better approach, some CLOBBERs are emitted by the C++ front-end via build_clobber_this, some by omp-expand, some during gimplification. I'm not a fan of useless IR rescans either, but this pass is supposed to run very rarely, not by default. > Another possibility would be to make this more generic > and emit the instrumentation when we lower GIMPLE_BIND during > the GIMPLE lowering pass, you wouldn't then rely on the CLOBBERs > some of which only appear when -fstack-reuse=none is not used. The CLOBBERs that trigger on Firefox and LLVM are emitted not during gimplification, but via build_clobber_this in the front-end. Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Tue, Oct 24, 2023 at 4:12 PM wrote: > > From: Daniil Frolov > > 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. Another generic comment - placing a built-in call probably pessimizes code generation unless we handle it specially during alias analysis (or in builtin_fnspec). I also don't like having another pass for this - did you investigate to do the instrumentation at the point the CLOBBERs are introduced? Another possibility would be to make this more generic and emit the instrumentation when we lower GIMPLE_BIND during the GIMPLE lowering pass, you wouldn't then rely on the CLOBBERs some of which only appear when -fstack-reuse=none is not used. Richard. > * 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
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind [PR66487]
Sam James writes: > Alexander Monakov 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
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind [PR66487]
Alexander Monakov 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_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) >> > >> >
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind [PR66487]
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
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
On Sat, 11 Nov 2023, Arsen Arsenović wrote: > > +#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); > > +} > > Would it be preferable to have a link-time error here if missing? Indeed, thank you for the suggestion, will keep that in mind for resending. That will allow to notice the problem earlier, and the user will be able to drop in this snippet in their project to resolve the issue. Alexander
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind [PR66487]
exactl...@ispras.ru writes: > From: Daniil Frolov > > 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
Re: [RFC PATCH] Detecting lifetime-dse issues via Valgrind
Hi, I like the idea of emitting Valgrind annotations. Instrumenting the compiler /a little/ could make a very useful tool even more useful. I have a minor remark, though (as someone not qualified to talk about the middle-end bits of code), in the case that the annotation built-in remains a libgcc bit: exactl...@ispras.ru writes: > +#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); > +} Would it be preferable to have a link-time error here if missing? Have a lovely night. -- Arsen Arsenović signature.asc Description: PGP signature
[RFC PATCH] Detecting lifetime-dse issues via Valgrind
From: Daniil Frolov 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 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 000..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