Add Valgrind GC integration #15304

CyberShadow · 2023-06-10T09:14:15Z

Resurrecting some old work from 2015.

Hopefully may help with problems such as https://issues.dlang.org/show_bug.cgi?id=23978.

What this does

This integrates Valgrind (specifically, the memcheck tool) with the D GC. It allows catching bad code such as the following:

import core.memory;

void main()
{
	auto arr = new int[3];
	GC.free(arr.ptr);
	arr[1] = 42;
}

The integration makes it so that harmless memory operations (such as those done by the GC) don't emit spurious warnings, and invalid memory operations (such as in the example above) do emit warnings.

How to use it

~~WIP (will add docs)~~ See changelog; the goal is to be able to do essentially

dmd -debug=VALGRIND path/to/conservative/gc.d my_program.d

to get the integration. (I.e., add the GC-related parts of Druntime to the compilation of your program so that it is rebuilt with -debug=VALGRIND).

dlang-bot · 2023-06-10T09:14:18Z

Thanks for your pull request, @CyberShadow!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15304"

CyberShadow · 2023-06-10T10:29:17Z

This relies on the Valgrind API, which is defined in the valgrind/valgrind.h and valgrind/memcheck.h header files. If Valgrind is installed, the header files can be used in C programs with e.g. #include <valgrind/valgrind.h>.

How we can approach this for the D side:

Make everything Valgrind-related opt-in. Nothing is changed requirements-wise, however, Valgrind integration can't be tested in the regular CI flow, and using the integration will be non-trivial (e.g. require rebuilding Druntime/Phobos or invoking a C compiler).
Add Valgrind as a build dependency for our Linux builds (release and CI), and use /usr/include/valgrind/memcheck.h. Is this OK?
Vendor the header files. They are BSD-licensed, so I think this should be OK too.
~~Use ImportC to use the C header files at user program build time.~~ Unfortunately ImportC isn't ready for this, e.g. for one thing it would need to understand GNU assembler syntax...
Port the necessary parts of the header files to D. I would not recommend this as they are full of platform-specific details.

I'm going to go with option 3 as it seems the most flexible and least troublesome.

This is not portable, and memcheck complains about this.

Add the BSD-licensed C header files which provide the API for controlling Valgrind from within programs executed under it. The files are from Valgrind v3.21.0.

We will use these in the GC implementation to tell Valgrind which memory operations are OK or not.

Allow the conservative GC to scan memory, whether it has been initialized by the application or not.

dkorpel

Nice work!

dkorpel · 2023-06-10T17:27:03Z

druntime/src/etc/valgrind/valgrind.d

+    void _d_valgrind_make_mem_defined  (const(void)* addr, size_t len);
+}
+
+void makeMemNoAccess (const(void)[] mem) nothrow { _d_valgrind_make_mem_noaccess (mem.ptr, mem.length); }


Please add ddoc comments to public functions

Thanks! Done.

ljmf00 · 2023-06-11T18:36:32Z

Awesome! CC @JohanEngelen does this gives some lights about compiler-rt ASAN being aware of the GC? I think valgrind interfaces are very similar

WalterBright · 2023-06-12T02:35:13Z

Use ImportC to use the C header files at user program build time. Unfortunately ImportC isn't ready for this, e.g. for one thing it would need to understand GNU assembler syntax...

ImportC currently replaces instances of GNU asm with assert(0). This enables compilation of the functions with inline asm in them, they just will assert if they try to execute the inline asm. The rest of the .h file will work. I haven't looked at the .h files here, but it seems to me that a manual conversion to D will wind up doing the same thing. Create a .c file that instantiates all the asm code, exposing them as functions, and then D can access them.

I'm interested in any and all problems with ImportC.

changelog/druntime.valgrind.dd

CyberShadow · 2023-06-12T04:39:18Z

ImportC currently replaces instances of GNU asm with assert(0). This enables compilation of the functions with inline asm in them, they just will assert if they try to execute the inline asm.

Okay, but that doesn't help us very much because the critical parts are all in inline assembly.

The rest of the .h file will work.

The remaining parts (about 20%) wrap the assembly code to create the public API.

I haven't looked at the .h files here, but it seems to me that a manual conversion to D will wind up doing the same thing.

I don't understand what that would mean in this context.

Create a .c file that instantiates all the asm code, exposing them as functions, and then D can access them.

This is what the implementation in this PR does.

CyberShadow · 2023-06-12T04:55:49Z

I think it would help if I explained how the Valgrind client API works.

Valgrind is a binary retranslation tool. You give it a compiled program containing x86_64 etc. instructions, and before running it, Valgrind translates it to a different x86_64 program, with some instrumentation added. As such, it is something akin to both a VM and a JIT.

As it is translating the program, it instruments it for additional functionality, depending on the tool used. Among other things, the memcheck tool tracks, on a bit-by-bit level, which variables (including in the stack and registers) hold defined values, and which do not. So, if the program were to dereference a value from a freshly-returned malloc call, or a byte from a struct padding, and then make a decision based on that value, memcheck will flag it as an error.

The client API controls the Valgrind retranslator. When Valgrind encounters these special instructions while translating our program's code, it will understand these as requests to adjust its internal state accordingly (i.e. to pretend that a copy of some value is defined just this once, or mark some memory range as undefined). The header file contains these instructions.

Worth pointing out that the instructions are "junk" no-op instructions that don't do anything if the program isn't running under Valgrind, so one thing we could actually do is enable Valgrind for our debug Phobos builds. At the cost of a very small slowdown, that will allow users to just run debug builds of D programs under Valgrind and get correct diagnostics about invalid memory operations.

The two share the same goal: mark memory which should not be accessed any more.

The GC stores free lists in the cells of the objects on the list. We would like to allow the GC to work with these lists, but still raise a warning if application code attempts to access them.

Freshly allocated memory should be writable but not readable. Explicitly deallocated or GC-ed memory should be neither readable or writable.

Copy/adapt the documentation in memcheck.h.

Run the Druntime Valgrind integration tests. libc6-dbg is needed to allow Valgrind to redirect certain functions.

Although the GC is the primary user, this is not a restriction of these Valgrind API wrappers.

Trick the optimizer to pretend we're doing something with the result of those invalid memory accesses.

JohanEngelen · 2023-06-12T10:40:00Z

Awesome! CC @JohanEngelen does this gives some lights about compiler-rt ASAN being aware of the GC? I think valgrind interfaces are very similar

Indeed. I have similar unfinished work.
Adding ASAN support is probably straightforward because the marking/poisoning/invalidation code is separated into special functions (including the "write to this invalid memory area" is very useful! undefinedWrite).

dkorpel · 2023-06-16T11:01:08Z

changelog/druntime.valgrind.dd

+
+```
+git clone -b v2.105.0 --depth=1 https://github.com/dlang/dmd
+dmd -g -debug=VALGRIND program.d -Idmd/druntime/src dmd/druntime/src/{core/internal/gc/impl/conservative/gc,rt/lifetime,etc/valgrind/valgrind}.d


I tried this, and it results in undefined reference to '_d_valgrind_make_mem_noaccess' etc., because it doesn't tell you to cc -c druntime/src/etc/valgrind/valgrind.c -o valgrind.a and include that as a source file. Other than that, works like a charm!

This documentation should not be only in the changelog.

The C parts should be compiled into libphobos.a everywhere on Posix. Check that you're using libphobos.a built from master, maybe?

I think valgrind.d could have version (Posix): instead of debug (VALGRIND): at the top to avoid having to recompile it too, but I did not experiment with it too much. There's some issues like the D file pulling in the C declarations even when the D declarations are not used by anything on some platforms...

You're right, the dmd command launches v2.104.0 from my package manager, but when I use dmd built from master, it includes the valgrind symbols.

CyberShadow · 2023-06-16T14:08:30Z

This documentation should not be only in the changelog.

Agreed. I think we also need a centralized place for us to document the --DRT switches. Maybe Druntime should have a documentation folder with just plain DDoc files, or something like that.

CyberShadow and others added 2 commits June 10, 2023 13:02

rt.minfo: Avoid calling realloc with size 0

a4e4ca8

This is not portable, and memcheck complains about this.

Add C Valgrind API

8e8a5f7

Add the BSD-licensed C header files which provide the API for controlling Valgrind from within programs executed under it. The files are from Valgrind v3.21.0.

CyberShadow force-pushed the valgrind branch from 6fe27dc to 896cc7c Compare June 10, 2023 13:13

CyberShadow added 2 commits June 10, 2023 13:47

Add some D bindings for the Valgrind C API

30b0423

We will use these in the GC implementation to tell Valgrind which memory operations are OK or not.

Silence Valgrind warnings caused by GC marking

79ececd

Allow the conservative GC to scan memory, whether it has been initialized by the application or not.

CyberShadow force-pushed the valgrind branch 2 times, most recently from 9517710 to 8361858 Compare June 10, 2023 13:53

CyberShadow marked this pull request as ready for review June 10, 2023 14:23

CyberShadow requested review from MartinNowak, wilzbach and ibuclaw as code owners June 10, 2023 14:23

CyberShadow force-pushed the valgrind branch from 8361858 to accfb6b Compare June 10, 2023 14:23

dkorpel reviewed Jun 11, 2023

View reviewed changes

CyberShadow force-pushed the valgrind branch from 221cde3 to ae3e5d2 Compare June 11, 2023 17:31

WalterBright reviewed Jun 12, 2023

View reviewed changes

changelog/druntime.valgrind.dd Show resolved Hide resolved

CyberShadow force-pushed the valgrind branch 3 times, most recently from c31b0d2 to ed43197 Compare June 12, 2023 08:29

CyberShadow and others added 6 commits June 12, 2023 09:24

Add test suite for Valgrind

86cfca9

Unify VALGRIND with MEMSTOMP

9dde08f

The two share the same goal: mark memory which should not be accessed any more.

Mask "invalid" access to free lists

ae5013b

The GC stores free lists in the cells of the objects on the list. We would like to allow the GC to work with these lists, but still raise a warning if application code attempts to access them.

Distinguish between writable and unwritable invalidation

397faed

Freshly allocated memory should be writable but not readable. Explicitly deallocated or GC-ed memory should be neither readable or writable.

Add use-after-free test

0ca5b76

Invalidate freshly-allocated memory

866f8c7

CyberShadow and others added 8 commits June 12, 2023 09:24

Integrate VALGRIND with SENTINEL

1ca05ab

Fix reporting of arrays of structs with destructors

a1bac4d

Re-invalidate reused blocks during malloc/calloc

998edd8

Add Changelog entry

b86cf01

.pre-commit-config.yaml: Exempt vendored files

e10ea2d

etc.valgrind.valgrind: Propagate return values of the vbits functions

7266c6b

etc.valgrind.valgrind: Add documentation for functions

e76e2be

Copy/adapt the documentation in memcheck.h.

druntime/test/valgrind: Add no_use_after_gc test

eb2f791

CyberShadow force-pushed the valgrind branch from ed43197 to a17ff4a Compare June 12, 2023 09:24

CyberShadow added 3 commits June 12, 2023 10:14

ci/cirrusci.sh: Install Valgrind on Linux

566ff4a

Run the Druntime Valgrind integration tests. libc6-dbg is needed to allow Valgrind to redirect certain functions.

etc.valgrind.valgrind: Add @nogc

9d9f639

Although the GC is the primary user, this is not a restriction of these Valgrind API wrappers.

druntime/test/valgrind: Fix no_use_after_gc test with -release

d0092f2

Trick the optimizer to pretend we're doing something with the result of those invalid memory accesses.

CyberShadow force-pushed the valgrind branch from 18cbde8 to d0092f2 Compare June 12, 2023 10:14

WalterBright merged commit 7cdae6e into dlang:master Jun 15, 2023

dkorpel reviewed Jun 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Valgrind GC integration #15304

Add Valgrind GC integration #15304

CyberShadow commented Jun 10, 2023 •

edited

Loading

dlang-bot commented Jun 10, 2023

CyberShadow commented Jun 10, 2023 •

edited

Loading

dkorpel left a comment

dkorpel Jun 10, 2023

CyberShadow Jun 11, 2023

ljmf00 commented Jun 11, 2023

WalterBright commented Jun 12, 2023

CyberShadow commented Jun 12, 2023

CyberShadow commented Jun 12, 2023 •

edited

Loading

JohanEngelen commented Jun 12, 2023

dkorpel Jun 16, 2023 •

edited

Loading

CyberShadow Jun 16, 2023

dkorpel Jun 16, 2023

CyberShadow commented Jun 16, 2023 •

edited

Loading

Add Valgrind GC integration #15304

Add Valgrind GC integration #15304

Conversation

CyberShadow commented Jun 10, 2023 • edited Loading

What this does

How to use it

dlang-bot commented Jun 10, 2023

Bugzilla references

Testing this PR locally

CyberShadow commented Jun 10, 2023 • edited Loading

dkorpel left a comment

Choose a reason for hiding this comment

dkorpel Jun 10, 2023

Choose a reason for hiding this comment

CyberShadow Jun 11, 2023

Choose a reason for hiding this comment

ljmf00 commented Jun 11, 2023

WalterBright commented Jun 12, 2023

CyberShadow commented Jun 12, 2023

CyberShadow commented Jun 12, 2023 • edited Loading

JohanEngelen commented Jun 12, 2023

dkorpel Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

CyberShadow Jun 16, 2023

Choose a reason for hiding this comment

dkorpel Jun 16, 2023

Choose a reason for hiding this comment

CyberShadow commented Jun 16, 2023 • edited Loading

CyberShadow commented Jun 10, 2023 •

edited

Loading

CyberShadow commented Jun 10, 2023 •

edited

Loading

CyberShadow commented Jun 12, 2023 •

edited

Loading

dkorpel Jun 16, 2023 •

edited

Loading

CyberShadow commented Jun 16, 2023 •

edited

Loading