On Mon, 23 Nov 2015, Martin Li?ka wrote: > On 11/21/2015 05:26 AM, Hans-Peter Nilsson wrote: > > On Thu, 19 Nov 2015, Martin Li?ka wrote: > >> Hello. > >> > >> In last two weeks I've removed couple of memory leaks, mainly tight to > >> middle-end. > >> Currently, a user of the GCC compiler can pass > >> '--enable-checking=valgrind' configure option > >> that will run all commands within valgrind environment, but as the > >> valgrind runs just with '-q' option, > >> the result is not very helpful.
I disagree. Bugs due to uninitialized memory are diagnosed and pin-pointed, as intended. If that has regressed, that's a bug. > >> I would like to start with another approach, where we can run all tests in > >> test-suite > >> within the valgrind sandbox and return an exit code if there's an error > >> seen by the tool. That's exactly what --enable-checking=valgrind does (well, is supposed to do. (Leaks not being diagnosed as errors.) > >> That unfortunately leads to many latent (maybe false positives, FE issues, > >> ...) that can > >> be efficiently ignored by valgrind suppressions file (the file is part of > >> suggested patch). How can you be sure about the false positives? It sounds like you're papering over bugs, perhaps bit-rot due to missing annotations? > >> The first version of the valgrind.supp can survive running compilation of > >> tramp3d with -O2 > >> and majority of tests in test-suite can successfully finish. Most of > >> memory leaks > >> mentioned in the file can be eventually fixed. Then better not suppress them. If they aren't visible with valgrind -q perhaps due to being leaks rather than use of uninitialized memory, then make that a separate mode. > > I didn't quite understand the need for the suppression files. > > Is it like Markus said, only because valgrind annotations are > > not on by default? Then let's change it so that's the default > > during DEV-PHASE = experimental (the development phase) or > > prerelease. I really thought that was the case by now. > > (The suppression files are IMHO a useful addition to contrib/ > > either way.) > > Hi. > > Well, the original motivation was to basically to fill up the file with all > common > errors (known issues) and to fix all newly introduced issues. That can > minimize > the number of errors reported by the tool. There *are* lots of bugs seen with valgrind -q and those should be fixed, not suppressed. If you want a suppression mode, make that separate. > However, as I run complete test-suite for all default languages, I've seen: > > == Statistics == > Total number of errors: 249615 > Number of different errors: 5848 > > Where two errors are different if they produce either different message or > back-backtrace. > For complete list of errors (sorted by # of occurrences), download: > > https://docs.google.com/uc?authuser=0&id=0B0pisUJ80pO1MENrWXBzak5naFk&export=download > > > > >> As I noticed in results log files, most of remaining issues are connected > >> to gcc.c and > >> lto-wrapper.c files. gcc.c heavily manipulates with strings and it would > >> probably require > >> usage of a string pool, that can easily eventually removed (just in case > >> of --enable-valgrind-annotations). > >> The second source file tends to produce memory leaks because of fork/exec > >> constructs. However both > >> can be improved during next stage1. > >> > >> Apart from aforementioned issues, the compiler does not contain so many > >> issues and I think it's > >> doable to prune them and rely on reported valgrind errors. > >> > >> Patch touches many .exp files, but basically does just couple of > >> modifications: > >> > >> 1) gcc-defs.exp introduces new global variable run_under_valgrind > >> 2) new procedure dg-run-valgrind distinguishes between just passing > >> options to 'gd-test', > >> or runs 'dg-test' with additional flags that enable valgrind (using > >> -wrapper) > >> 3) new procedure dg-runtest-valgrind does the similar > >> 4) many changes in corresponding *.exp files that utilize these procedures > >> > >> The patch should be definitely part of next stage1, but I would appreciate > >> any thoughts > >> about the described approach? > > > > IIRC you can replace the actual dg-runtest proc with your own > > (implementing a wrapper). Grep aroung, I think we do that > > already. That's certainly preferable instead of touching all > > callers. > > You are right, the suggested patch was over-kill, wrapper should be fine for > that. > Currently I've been playing with a bit different approach (suggested by > Markus), > where I would like to enable valgrind in gcc.c using an environmental > variable. > > Question is if it should replace existing ENABLE_VALGRIND_CHECKING and how to > integrate it with a valgrind suppressions file? My answer is "no": by default nothing should be suppressed with --enable-checking=valgrind and the effect should be that annotations are active and valgrind -q is the wrapper. Add a suppression mode for your needs. brgds, H-P
