| Message ID | 20161026150218.3F1A4439942E0@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 26 Oct 2016, Florian Weimer wrote: > The functionality in <mcheck.h> will eventually be replaced with > no-op functions (and a separate, preloadable DSO). If you declare functionality deprecated then you need to update the manual accordingly. But I don't think the right approach is to declare deprecated on an "eventually be replaced" basis. Rather, the deprecation and the new DSO should be in the same patch series, going in the same glibc version, so the NEWS file and the main documentation can tell users of the mtrace script to preload the DSO and remove calls to the mtrace function (if that's the intended way to update code using mtrace and the one to be used for all the glibc tests that make use of it). (And that's general for any deprecation: make clear how people should update their code and ensure the replacement is actually ready first, don't just say something is deprecated in NEWS without the replacement being obvious.)
On 10/26/2016 05:55 PM, Joseph Myers wrote: > On Wed, 26 Oct 2016, Florian Weimer wrote: > >> The functionality in <mcheck.h> will eventually be replaced with >> no-op functions (and a separate, preloadable DSO). > > If you declare functionality deprecated then you need to update the manual > accordingly. But I don't think the right approach is to declare > deprecated on an "eventually be replaced" basis. Rather, the deprecation > and the new DSO should be in the same patch series, going in the same > glibc version, so the NEWS file and the main documentation can tell users > of the mtrace script to preload the DSO and remove calls to the mtrace > function (if that's the intended way to update code using mtrace and the > one to be used for all the glibc tests that make use of it). In my opinion, the larger ecosystem already provides suitable replacements. * <mcheck.h> and all malloc hook functions are now deprecated. Future implementations of the mcheck- and mtrace-related functions will not have any effect, and glibc will stop calling the hook functions from its malloc implementation. Instead of mcheck and mtrace, developers should consider using valgrind. As a replacement for the hook functions, developers can interpose their own malloc implementation. Plus removal of the documentation of the hook functions from the manual (if there is anything left). In any case, I'll submit a separate patch for documenting the malloc interposition feature (bug 20424). Thanks, Florian
On Tue, 15 Nov 2016, Florian Weimer wrote: > In my opinion, the larger ecosystem already provides suitable replacements. > > * <mcheck.h> and all malloc hook functions are now deprecated. Future > implementations of the mcheck- and mtrace-related functions will not > have any effect, and glibc will stop calling the hook functions from > its malloc implementation. Instead of mcheck and mtrace, developers > should consider using valgrind. As a replacement for the hook > functions, developers can interpose their own malloc implementation. I don't consider valgrind suitable for replacing the uses of mtrace in the glibc testsuite (or other similar uses elsewhere for lightweight checking for leaks). It's massively more heavyweight, has limited architecture support, as far as I know can only be built with an installed glibc and I don't know if there are other complications building it in a bootstrap environment. Whereas a simple preloadable interposing malloc implementation provided with glibc could support the mtrace functionality as well as providing an example for people writing interposing implementations to use. I don't know enough about the functionality GDB expects from linking with -lmcheck by default in development to know whether other malloc improvements would provide similar checking by default or whether that would also need GDB to provide its own malloc (again, I think valgrind is vastly too heavyweight for using by default in GDB development).
On 11/15/2016 04:39 PM, Joseph Myers wrote: > On Tue, 15 Nov 2016, Florian Weimer wrote: > >> In my opinion, the larger ecosystem already provides suitable replacements. >> >> * <mcheck.h> and all malloc hook functions are now deprecated. Future >> implementations of the mcheck- and mtrace-related functions will not >> have any effect, and glibc will stop calling the hook functions from >> its malloc implementation. Instead of mcheck and mtrace, developers >> should consider using valgrind. As a replacement for the hook >> functions, developers can interpose their own malloc implementation. > > I don't consider valgrind suitable for replacing the uses of mtrace in the > glibc testsuite (or other similar uses elsewhere for lightweight checking > for leaks). I completely agree. The above is for the NEWS file, for external use. For internal use, we still need a solution. (I have an mtrace-compatible interposition-based tracer almost finished, but it may not make the cut for the next release, and it may be superseded by DJ's work anyway.) I think we can run internal deprecation at a different pace than external deprecation. > I don't know enough about the functionality GDB expects from linking with > -lmcheck by default in development to know whether other malloc > improvements would provide similar checking by default or whether that > would also need GDB to provide its own malloc (again, I think valgrind is > vastly too heavyweight for using by default in GDB development). I wasn't aware that GDB developers are using this functionality. I'll ask them how critical it is to their needs. But I expect that there will be no glibc release which neither provides mcheck, nor provides an alternative. Thanks, Florian
On Tue, 15 Nov 2016, Florian Weimer wrote: > > I don't consider valgrind suitable for replacing the uses of mtrace in the > > glibc testsuite (or other similar uses elsewhere for lightweight checking > > for leaks). > > I completely agree. > > The above is for the NEWS file, for external use. For internal use, we still > need a solution. (I have an mtrace-compatible interposition-based tracer > almost finished, but it may not make the cut for the next release, and it may > be superseded by DJ's work anyway.) I think we can run internal deprecation > at a different pace than external deprecation. Well, I think mtrace (meaning the ability to run with tracing then run the mtrace script to process the results, more than the functions to turn tracing on and off) is similarly useful externally as a lightweight system for tracing allocations and detecting leaks. And it is of course documented in the manual.
On Wed, Nov 16, 2016 at 1:36 AM, Joseph Myers <joseph@codesourcery.com> wrote: > On Tue, 15 Nov 2016, Florian Weimer wrote: > >> > I don't consider valgrind suitable for replacing the uses of mtrace in the >> > glibc testsuite (or other similar uses elsewhere for lightweight checking >> > for leaks). >> >> I completely agree. >> >> The above is for the NEWS file, for external use. For internal use, we still >> need a solution. (I have an mtrace-compatible interposition-based tracer >> almost finished, but it may not make the cut for the next release, and it may >> be superseded by DJ's work anyway.) I think we can run internal deprecation >> at a different pace than external deprecation. > > Well, I think mtrace (meaning the ability to run with tracing then run the > mtrace script to process the results, more than the functions to turn > tracing on and off) is similarly useful externally as a lightweight system > for tracing allocations and detecting leaks. And it is of course > documented in the manual. It is a useful feature to use on constrained systems to do basic allocation tracing. The main deficiency it has is it is not thread-safe, which makes it rather useless for many modern applications. In an ideal world glibc malloc would support a wider range of tracing and profiling features similar to jemalloc/tcmalloc, e.g. thread-safe allocation tracing, pprof.
On 11/16/2016 02:36 AM, Joseph Myers wrote: > On Tue, 15 Nov 2016, Florian Weimer wrote: > >>> I don't consider valgrind suitable for replacing the uses of mtrace in the >>> glibc testsuite (or other similar uses elsewhere for lightweight checking >>> for leaks). >> >> I completely agree. >> >> The above is for the NEWS file, for external use. For internal use, we still >> need a solution. (I have an mtrace-compatible interposition-based tracer >> almost finished, but it may not make the cut for the next release, and it may >> be superseded by DJ's work anyway.) I think we can run internal deprecation >> at a different pace than external deprecation. > > Well, I think mtrace (meaning the ability to run with tracing then run the > mtrace script to process the results, more than the functions to turn > tracing on and off) is similarly useful externally as a lightweight system > for tracing allocations and detecting leaks. And it is of course > documented in the manual. In my experience, it is not useful at all for finding the cause of leaks because you only get the address of the immediate caller of malloc, which is often a wrapper. The address is subject to ASLR as well. Based on the comments in this thread, I'm not sure if people find mtrace theoretically useful, or actually use it. :) In my proposed NEWS entry, I forgot to mention Address Sanitizer (as was pointed out in the GDB discussion). It has leak detection capabilities as well: ==7440==ERROR: LeakSanitizer: detected memory leaks Direct leak of 32 byte(s) in 1 object(s) allocated from: #0 0x7fc52f77097a in malloc (/lib64/libasan.so.2+0x9897a) #1 0x400703 in main (/tmp/a.out+0x400703) #2 0x7fc52f33757f in __libc_start_main (/lib64/libc.so.6+0x2057f) And it runs significantly faster than valgrind. Florian
On Thu, 17 Nov 2016, Florian Weimer wrote: > > Well, I think mtrace (meaning the ability to run with tracing then run the > > mtrace script to process the results, more than the functions to turn > > tracing on and off) is similarly useful externally as a lightweight system > > for tracing allocations and detecting leaks. And it is of course > > documented in the manual. > > In my experience, it is not useful at all for finding the cause of leaks > because you only get the address of the immediate caller of malloc, which is > often a wrapper. The address is subject to ASLR as well. > > Based on the comments in this thread, I'm not sure if people find mtrace > theoretically useful, or actually use it. :) I use it for similar purposes to the glibc testsuite use: compile code once then possibly run it with tracing to check for leaks. (Where the expectation is that the cause of the leak is in the code you just added/changed that resulted in the check for leaks starting to fail.) I'd be fine with extending the trace format to allow listing more of a backtrace (using glibc's backtrace functions in the preloadable tracing malloc implementation).
On 11/17/2016 08:00 AM, Florian Weimer wrote: > In my experience, it is not useful at all for finding the cause of leaks > because you only get the address of the immediate caller of malloc, > which is often a wrapper. The address is subject to ASLR as well. > > Based on the comments in this thread, I'm not sure if people find mtrace > theoretically useful, or actually use it. :) I make use of mtrace extensively when debugging oomkills on Cray systems. Traditional memory leaks are typically not as big a problem as heap fragmentation. Special hardware needs require non-default mallopt settings, notably severly restricting use of mmap, so fragmentation is a much larger problem for us than for many others. In my experience, mtrace is lightweight enough to not noticeably perturb the heap structure over the course of the program, usually allowing me to pinpoint which allocations are leading to fragmentation. Heavier instrumentation, such as valgrind, messes with it far too much to isolate which allocations from the application are actually causing fragmentation when the instrumentation is not present, making them much less useful to me. > In my proposed NEWS entry, I forgot to mention Address Sanitizer (as was > pointed out in the GDB discussion). It has leak detection capabilities > as well: > > ==7440==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 32 byte(s) in 1 object(s) allocated from: > #0 0x7fc52f77097a in malloc (/lib64/libasan.so.2+0x9897a) > #1 0x400703 in main (/tmp/a.out+0x400703) > #2 0x7fc52f33757f in __libc_start_main (/lib64/libc.so.6+0x2057f) > > And it runs significantly faster than valgrind. Does it have a noticeable impact on the layout of the heap compared to non-instrumented runs? Steven Vormwald
On 17/11/2016 12:50, Steve Vormwald wrote: > On 11/17/2016 08:00 AM, Florian Weimer wrote: >> In my experience, it is not useful at all for finding the cause of leaks >> because you only get the address of the immediate caller of malloc, >> which is often a wrapper. The address is subject to ASLR as well. >> >> Based on the comments in this thread, I'm not sure if people find mtrace >> theoretically useful, or actually use it. :) > > I make use of mtrace extensively when debugging oomkills on Cray > systems. Traditional memory leaks are typically not as big a problem as > heap fragmentation. Special hardware needs require non-default > mallopt settings, notably severly restricting use of mmap, so fragmentation > is a much larger problem for us than for many others. In my experience, > mtrace is lightweight enough to not noticeably perturb the heap structure > over the course of the program, usually allowing me to pinpoint which allocations > are leading to fragmentation. Heavier instrumentation, such as valgrind, > messes with it far too much to isolate which allocations from the application > are actually causing fragmentation when the instrumentation is not present, > making them much less useful to me. > >> In my proposed NEWS entry, I forgot to mention Address Sanitizer (as was >> pointed out in the GDB discussion). It has leak detection capabilities >> as well: >> >> ==7440==ERROR: LeakSanitizer: detected memory leaks >> >> Direct leak of 32 byte(s) in 1 object(s) allocated from: >> #0 0x7fc52f77097a in malloc (/lib64/libasan.so.2+0x9897a) >> #1 0x400703 in main (/tmp/a.out+0x400703) >> #2 0x7fc52f33757f in __libc_start_main (/lib64/libc.so.6+0x2057f) >> >> And it runs significantly faster than valgrind. > > Does it have a noticeable impact on the layout of the heap compared to > non-instrumented runs? Yes, since it is built on ASAN it uses an interposed malloc implementation and it also uses more VMA (as shadow memory for internal error detection).
On 11/17/2016 03:50 PM, Steve Vormwald wrote: > On 11/17/2016 08:00 AM, Florian Weimer wrote: >> In my experience, it is not useful at all for finding the cause of leaks >> because you only get the address of the immediate caller of malloc, >> which is often a wrapper. The address is subject to ASLR as well. >> >> Based on the comments in this thread, I'm not sure if people find mtrace >> theoretically useful, or actually use it. :) > > I make use of mtrace extensively when debugging oomkills on Cray > systems. Traditional memory leaks are typically not as big a problem as > heap fragmentation. Special hardware needs require non-default > mallopt settings, notably severly restricting use of mmap, so fragmentation > is a much larger problem for us than for many others. This is very interesting, thanks. I've been considering for a while to remove the use of sbrk from the allocator, both to simplify it and to take away the possibility for attacks to tweak the NON_MAIN_ARENA flag to trick the allocator into doing something bad. Some embedded targets appear to strongly prefer the sbrk as well, so maybe this approach is doomed. Would your configuration have problems if we handed all allocations which are larger than 1 MiB or so straight to mmap? > In my experience, > mtrace is lightweight enough to not noticeably perturb the heap structure > over the course of the program, usually allowing me to pinpoint which allocations > are leading to fragmentation. Heavier instrumentation, such as valgrind, > messes with it far too much to isolate which allocations from the application > are actually causing fragmentation when the instrumentation is not present, > making them much less useful to me. DJ's tracer should cover that well because it's designed to alter heap layout at all (its own allocations use mmap). We would also be interested in general patterns of fragmentation you have encountered, if any. Maybe there's something we can do about them. >> In my proposed NEWS entry, I forgot to mention Address Sanitizer (as was >> pointed out in the GDB discussion). It has leak detection capabilities >> as well: >> >> ==7440==ERROR: LeakSanitizer: detected memory leaks >> >> Direct leak of 32 byte(s) in 1 object(s) allocated from: >> #0 0x7fc52f77097a in malloc (/lib64/libasan.so.2+0x9897a) >> #1 0x400703 in main (/tmp/a.out+0x400703) >> #2 0x7fc52f33757f in __libc_start_main (/lib64/libc.so.6+0x2057f) >> >> And it runs significantly faster than valgrind. > > Does it have a noticeable impact on the layout of the heap compared to > non-instrumented runs? Eh, yes. It's a totally different allocator. So I can see why mtrace is attractive. Thanks, Florian
> On 11/18/2016 04:13 AM, Florian Weimer wrote: > I've been considering for a while to remove the use of sbrk from the > allocator, both to simplify it and to take away the possibility for > attacks to tweak the NON_MAIN_ARENA flag to trick the allocator into > doing something bad. Some embedded targets appear to strongly prefer > the sbrk as well, so maybe this approach is doomed. For us anyway, the use of sbrk isn't as important as its semantics (eg, heap is contiguous in virtual address space and only grows/shrinks on one end). Thus, things like libhugetlbfs that provide the same semantics via __morecore work just as well. > Would your configuration have problems if we handed all allocations > which are larger than 1 MiB or so straight to mmap? That would probably not work out for us. The underlying problem we have is that unmapping pages can be extremely expensive (on the order of tens of seconds or worse in certain circumstances). We use segments with fixed base addresses and maximum length to transparently make portions of a process's virtual address space (notably including all user allocations) available for hardware DMA. Updating page mappings within a segment is extremely cheap. Adding a new segment is also relatively cheap, but they are a limited resource. Invalidating a segment so it can be reused can get very expensive since everything that has used it needs to be notified of the invalidation. We can take a fast path if the allocator has sbrk-like semantics as described above because we know it only requires updating page mappings, assuming the existing segment is large enough. We have to be much more conservative for allocations that are satisfied with mmap, leading to a lot of expensive and potentially unnecessary segment creation and invalidation. > We would also be interested in general patterns of fragmentation you > have encountered, if any. Maybe there's something we can do about them. The usual culprit is something "caching" small (<128byte) allocations for future reuse rather than freeing them when they become unused, which prevents the allocator from coalescing free regions to satisfy larger allocations. Changing the code either to allocate in bulk instead of one object at a time or to simply free them without caching is generally sufficient to fix the fragmentation issue. Steven Vormwald
Steve Vormwald <sdvormwa@cray.com> writes: > The usual culprit is something "caching" small (<128byte) allocations for > future reuse rather than freeing them when they become unused, which Have you tried disabling fastbins? I.e. mallopt(M_MXFAST, 0) ?
I still feel we have a fairly complete set of replacements available today (Address Sanitizer, valgrind for debugging; interposition instead of __more_core &c), and that it makes sense to announce our planned deprecation early. It does, however, look rather unlikely I will have concrete to show by the end of December 2016. Final comments? Thanks, Florian
diff --git a/NEWS b/NEWS index ea1a0e0..7cbaa28 100644 --- a/NEWS +++ b/NEWS @@ -67,6 +67,11 @@ Version 2.25 for the Linux quota interface which predates kernel version 2.4.22 has been removed. +* <mcheck.h> and all malloc hook functions are now deprecated. Future + implementations of the mcheck- and mtrace-related functions will not have + any effect, and glibc will stop calling the hook functions from its malloc + implementation. + * The malloc_get_state and malloc_set_state functions have been removed. Already-existing binaries that dynamically link to these functions will get a hidden implementation in which malloc_get_state is a stub. As far diff --git a/malloc/malloc.h b/malloc/malloc.h index e0c2788..a18401e 100644 --- a/malloc/malloc.h +++ b/malloc/malloc.h @@ -68,11 +68,11 @@ extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur; /* Underlying allocation function; successive calls should return contiguous pieces of memory. */ -extern void *(*__morecore) (ptrdiff_t __size); +extern void *(*__morecore) (ptrdiff_t __size) __MALLOC_DEPRECATED; /* Default value of `__morecore'. */ extern void *__default_morecore (ptrdiff_t __size) -__THROW __attribute_malloc__; +__THROW __attribute_malloc__ __MALLOC_DEPRECATED; /* SVID2/XPG mallinfo structure */ @@ -149,7 +149,8 @@ extern void *(*__MALLOC_HOOK_VOLATILE __memalign_hook)(size_t __alignment, size_t __size, const void *) __MALLOC_DEPRECATED; -extern void (*__MALLOC_HOOK_VOLATILE __after_morecore_hook) (void); +extern void (*__MALLOC_HOOK_VOLATILE __after_morecore_hook) (void) +__MALLOC_DEPRECATED; /* Activate a standard set of debugging hooks. */ extern void __malloc_check_init (void) __THROW __MALLOC_DEPRECATED; diff --git a/malloc/mcheck.h b/malloc/mcheck.h index 416fcd6..aa74b1b 100644 --- a/malloc/mcheck.h +++ b/malloc/mcheck.h @@ -15,11 +15,20 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +/* Note: This header file is deprecated and will be removed in a + future glibc version. */ + #ifndef _MCHECK_H #define _MCHECK_H 1 #include <features.h> +#ifdef _LIBC +# define __MCHECK_DEPRECATED +#else +# define __MCHECK_DEPRECATED __attribute_deprecated__ +#endif + __BEGIN_DECLS /* Return values for `mprobe': these are the kinds of inconsistencies that @@ -38,23 +47,25 @@ enum mcheck_status before `malloc' is ever called. ABORTFUNC is called with an error code (see enum above) when an inconsistency is detected. If ABORTFUNC is null, the standard function prints on stderr and then calls `abort'. */ -extern int mcheck (void (*__abortfunc)(enum mcheck_status)) __THROW; +extern int mcheck (void (*__abortfunc)(enum mcheck_status)) + __THROW __MCHECK_DEPRECATED; /* Similar to `mcheck' but performs checks for all block whenever one of the memory handling functions is called. This can be very slow. */ -extern int mcheck_pedantic (void (*__abortfunc)(enum mcheck_status)) __THROW; +extern int mcheck_pedantic (void (*__abortfunc)(enum mcheck_status)) + __THROW __MCHECK_DEPRECATED; /* Force check of all blocks now. */ -extern void mcheck_check_all (void); +extern void mcheck_check_all (void) __MCHECK_DEPRECATED; /* Check for aberrations in a particular malloc'd block. You must have called `mcheck' already. These are the same checks that `mcheck' does when you free or reallocate a block. */ -extern enum mcheck_status mprobe (void *__ptr) __THROW; +extern enum mcheck_status mprobe (void *__ptr) __THROW __MCHECK_DEPRECATED; /* Activate a standard collection of tracing hooks. */ -extern void mtrace (void) __THROW; -extern void muntrace (void) __THROW; +extern void mtrace (void) __THROW __MCHECK_DEPRECATED; +extern void muntrace (void) __THROW __MCHECK_DEPRECATED; __END_DECLS #endif /* mcheck.h */