Improve valgrind hooking, though I'm not sure if it's helpful yet

e94df26f · Kevin Modzelewski · 96903554 · e94df26f · e94df26f · e94df26f
Commit e94df26f authored Apr 22, 2014 by Kevin Modzelewski
10 changed files
--- a/docs/INSTALLING.md
+++ b/docs/INSTALLING.md
-Pyston currently only supports installing from source; the following instructions have only been tested on Ubuntu, but should ideally work on a Mac as well.
+Pyston currently only supports installing from source; the following instructions have only been tested on Ubuntu, and currently has some non-trivial build issues on Mac (hopefully will be addressed soon once we get access to a Mac VM).
-Pyston expects to find all of its dependencies in ~/pyston_deps:
+Pyston's build system expects to find all of its dependencies in `~/pyston_deps`:
 ```
 mkdir ~/pyston_deps
 ```
+The instructions in this file assume that pyston is checked out to `~/pyston`, though it can be checked out anywhere as long as the rest of these instructions are appropriately modified.
+```
+git clone https://github.com/dropbox/pyston.git ~/pyston
+```
 ### Compiler for clang
 clang requires a fairly modern [host compiler](http://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library), so typically you will have to install a new one.  The easiest thing to do is to just create a fresh build of GCC:
@@ -61,48 +67,43 @@ wget http://download.savannah.gnu.org/releases/libunwind/libunwind-1.1.tar.gz
 tar xvf libunwind-1.1.tar.gz
 mkdir libunwind-1.1-install
 cd libunwind-1.1
-# disable shared libraries because we'll be installing this in a place that the loader can't find it.
+# disable shared libraries because we'll be installing this in a place that the loader can't find it:
 ./configure --prefix=$HOME/pyston_deps/libunwind-1.1-install --enable-shared=0
 patch -p1 <~/pyston/libunwind_patches/0001-Change-the-RBP-validation-heuristic-to-allow-size-0-.patch
 make -j4
 make install
 ```
+### zsh
+`zsh` is needed when running pyston tests.
+```
+apt-get install zsh
+```
+# Optional dependencies
+There are a number of optional dependencies that the build system knows about, but aren't strictly necessary for building and running Pyston.  Most of them are related to developing and debugging:
 ### valgrind
-Pyston currently has a build-time dependency on valgrind headers, for adding hooks to its custom memory allocator.  (TODO add a flag to enable this since it's normally not used.)  To satisfy this, do:
+Since Pyston uses a custom memory allocator, it makes use of the valgrind client hooks to help valgrind understand it.  This means that you'll need a copy of the valgrind includes around; I also suggest that if you want to use valgrind, you use a recent version since there are some instructions that LLVM emits that somewhat-recent versions of valgrind don't understand.
+To install:
 ```
 cd ~/pyston_deps
 wget http://valgrind.org/downloads/valgrind-3.9.0.tar.bz2
 tar xvf valgrind-3.9.0.tar.bz2
-```
-If you'd like to run valgrind, we recommend building this 3.9.0 release, since some older versions (such as what are in the Ubuntu 12.04 apt repositories) aren't new enough.  To finish installing, assuming you ran the above steps:
-```
-cd ~/pyston_deps
 mkdir valgrind-3.9.0-install
 cd valgrind-3.9.0
 ./configure --prefix=$HOME/pyston_deps/valgrind-3.9.0-install
 make -j4
 make install
 sudo apt-get install libc6-dbg
+cd ~/pyston/src
+echo "ENABLE_VALGRIND := 1" >> Makefile.local
 ```
-Then, add this line to your Makefile.local:
-```
-VALGRIND := VALGRIND_LIB=$(HOME)/pyston_deps/valgrind-3.9.0-install/lib/valgrind $(HOME)/pyston_deps/valgrind-3.9.0-install/bin/valgrind
-```
-### zsh
-`zsh` is needed when running pyston tests.
-```
-apt-get install zsh
-```
-# Optional dependencies
 ### Debug build of libunwind
 Assuming you've already built the normal version above:
@@ -129,7 +130,7 @@ You can then use distcc by doing `make USE_DISTCC=1`
 ### gtest
-For running the unittests, though all the unittests are currently disabled:
+For running the unittests:
 ```
 cd ~/pyston_deps
@@ -150,11 +151,8 @@ tar xvf gdb-7.6.2.tar.gz
 cd gdb-7.6.2
 ./configure
 make -j4
-```
+cd ~/pyston/src
+echo "GDB := \$(HOME)/pyston_deps/gdb-7.6.2/gdb/gdb" >> Makefile.local
-Then add this to your Makefile.local:
-```
-GDB := $(HOME)/pyston_deps/gdb-7.6.2/gdb/gdb
 ```
 ### gperftools (-lprofiler)
@@ -165,7 +163,7 @@ standard ./configure, make, make install
 ### gold
-gold is highly recommended as a faster linker.  Pyston contains build-system support for automatically using gold if available.  gold may already be installed on your system; you can check by typing `which gold`.  If it's not installed already:
+gold is highly recommended as a faster linker, and Pyston contains build-system support for automatically using gold if available.  gold may already be installed on your system; you can check by typing `which gold`.  If it's not installed already:
 ```
 cd ~/pyston_deps

--- a/src/Makefile
+++ b/src/Makefile
@@ -11,7 +11,8 @@ USE_CLANG  := 1
 USE_CCACHE := 1
 USE_DISTCC := 0
-VALGRIND := valgrind
+ENABLE_VALGRIND := 0
 GDB := gdb
 GCC_DIR := $(DEPS_DIR)/gcc-4.8.2-install
 GTEST_DIR := $(DEPS_DIR)/gtest-1.7.0
@@ -113,9 +114,17 @@ CLANG_EXE := $(LLVM_BIN)/clang++
 COMMON_CXXFLAGS := -g -Werror -Wreturn-type -Woverloaded-virtual -Wall -Wno-sign-compare -Wno-unused -I. -I../include -fno-omit-frame-pointer
 COMMON_CXXFLAGS += -std=c++11
-COMMON_CXXFLAGS += -I$(DEPS_DIR)/valgrind-3.9.0/include
 COMMON_CXXFLAGS += -Wextra -Wno-sign-compare
 COMMON_CXXFLAGS += -Wno-unused-parameter # should use the "unused" attribute
+ifeq ($(ENABLE_VALGRIND),0)
+	COMMON_CXXFLAGS += -DNVALGRIND
+	VALGRIND := false
+else
+	COMMON_CXXFLAGS += -I$(DEPS_DIR)/valgrind-3.9.0/include
+	VALGRIND := VALGRIND_LIB=$(HOME)/pyston_deps/valgrind-3.9.0-install/lib/valgrind $(HOME)/pyston_deps/valgrind-3.9.0-install/bin/valgrind
+endif
 # Extra flags to enable soon:
 # COMMON_CXXFLAGS += -Wold-style-cast -Wconversion -Wsign-conversion -Wnon-virtual-dtor -Winit-self -Wimplicit-int -Wmissing-include-dirs -Wswitch-enum -Wunused-variable -Wunused -Wstrict-overflow=5 -Wfloat-equal -Wtraditional-conversion -Wundef -Wunsafe-loop-optimizations -Wpointer-arith -Wtype-limits -Wwrite-strings -Wclobbered -Wconversion -Wempty-body -Wlogical-op -Waggregate-return -Wstrict-prototypes -Wold-style-declaration -Wold-style-definition -Wmissing-parameter-type -Wmissing-field-initializers -Wredundant-decls -Wnested-externs -Winline -Wint-to-pointer-cast -Wpointer-to-int-cast -Wlong-long -Wvla
 # Not sure about these:
@@ -134,8 +143,8 @@ COMMON_CXXFLAGS += -DDEFAULT_PYTHON_MAJOR_VERSION=$(PYTHON_MAJOR_VERSION) -DDEFA
 EXTRA_CXXFLAGS ?=
 CXXFLAGS := $(LLVM_CXXFLAGS) $(COMMON_CXXFLAGS) -O0 -DBINARY_SUFFIX= -DBINARY_STRIPPED_SUFFIX=_stripped $(EXTRA_CXXFLAGS)
-CXXFLAGS_PROFILE = $(LLVM_PROFILE_CXXFLAGS) $(COMMON_CXXFLAGS) -pg -O3 -DNDEBUG -DBINARY_SUFFIX=_release -DBINARY_STRIPPED_SUFFIX= -fno-function-sections $(EXTRA_CXXFLAGS)
+CXXFLAGS_PROFILE = $(LLVM_PROFILE_CXXFLAGS) $(COMMON_CXXFLAGS) -pg -O3 -DNDEBUG -DNVALGRIND -DBINARY_SUFFIX=_release -DBINARY_STRIPPED_SUFFIX= -fno-function-sections $(EXTRA_CXXFLAGS)
-CXXFLAGS_RELEASE := $(LLVM_RELEASE_CXXFLAGS) $(COMMON_CXXFLAGS) -O3 -fstrict-aliasing -enable-tbaa -DNDEBUG -DBINARY_SUFFIX=_release -DBINARY_STRIPPED_SUFFIX= $(EXTRA_CXXFLAGS)
+CXXFLAGS_RELEASE := $(LLVM_RELEASE_CXXFLAGS) $(COMMON_CXXFLAGS) -O3 -fstrict-aliasing -enable-tbaa -DNDEBUG -DNVALGRIND -DBINARY_SUFFIX=_release -DBINARY_STRIPPED_SUFFIX= $(EXTRA_CXXFLAGS)
 # Use our "custom linker" that calls gold if available
 COMMON_LDFLAGS := -B../tools/build_system -L/usr/local/lib -lpthread -ldl -lcurses -lm -lunwind -lz -L$(DEPS_DIR)/gcc-4.8.2-install/lib64
@@ -678,13 +687,16 @@ $(call make_search,dbg_%)
 debug_%: %.py pyston_debug $(RUN_DEPS)
 	$(GDB) $(GDB_CMDS) --args ./pyston_debug $(ARGS) $<
 $(call make_search,debug_%)
+ifneq ($(ENABLE_VALGRIND),0)
 memcheck_%: %.py pyston_dbg $(RUN_DEPS)
-	$(VALGRIND) --tool=memcheck --leak-check=no ./pyston_dbg $(ARGS) $<
+	$(VALGRIND) --tool=memcheck --leak-check=no --db-attach=yes ./pyston_dbg $(ARGS) $<
 $(call make_search,memcheck_%)
 memcheck_gdb_%: %.py pyston_dbg $(RUN_DEPS)
 	set +e; $(VALGRIND) -v -v -v -v -v --tool=memcheck --leak-check=no --track-origins=yes --vgdb=yes --vgdb-error=0 ./pyston_dbg $(ARGS) $< & export PID=$$! ; \
 	$(GDB) --ex "set confirm off" --ex "target remote | $(DEPS_DIR)/valgrind-3.9.0-install/bin/vgdb" --ex "continue" --ex "bt" ./pyston_dbg; kill -9 $$PID
 $(call make_search,memcheck_%)
+# "kill valgrind":
 kv:
 	ps aux | awk '/[v]algrind/ {print $$2}' | xargs kill -9; true
 memcheck_release_%: %.py pyston $(RUN_DEPS)
@@ -696,6 +708,7 @@ $(call make_search,memcheck_debug_%)
 memleaks_%: %.py pyston_dbg $(RUN_DEPS)
 	$(VALGRIND) --tool=memcheck --leak-check=full --leak-resolution=low --show-reachable=yes ./pyston_dbg $(ARGS) $<
 $(call make_search,memleaks_%)
+endif
 # gprof-based profiling:
 .PHONY: prof_% profile_%

--- a/src/codegen/opt/dead_allocs.cpp
+++ b/src/codegen/opt/dead_allocs.cpp
@@ -71,6 +71,12 @@ class DeadAllocsPass : public FunctionPass {
                    continue;
                }
+                if (PtrToIntInst *pti = dyn_cast<PtrToIntInst>(user)) {
+                    if (canBeRead(pti, chain))
+                        return true;
+                    continue;
+                }
                if (PHINode *phi = dyn_cast<PHINode>(user)) {
                    if (canBeRead(phi, chain))
                        return true;

--- a/src/codegen/patchpoints.cpp
+++ b/src/codegen/patchpoints.cpp
@@ -115,7 +115,7 @@ PatchpointSetupInfo* createGenericPatchpoint(CompiledFunction *parent_cf, TypeRe
 }
 PatchpointSetupInfo* createGetattrPatchpoint(CompiledFunction *parent_cf, TypeRecorder* type_recorder) {
-    return PatchpointSetupInfo::initialize(true, 1, 128, parent_cf, Getattr, type_recorder);
+    return PatchpointSetupInfo::initialize(true, 1, 144, parent_cf, Getattr, type_recorder);
 }
 PatchpointSetupInfo* createGetitemPatchpoint(CompiledFunction *parent_cf, TypeRecorder* type_recorder) {

--- a/src/core/common.h
+++ b/src/core/common.h
@@ -57,10 +57,6 @@ do { \
 #define OFFSET(cls, attr) ((char*)&(((cls*)0x01)->attr) - (char*)0x1)
-//#ifndef NDEBUG
-//#define VALGRIND
-//#endif
 // Allow using std::pair as keys in hashtables:
 namespace std {
    template <typename T1, typename T2> struct hash<pair<T1, T2> > {

--- a/src/gc/collector.cpp
+++ b/src/gc/collector.cpp
@@ -25,6 +25,10 @@
 #include "gc/heap.h"
 #include "gc/root_finder.h"
+#ifndef NVALGRIND
+#include "valgrind.h"
+#endif
 namespace pyston {
 namespace gc {
@@ -85,6 +89,12 @@ extern "C" kindid_t registerKind(const AllocationKind *kind) {
 }
 static void markPhase() {
+#ifndef NVALGRIND
+    // Have valgrind close its eyes while we do the conservative stack and data scanning,
+    // since we'll be looking at potentially-uninitialized values:
+    VALGRIND_DISABLE_ERROR_REPORTING;
+#endif
    TraceStack stack(roots);
    collectStackRoots(&stack);
@@ -123,6 +133,10 @@ static void markPhase() {
        gcf(&visitor, p);
    }
+#ifndef NVALGRIND
+    VALGRIND_ENABLE_ERROR_REPORTING;
+#endif
 }
 static void sweepPhase() {

--- a/src/gc/collector.h
+++ b/src/gc/collector.h
@@ -25,7 +25,11 @@ namespace gc {
 #define MARK_BIT 0x1
 inline GCObjectHeader* headerFromObject(void* obj) {
+#ifndef NVALGRIND
+    return static_cast<GCObjectHeader*>((void*)((char*)obj + 0));
+#else
    return static_cast<GCObjectHeader*>(obj);
+#endif
 }
 inline void setMark(GCObjectHeader *header) {

--- a/src/gc/gc_alloc.h
+++ b/src/gc/gc_alloc.h
@@ -20,6 +20,10 @@
 #include "gc/heap.h"
 #include "gc/collector.h"
+#ifndef NVALGRIND
+#include "valgrind.h"
+#endif
 namespace pyston {
 namespace gc {
@@ -30,7 +34,26 @@ inline void* gc_alloc(size_t bytes) {
        //runCollection();
    //}
+#ifndef NVALGRIND
+    // Adding a redzone will confuse the allocator, so disable it for now.
+#define REDZONE_SIZE 0
+    // This can also be set to "RUNNING_ON_VALGRIND", which will only turn on redzones when
+    // valgrind is actively running, but I think it's better to just always turn them on.
+    // They're broken and have 0 size anyway.
+#define ENABLE_REDZONES 1
+    void* r;
+    if (ENABLE_REDZONES) {
+        void* base = global_heap.alloc(bytes + REDZONE_SIZE * 2);
+        r = ((char*)base) + REDZONE_SIZE;
+        //printf("alloc base = %p\n", base);
+    } else {
+        r = global_heap.alloc(bytes);
+    }
+    VALGRIND_MALLOCLIKE_BLOCK(r, bytes, REDZONE_SIZE, false);
+#else
    void* r = global_heap.alloc(bytes);
+#endif
 #ifndef NDEBUG
    // I think I have a suspicion: the gc will see the constant and treat it as a
@@ -40,7 +63,7 @@ inline void* gc_alloc(size_t bytes) {
        //raise(SIGTRAP);
    //}
-    //if (VERBOSITY()) printf("Allocated: %p\n", r);
+    //if (VERBOSITY()) printf("Allocated %ld bytes at [%p, %p)\n", bytes, r, (char*)r + bytes);
 #endif
    return r;
@@ -48,12 +71,37 @@ inline void* gc_alloc(size_t bytes) {
 inline void* gc_realloc(void* ptr, size_t bytes) __attribute__((visibility("default")));
 inline void* gc_realloc(void* ptr, size_t bytes) {
+#ifndef NVALGRIND
+    void* rtn;
+    if (ENABLE_REDZONES) {
+        void* base = (char*)ptr - REDZONE_SIZE;
+        void* rtn_base = global_heap.realloc(base, bytes + 2 * REDZONE_SIZE);
+        rtn = (char*)rtn_base + REDZONE_SIZE;
+    } else {
+        rtn = global_heap.realloc(ptr, bytes);
+    }
+    VALGRIND_FREELIKE_BLOCK(ptr, REDZONE_SIZE);
+    VALGRIND_MALLOCLIKE_BLOCK(rtn, bytes, REDZONE_SIZE, true);
+    return rtn;
+#else
    return global_heap.realloc(ptr, bytes);
+#endif
 }
 inline void gc_free(void* ptr) __attribute__((visibility("default")));
 inline void gc_free(void* ptr) {
+#ifndef NVALGRIND
+    if (ENABLE_REDZONES) {
+        void* base = (char*)ptr - REDZONE_SIZE;
+        global_heap.free(base);
+    } else {
+        global_heap.free(ptr);
+    }
+    VALGRIND_FREELIKE_BLOCK(ptr, REDZONE_SIZE);
+#else
    global_heap.free(ptr);
+#endif
 }
 }

--- a/src/gc/heap.cpp
+++ b/src/gc/heap.cpp
@@ -19,7 +19,9 @@
 #include <stdint.h>
 #include <sys/mman.h>
+#ifndef NVALGRIND
 #include "valgrind.h"
+#endif
 #include "gc/gc_alloc.h"
@@ -121,8 +123,9 @@ static Block* alloc_block(uint64_t size, Block** prev) {
    rtn->prev = prev;
    rtn->next = NULL;
-#ifdef VALGRIND
+#ifndef NVALGRIND
-    VALGRIND_CREATE_MEMPOOL(rtn, 0, true);
+    // Not sure if this mempool stuff is better than the malloc-like interface:
+    //VALGRIND_CREATE_MEMPOOL(rtn, 0, true);
 #endif
    // Don't think I need to do this:
@@ -217,8 +220,8 @@ void* Heap::allocSmall(size_t rounded_size, Block** prev, Block** full_head) {
        assert(offset % rounded_size == 0);
 #endif
-#ifdef VALGRIND
+#ifndef NVALGRIND
-        VALGRIND_MEMPOOL_ALLOC(cur, rtn, rounded_size);
+        //VALGRIND_MEMPOOL_ALLOC(cur, rtn, rounded_size);
 #endif
        return rtn;
@@ -239,8 +242,8 @@ void _freeFrom(void* ptr, Block* b) {
    assert((b->isfree[bitmap_idx] & mask) == 0);
    b->isfree[bitmap_idx] ^= mask;
-#ifdef VALGRIND
+#ifndef NVALGRIND
-    VALGRIND_MEMPOOL_FREE(b, ptr);
+    //VALGRIND_MEMPOOL_FREE(b, ptr);
 #endif
 }
@@ -290,7 +293,13 @@ void* Heap::realloc(void* ptr, size_t bytes) {
    void* rtn = alloc(bytes);
+#ifndef NVALGRIND
+    VALGRIND_DISABLE_ERROR_REPORTING;
+    memcpy(rtn, ptr, std::min(bytes, size));
+    VALGRIND_ENABLE_ERROR_REPORTING;
+#else
    memcpy(rtn, ptr, std::min(bytes, size));
+#endif
    _freeFrom(ptr, b);
    return rtn;

--- a/src/gc/root_finder.cpp
+++ b/src/gc/root_finder.cpp
@@ -15,6 +15,7 @@
 #define UNW_LOCAL_ONLY
 #include <libunwind.h>
+#include <cstring>
 #include <setjmp.h>
 #include <cstdio>
 #include <cstdlib>
@@ -30,6 +31,10 @@
 #include "gc/heap.h"
 #include "gc/root_finder.h"
+#ifndef NVALGRIND
+#include "valgrind.h"
+#endif
 #ifndef LIBUNWIND_PYSTON_PATCH_VERSION
 #error "Please use a patched version of libunwind; see docs/INSTALLING.md"
 #elif LIBUNWIND_PYSTON_PATCH_VERSION != 0x01
@@ -63,14 +68,18 @@ void collectStackRoots(TraceStack *stack) {
    // collectStackRoots itself is allowed to save the callee-save registers
    // on its own stack.
    jmp_buf registers __attribute__((aligned(sizeof(void*))));
-#ifdef VALGRIND
-    memset(&registers, 0, sizeof(registers));
+#ifndef NVALGRIND
-    memset(&cursor, 0, sizeof(cursor));
+    if (RUNNING_ON_VALGRIND) {
-    memset(&uc, 0, sizeof(uc));
+        memset(&registers, 0, sizeof(registers));
-    memset(&ip, 0, sizeof(ip));
+        memset(&cursor, 0, sizeof(cursor));
-    memset(&sp, 0, sizeof(sp));
+        memset(&uc, 0, sizeof(uc));
-    memset(&bp, 0, sizeof(bp));
+        memset(&ip, 0, sizeof(ip));
+        memset(&sp, 0, sizeof(sp));
+        memset(&bp, 0, sizeof(bp));
+    }
 #endif
    setjmp(registers);
    assert(sizeof(registers) % 8 == 0);