memory: Add memory-debug scribbling when tcmalloc is disabled and not compiled for optimization by jmarantz · Pull Request #5450 · envoyproxy/envoy

jmarantz · 2019-01-02T15:05:38Z

Description: This is an alternate approach to #5424 which is broken due to bazel's inability to do a prioritized select(). In this version, we just slip in some alternate operator new/delete overrides whenever we are doing a debug build (#ifndef NDEBUG) without tcmalloc. The memory-scribbling code is new to Envoy but has a lot of mileage on it it in its original location: https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/kernel/base/mem_debug.cc
Risk Level: low
Testing: //test/... with --compilation_mode=dbg and --define=tcmalloc=disabled , but relying on CI to see what happens in other configurations, thus this is WiP.
Docs Changes: n/a
Release Notes: n/a

…bug mode. Either way, bazel seems to be a problem. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-02T16:28:33Z

still having bazel issues turning this code off with tsan/asan. closing for now.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-03T20:34:54Z

re-opening to see if I can get this to pass CI. Still not ready for any sort of review.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

source/common/memory/mem_debug.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz

this is still not really ready for review but thanks for the comments :)

However I think I am heading in this direction rather than tcmalloc's debugallocator, which I can't get to work with bazel.

source/common/memory/mem_debug.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…shSet. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…on* classes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-04T22:35:01Z

OK I think this is ready for review now. #5424 is looking very difficult due to bazel limitations.

…ging is enabled. Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway · 2019-01-04T23:44:49Z

source/common/memory/align.h

+inline uint64_t align(uint64_t size, uint64_t alignment) {
+  // Check that alignment is a power of 2:
+  // http://www.graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
+  assert((alignment > 0) && ((alignment & (alignment - 1)) == 0));


ASSERT or RELEASE_ASSERT?

I was trying to avoid assertion layers that might allocate memory in the context of a utility designed to help with memory allocation. Although actually the right thing here is to use static_assert, which I can do with a little template magic.

ggreenway · 2019-01-04T23:48:12Z

source/common/memory/debug.cc

+
+#include "common/memory/align.h"
+
+static std::atomic<int64_t> bytes_allocated(0);


Why signed?

I was thinking that a negative value was possible if a block of memory was subtly corrupted before it was freed. However it's better to assert when that happens than let this go negative. I'll fix.

ggreenway · 2019-01-04T23:48:38Z

source/common/memory/debug.cc

+} // namespace Memory
+} // namespace Envoy
+
+#ifdef MEMORY_DEBUG_ENABLED


ENVOY_MEMORY_DEBUG_ENABLED?

ggreenway · 2019-01-04T23:49:17Z

source/common/memory/debug.cc

+namespace {
+
+constexpr uint32_t LiveMarker1 = 0xfeedface;         // first 4 bytes after alloc
+constexpr uint64_t LiveMarker2 = 0xfeedfacefeedface; // first 4 bytes after alloc


Comment says 4 bytes, but this is 8 bytes long

ggreenway · 2019-01-04T23:49:55Z

source/common/memory/debug.cc

+
+// Writes scribble_word over the block of memory starting at ptr and extending
+// size bytes.
+inline void scribble(void* ptr, uint64_t rounded_size, uint64_t scribble_word) {


I don't think inline does anything here; omit?

done; I was thinking I might be able to convince the compiler to optimize out the assert on alignment; but it's likely not worth it, and the compiler may figure out how to do that anyway.

ggreenway · 2019-01-04T23:52:12Z

source/common/memory/debug.cc

+// malloced memory is 0 get data that, when interpreted as pointers, will SEGV,
+// and that will be easily seen in the debugger (0xfeedface pattern).
+void* debugMalloc(uint64_t size) {
+  assert(size <= 0xffffffff); // For now we store the original size in a uint32_t.


This makes me nervous.

It wouldn't be hard to use all 8 bytes in the marker to store the size. But I'm curious why it makes you nervous? Do you think -- especially in a testing/debugging contest, that we are likely to see 4G requests to malloc? I would imagine that would not behave well, and it might not be a bad idea to get an assert fail while debugging or testing to become aware of this.

If that's OK with you I'll document the assert better. If you'd prefer to allow >=4G allocations I can change it.

ggreenway · 2019-01-04T23:54:00Z

source/common/memory/debug.cc

+  assert(marker != NULL);
+  marker[0] = LiveMarker1;
+  marker[1] = size;
+  uint32_t* ret = marker + 2;


This is labeled ret, but it is not the returned value.

Actually I made it the returned value, but also renamed to 'payload'; wdyt?

source/common/memory/debug.h

ggreenway · 2019-01-05T00:02:56Z

It's disappointing that there's not a good way to use the debug tcmalloc. But given that isn't possible, I am in favor of this approach to improve memory checking.

PiotrSikora · 2019-01-05T02:15:06Z

source/common/memory/debug.cc

+
+namespace {
+
+constexpr uint32_t LiveMarker1 = 0xfeedface;         // first 4 bytes after alloc


Those names should all be uppercase.

From https://github.com/envoyproxy/envoy/blob/master/STYLE.md

The Google C++ style guide points out that constant vars should be named kConstantVar. In the Envoy codebase we use ConstantVar or CONSTANT_VAR. If you pick CONSTANT_VAR, please be certain the name is globally significant to avoid potential conflicts with #defines, which are not namespace-scoped, and may appear in externally controlled header files.

PiotrSikora · 2019-01-05T02:16:17Z

source/common/memory/debug.cc

+  assert(marker != NULL);
+  marker[0] = LiveMarker1;
+  marker[1] = size;
+  uint32_t* ret = marker + 2;


s/2/sizeof(Overhead)/sizeof(uint32_t)/

OK though I'm not sure if that's easier to read. The '2' is the logical progression from assigning marker[0] and then marker[1]; now we are working on what begins at index 2, which is the payload.

PiotrSikora · 2019-01-05T02:19:45Z

source/common/memory/debug.cc

+
+} // namespace
+
+void* operator new(uint64_t size) { return debugMalloc(size); }


Shouldn't this be size_t?

urr, yes :)

PiotrSikora · 2019-01-05T02:23:47Z

source/common/memory/debug.cc

+// and that will be easily seen in the debugger (0xfeedface pattern).
+void* debugMalloc(uint64_t size) {
+  assert(size <= 0xffffffff); // For now we store the original size in a uint32_t.
+  uint64_t rounded = Envoy::Memory::align(size, Overhead);


Note: this can now return 0 (it couldn't in the original code), so you might want to skip calls to malloc and scribble in that case.

I still need to add the overhead so we can do the inverse accounting on free. And why special-case the call to scribble, which should happily does nothing in the degenerate case?

PiotrSikora · 2019-01-05T02:26:20Z

source/common/memory/debug.cc

+    uint64_t rounded = Envoy::Memory::align(size, Overhead);
+    bytes_allocated -= rounded;
+    scribble(ptr, rounded, DeadMarker2);
+    assert(LiveMarker1 == marker[0]);


Could you move this check right after assigning marker and before accessing marker[1]? Since if this is wrong, then so will be marker[1].

I moved it to after the assignment of marker. I'm not sure what you meant in the second half of your suggestion.

PiotrSikora · 2019-01-05T02:38:27Z

source/common/memory/debug.cc

+
+constexpr uint32_t LiveMarker1 = 0xfeedface;         // first 4 bytes after alloc
+constexpr uint64_t LiveMarker2 = 0xfeedfacefeedface; // first 4 bytes after alloc
+constexpr uint32_t DeadMarker1 = 0xabacabff;         // first 4 bytes after free


I cannot comment on the original thread, so reviving it here.

What's the value of having 2 different dead markers? Is that useful in practice?

It has been occasionally useful, yes, to to distinguish between blobs of memories that are allocated but initialized vs freed.

note also that tcmalloc's debugallocation does the same thing (with different patterns).

Yes, the usefulness of Live and Dead is quite obvious... but my question was about having different Marker1 and Marker2 (i.e. marking first 4 bytes of the freshly allocated / freed memory differently from the rest).

It must have been in some distant debug scenario this distinction became interesting. But I can't pull it out of history, and so I'll just make there be one dead marker.

PiotrSikora · 2019-01-05T02:39:44Z

source/common/memory/debug.cc

+// and that will be easily seen in the debugger (0xfeedface pattern).
+void* debugMalloc(uint64_t size) {
+  assert(size <= 0xffffffff); // For now we store the original size in a uint32_t.
+  uint64_t rounded = Envoy::Memory::align(size, Overhead);


Nit: please rename rounded to aligned or aligned_size.

PiotrSikora · 2019-01-05T02:40:37Z

source/common/memory/debug.cc

+
+#include "common/memory/align.h"
+
+static std::atomic<int64_t> bytes_allocated(0);


Could we also track allocated_bytes_with_overhead to account for alignment and allocation headers?

Who would consume that?

The main reason I'm tracking this at all is so that the memory-constraining tests can run in debug mode.

PiotrSikora · 2019-01-05T02:44:01Z

source/common/memory/stats.cc

 namespace Memory {

-uint64_t Stats::totalCurrentlyAllocated() { return 0; }
+uint64_t Stats::totalCurrentlyAllocated() { return Debug::bytesUsed(); }


I'd slightly prefer to have this whole block duplicated, so that we have:

#ifdef TCMALLOC ... #elif defined(MEMORY_DEBUG_ENABLED) ... #else ... #endif

instead of adding code to the build without debug memory.

Good point...I reorganized it a bit; ptal.

PiotrSikora · 2019-01-05T13:46:26Z

source/common/memory/debug.h

+
+// We don't run memory debugging for optimizd builds to avoid impacting
+// production performance.
+#ifndef NDEBUG


I think, I'd prefer if users had to opt-in into this via --define debug_memory=enabled or something like that, instead of this being automatically injected to all non-optimized / non-sanitized builds with --define tcmalloc=disabled.

This is a healthy discussion to have...my motivation was to help people find memory bugs when they aren't looking for them. If they are looking for them they can use valgrind which is superior but onerous.

However, as I explore using this in different compile modes, I am also running into trouble with what looks to me to be inconsistent usage of operator new/delete in protobufs. This results in segv before main in these contexts, whereas in others it works great.

At least until all the build/run modes are working, I think this suggestion makes sense and I'll have this off-by-default. I re-worked the build files around this strategy. WDYT?

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-06T15:58:50Z

OK; I'm back to wanting to use the scribbling support built into tcmalloc :)

The reason I had backed off of that approach before is that I couldn't figure out how to have it just automatically work for debug builds due to the limitations of bazel select. But given that in this approach we are going to force the user to use bazel config overrides, we might as well use the support built into tcmalloc, which doesn't have this operator new/delete override messiness with protobufs.

Sorry about the churn, and thanks for all the comments along the way.

jmarantz

Responding to these comments; though I've closed the PR.

jmarantz · 2019-01-05T15:50:07Z

source/common/memory/align.h

+inline uint64_t align(uint64_t size, uint64_t alignment) {
+  // Check that alignment is a power of 2:
+  // http://www.graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
+  assert((alignment > 0) && ((alignment & (alignment - 1)) == 0));


I was trying to avoid assertion layers that might allocate memory in the context of a utility designed to help with memory allocation. Although actually the right thing here is to use static_assert, which I can do with a little template magic.

jmarantz · 2019-01-05T15:52:50Z

source/common/memory/debug.cc

+
+#include "common/memory/align.h"
+
+static std::atomic<int64_t> bytes_allocated(0);


I was thinking that a negative value was possible if a block of memory was subtly corrupted before it was freed. However it's better to assert when that happens than let this go negative. I'll fix.

jmarantz · 2019-01-05T15:54:33Z

source/common/memory/debug.cc

+
+#include "common/memory/align.h"
+
+static std::atomic<int64_t> bytes_allocated(0);


Who would consume that?

The main reason I'm tracking this at all is so that the memory-constraining tests can run in debug mode.

jmarantz · 2019-01-05T15:55:48Z

source/common/memory/debug.cc

+} // namespace Memory
+} // namespace Envoy
+
+#ifdef MEMORY_DEBUG_ENABLED


jmarantz · 2019-01-05T15:56:49Z

source/common/memory/debug.cc

+
+namespace {
+
+constexpr uint32_t LiveMarker1 = 0xfeedface;         // first 4 bytes after alloc


From https://github.com/envoyproxy/envoy/blob/master/STYLE.md

The Google C++ style guide points out that constant vars should be named kConstantVar. In the Envoy codebase we use ConstantVar or CONSTANT_VAR. If you pick CONSTANT_VAR, please be certain the name is globally significant to avoid potential conflicts with #defines, which are not namespace-scoped, and may appear in externally controlled header files.

jmarantz · 2019-01-05T19:13:30Z

source/common/memory/debug.cc

+  assert(marker != NULL);
+  marker[0] = LiveMarker1;
+  marker[1] = size;
+  uint32_t* ret = marker + 2;


OK though I'm not sure if that's easier to read. The '2' is the logical progression from assigning marker[0] and then marker[1]; now we are working on what begins at index 2, which is the payload.

jmarantz · 2019-01-05T19:17:30Z

source/common/memory/debug.cc

+    uint64_t rounded = Envoy::Memory::align(size, Overhead);
+    bytes_allocated -= rounded;
+    scribble(ptr, rounded, DeadMarker2);
+    assert(LiveMarker1 == marker[0]);


I moved it to after the assignment of marker. I'm not sure what you meant in the second half of your suggestion.

jmarantz · 2019-01-05T19:21:57Z

source/common/memory/stats.cc

 namespace Memory {

-uint64_t Stats::totalCurrentlyAllocated() { return 0; }
+uint64_t Stats::totalCurrentlyAllocated() { return Debug::bytesUsed(); }


Good point...I reorganized it a bit; ptal.

jmarantz · 2019-01-05T19:26:28Z

source/common/memory/debug.cc

+// malloced memory is 0 get data that, when interpreted as pointers, will SEGV,
+// and that will be easily seen in the debugger (0xfeedface pattern).
+void* debugMalloc(uint64_t size) {
+  assert(size <= 0xffffffff); // For now we store the original size in a uint32_t.


It wouldn't be hard to use all 8 bytes in the marker to store the size. But I'm curious why it makes you nervous? Do you think -- especially in a testing/debugging contest, that we are likely to see 4G requests to malloc? I would imagine that would not behave well, and it might not be a bad idea to get an assert fail while debugging or testing to become aware of this.

If that's OK with you I'll document the assert better. If you'd prefer to allow >=4G allocations I can change it.

jmarantz · 2019-01-05T20:22:35Z

source/common/memory/debug.cc

+
+constexpr uint32_t LiveMarker1 = 0xfeedface;         // first 4 bytes after alloc
+constexpr uint64_t LiveMarker2 = 0xfeedfacefeedface; // first 4 bytes after alloc
+constexpr uint32_t DeadMarker1 = 0xabacabff;         // first 4 bytes after free


It must have been in some distant debug scenario this distinction became interesting. But I can't pull it out of history, and so I'll just make there be one dead marker.

jmarantz added 6 commits December 30, 2018 19:12

Copy mem_debug impl from pagespeed as an alternative to tcmalloc's de…

a5d2e9a

…bug mode. Either way, bazel seems to be a problem. Signed-off-by: Joshua Marantz <jmarantz@google.com>

add mem-debug files.

c5ab27c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

attempt to use tcmalloc hook to do allocation scribbling; doesn't work.

f608251

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove broken attempt to get alloc scribbling working with tcmalloc.

68668a6

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove superfluous changes

cf1ff7e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove stale comment.

c20d7cd

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz closed this Jan 2, 2019

jmarantz mentioned this pull request Jan 2, 2019

debug: Link in the debug version of tcmalloc for debug builds. #5424

Merged

Hack in a maze of twisty passages to get -D setting through blaze.

d2d196b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz reopened this Jan 3, 2019

jmarantz added 2 commits January 3, 2019 15:36

format

324d7c2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into mem_debug

8f4dd93

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora suggested changes Jan 4, 2019

View reviewed changes

jmarantz added 3 commits January 4, 2019 12:09

Clean up coding and hook up allocated-bytes count into memory/stats.cc.

a3c0b54

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into mem_debug

a37ebbb

Signed-off-by: Joshua Marantz <jmarantz@google.com>

formatting

bd0d593

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Jan 4, 2019

View reviewed changes

jmarantz added 7 commits January 4, 2019 13:43

Added a few signed/unsigned cleanups, assert <4g allocations, etc.

32b8d0d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Clean up namespaces, filenames, etc. Share align() with BlockMemoryHa…

3fe801c

…shSet. Signed-off-by: Joshua Marantz <jmarantz@google.com>

use raw 'noexcept'.

6282866

Signed-off-by: Joshua Marantz <jmarantz@google.com>

More comment cleanups.

4022204

Signed-off-by: Joshua Marantz <jmarantz@google.com>

fix build path

a5303ef

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove some stray paths -- I am linking into main.cc but the MainComm…

efdfc93

…on* classes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

More cleanups & add test.

0da923e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WiP/RFC: memory: Add memory-debug scribbling when tcmalloc is disabled~~ memory: Add memory-debug scribbling when tcmalloc is disabled and not compiled for optimization Jan 4, 2019

Use the exported MEMORY_DEBUG_ENABLED for determining if memory debug…

a612677

…ging is enabled. Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway requested changes Jan 5, 2019

View reviewed changes

ggreenway self-assigned this Jan 5, 2019

PiotrSikora suggested changes Jan 5, 2019

View reviewed changes

PiotrSikora reviewed Jan 5, 2019

View reviewed changes

jmarantz added 2 commits January 6, 2019 10:16

Address review comments.

d532232

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add BUILD file for new test.

664c08a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz closed this Jan 6, 2019

jmarantz commented Jan 6, 2019

View reviewed changes


		#include "common/memory/align.h"

		static std::atomic<int64_t> bytes_allocated(0);


		namespace {

		constexpr uint32_t LiveMarker1 = 0xfeedface; // first 4 bytes after alloc


		} // namespace

		void* operator new(uint64_t size) { return debugMalloc(size); }

Conversation

jmarantz commented Jan 2, 2019

Uh oh!

jmarantz commented Jan 2, 2019

Uh oh!

jmarantz commented Jan 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Jan 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway commented Jan 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment