-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/link: valgrind compatible output #782
Comments
Not likely to happen. Valgrind is really a tool for C programs. It assumes it can replace malloc with its own copy, it assumes the standard C stack model, I'm sure it assumes other things specific to that world too. Equally important, Valgrind doesn't lend much benefit to Go. Go has no uninitialized variables and no dangling pointers. I understand that it could be useful for debugging problems with cgo, but I'm not sure that it would be in practice. If anyone wants to work on this, patches welcome. Labels changed: added priority-low, expertneeded, removed priority-medium. Status changed to LongTerm. |
Valgrind bug reported here: https://bugs.kde.org/show_bug.cgi?id=262916 |
The latest Valgrind from SVN actually works. I think I was still running 3.6.1 in my previous test. Valgrind produces some of the following warnings: Conditional jump or move depends on uninitialised value(s) Use of uninitialised value of size x Invalid read/write of size x I'll take a closer look at these now. |
I think further debugging is needed with the latest version of Valgrind, and Go developers are probably better equipped to understand out what Valgrind is doing wrong (if anything), even if Valgrind developers fix it then. Finally, it probably leads to some VALGRIND_MAKE_MEM_DEFINED bits in the Go runtime code. I will test the state of things with Go tip and the latest Valgrind and report back. |
With Valgrind 3.8.1 on any binary I get: ==6081== Warning: ignored attempt to set SIGRT32 handler in sigaction(); ==6081== the SIGRT32 signal is used internally by Valgrind fatal error: rt_sigaction failure (this is from go) So I think the runtime should skip setting up an action for SIGRT32? I will play with it a bit. |
With the rt_sigaction error check disabled, this sometimes works: valgrind --tool=none -v ./reflect.test -test.v but also sometimes fails with nil pointer derefs, etc. This points to some issues in the Valgrind runtime. valgrind --tool=memcheck survives some packages but segfaults on reflect. So next steps: 1. Fix the SIGRT32 sigaction thing in the Go runtime. Some guidance would be appreciated. 2. Fix the Valgrind runtime so that nullgrind can always run all the tests. 3. Tackle the memcheck segfault/stacktrace debugging task from the valgrind-developers mailing list: """ You might use the Valgrind gdbserver to use gdb to do the stacktrace just before the error is reported by Valgrind. If gdb+Valgrind gdbserver can produce a stacktrace of the simulated cpu and Valgrind "core" cannot make a stacktrace with the same simulated cpu state, then it looks like the Valgrind stacktrace logic is to be enhanced/corrected. If both can't make a stacktrace, but the native gdb can do a stacktrace, then that looks more like a Valgrind simulation bug. """ 4. Add the stuff from http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq to the Go runtime code to hopefully cut down on spurious Valgrind warnings. |
It actually makes more sense to run valgrind on go programs than is apparent at first glance: Some fuzzy testing and similar tools (for example https://code.google.com/p/avalanche) use valgrind to run the program being tested. |
robryk, what exactly valgrind tool do you mean? memcheck makes no sense for Go, because Go does not have out-of-bounds, use-after-free and leaks (in C sense) helgrind could make sense, but helgrind does not support Go and w/o this support we can't do anything; also Go has own race detector which covers most of helgrind functionality I propose to close this issue. It's un-actionable and confusing. Valgrind is for C/C++. |
Right now (go 1.4) valgrind reports
|
any progress on this? use valgrind for CGo libs will be a big plus |
It seems the valgrind developer probably have fixed the issue on their side.
I tried to run reflect.test under valgrind --tool=none (version 3.11.0),
and the test passes.
various other tests are also passing, including the main cgo test
($GOROOT/misc/cgo/test).
That said, Memcheck still reports a lot of warning for the simplest go code
($GOROOT/test/helloworld.go), and I don't think we can fix those because
valgrind seems to make some C ABI assumptions that are invalid in Go ABI
and don't understand Go's moving stack.
https://gist.github.com/minux/957dbc35a48be75588cb0f386cda6a60
Perhaps we should close this issue instead, I think Memcheck is never going
to work and Nulgrind already runs all the Go programs that I tried. Any
objections?
|
Is there a way to have memcheck ignore Go code part ? after all, like
previous message suggest, we only want it to check linked C-libraries.
…On Fri, Feb 10, 2017 at 5:09 AM, Russ Cox ***@***.***> wrote:
Closed #782 <#782>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#782 (comment)>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/ABMsELkTlZc-z4SQAS9irxWWTSjxJltmks5rbGF3gaJpZM4EBPnB>
.
|
I don't know. It would be better to bring this up with the memcheck authors. If you find out something simple that can be done, great. Note that Go does work with https://github.com/google/sanitizers/wiki/MemorySanitizer and https://github.com/google/sanitizers/wiki/AddressSanitizer. |
The text was updated successfully, but these errors were encountered: