Memory canary #2293

ChrisJefferson · 2018-03-24T10:12:04Z

This is an old piece of code I had, simplified and resurrected.

This PR is split into two pieces:

The first, bigger but simpler commit, introduces to GASMAN the concept of MptrEndBags, which denotes the end of the master pointers. Previously, the start of bag memory (called OldBags) was also the end of the master pointers. This lets the two move independantly. This is useful for part 2, but also might be useful for other things (I know I, and others, have considered changing GASMAN to rearrange objects for locality as it GCs. That would be much easier after this PR).

The second, crazier but more self contained part, adds a configure-time switch ( --enable-memory-checking), which adds to GAP the ability to find places where pointers into bag data are incorrectly accessed after a call to GASMAN.

The easiest way to see this in action is using the 3rd commit (which I will remove before merging!) which adds a simple but extremely hard to track down bug (if you didn't know where it was) in GASMAN_LIMITS(), where we try to write to a bag location after making a new plist (note the code also catches invalid reads as well as writes).

Do:

gap> GASMAN_LIMITS();
[ 131072, 2097152, 0 ]
gap> GASMAN_MEM_CHECK(1); # Enables checks
true
gap> GASMAN_LIMITS();
Segmentation fault

If you run GAP in a debugger, it will drop you onto the bad line.

You can start checks at startup by running GAP with --enableMemCheck. Note that GAP will take a LONG time to start (I'm talking a couple of days).

While this code has some slightly crazy memory things, I've tried to keep them tightly defined within #ifdef GAP_MEM_CHECK. I've also tried to keep the code short (there are a number of things one could do more cleverly, but the code is already quite scary).

codecov · 2018-03-25T11:40:12Z

Codecov Report

Merging #2293 into master will decrease coverage by 0.02%.
The diff coverage is 81.08%.

@@            Coverage Diff             @@
##           master    #2293      +/-   ##
==========================================
- Coverage   72.33%    72.3%   -0.03%     
==========================================
  Files         479      479              
  Lines      250960   251956     +996     
==========================================
+ Hits       181538   182185     +647     
- Misses      69422    69771     +349

Impacted Files	Coverage Δ
src/system.h	`66.66% <ø> (-33.34%)`	⬇️
lib/system.g	`67.13% <ø> (ø)`	⬆️
src/gasman.h	`90% <ø> (ø)`	⬆️
src/system.c	`68.62% <0%> (-0.51%)`	⬇️
src/gap.c	`80.41% <100%> (+0.03%)`	⬆️
lib/init.g	`71.54% <66.66%> (-0.04%)`	⬇️
src/saveload.c	`64.21% <66.66%> (ø)`	⬆️
src/gasman.c	`85.47% <95.83%> (+0.18%)`	⬆️
hpcgap/lib/hpc/queue.g	`66.4% <0%> (-3.2%)`	⬇️
... and 8 more

fingolfin · 2018-03-25T12:00:25Z

The first commit makes perfect sense to me, and I'd merge it instantly if it was a separate PR -- in case the second commit takes longer to review, it might be worthwhile to get the first one in ASAP, to avoid bitrot.

fingolfin

Overall this looks sane and very useful to me. I have not yet actually tried it yet, though. Only have minor code nitpicks.

fingolfin · 2018-03-25T12:01:56Z

configure.ac

+    [AS_HELP_STRING([--enable-memory-checking], [enable memory checking])],
+    [AC_DEFINE([GAP_MEM_CHECK], [1], [define if building with memory checking])],
+    [enable_memory_checking=no]
+    )


This option is not available in HPC-GAP, is it? Both because it is GASMAN specific, and because it does not perform any thread locking.

It would be kind of nice if this gave an error here if HPCGAP is enabled.

And perhaps also add in "WARNING: this makes GAP super slow" ?

Another thing: you already put in some kind of valgrind based memory checking in the past. Looking at this configure option, it is not clear which of the various memory debugging techniques this refers to...

I hope to tidy up and document the valgrind stuff at some point. I have added a message on GAP startup saying it takes a long time to start up.

fingolfin · 2018-03-25T12:02:10Z

lib/init.g

@@ -560,6 +560,9 @@ BindGlobal( "ShowKernelInformation", function()
  if GAPInfo.KernelInfo.KernelDebug then
    Add( config, "KernelDebug" );
  fi;
+  if GAPInfo.KernelInfo.MemCheck then
+    Add(config, "MemCheck (EXPERIMENTAL)");


Why the "EXPERIMENTAL"? When would it be removed, if ever?

Experimental might be a bad word. If we later changed GCs to boehm it would get removed, and I could imagine there could be later changes which significantly change the behaviour. Also I don't really want people to start using this as their standard GAP, because it is slower and more limited in a number of ways (doesn't give memory back to the OS for example).

fingolfin · 2018-03-25T12:03:09Z

src/gap.c

+Obj FuncGASMAN_MEM_CHECK(Obj self, Obj newval)
+{
+    EnableMemCheck = INT_INTOBJ(newval);
+    return True;


Why does this return true, instead of 0?

fingolfin · 2018-03-25T12:05:50Z

src/gasman.c

+ * a reference to the internals of a GAP object are kept across
+ * a garbage collection (which moves GAP objects around).
+ *
+ * MultiHeap provides a method of detecting such problems, at


This is the first time the word "MultiHeap" appears, which I find confusing. Either get rid of this; or else "define" it before use.

I will change this. I used to call this MultiHeap.

fingolfin · 2018-03-25T12:06:44Z

src/gasman.c

+ *
+ * MultiHeap provides a method of detecting such problems, at
+ * the cost of GREATLY decreased performance (Starting GAP in
+ * MultiHeap mode takes hours, rather than seconds).


In the PR, you say startup takes "days", here "hours" -- which is it? On what kind of system (clearly I can find old systems where it's much slower, so here really some info on the machine would be useful to gauge the severity of this slowdown)

Depends on the machine. about 18 hours on Linux, 4 days on the linux subsystem on windows, haven't timed it on a mac yet. I'll just say 'days'.

fingolfin · 2018-03-25T12:12:22Z

src/gasman.c

+        return;
+
+    Int newBase = oldBase + 1;
+    // 0 is special, skip it when cycling


This explanation seems to start in the middle of a thought, i.e. the reader has to already understand what's going on generally to make sense of it. Perhaps this is clearer?

// cycle to next memory area, wrapping around at end, and skipping area 0 (which is special) Int newBase = oldBase + 1; if (newBase >= GetMembufCount()) newBase = 1;

As you can see, I also use >= instead of == for paranoia -- the interface does not seem to guarantee that the return value of GetMembufCount() is constant. This could be "fixed" by replacing GetMembufCount() by a global constant (enum).

fingolfin · 2018-03-25T12:12:41Z

src/gasman.c

+
+    MoveBagMemory(GetMembuf(oldBase), GetMembuf(newBase));
+
+    // Block access to old memory, provide access to new memory.


consistency: "Block" -> "block", remove trailing full stop

Also, perhaps "provide" -> "enable" ?

fingolfin · 2018-03-25T12:16:42Z

src/system.c

+ *
+ * The following code is used by GAP_MEM_CHECK support, which is
+ * documented in gasman.c
+ */


Perhaps move this comment before the #if/#error, as it is already relevant to those?

fingolfin · 2018-03-25T12:18:41Z

src/system.c

+        }
+    }
+
+    // Make the first membufs the earlier in memory. This is where the


Is that valid english? In any case, I find it confusing, how about this alternative instead? "Sort the memory buffers ascending by their address, so that the first buffer comes first in memory. This is the one where the master pointers will live".

fingolfin · 2018-03-25T12:21:05Z

src/system.c

-  { 0  , "quitonbreak", toggle, &SyQuitOnBreak, 0}, /* Quit GAP if we enter the break loop */
-  { 0, "", 0, 0, 0}};
+    { 0, "", 0, 0, 0 }
+};


I understand that running git clang-format is quite convenient, but here it just makes reviewing this part of the PR hard (or impossible). I also find the formatted output quite ugly in parts, du to the way it wraps. So perhaps first have a commit which reformats this struct (ideally with some manual movement of comments, to make it less ugly, but that's optional); and then modify it in a commit after that?

ChrisJefferson · 2018-03-25T18:50:52Z

The only missing part from the PR now (I believe) is that it does not forbid building with HPC-GAP, but the better fix there is I believe to block use with BOEHM_GC, which is related to another open patch.

fingolfin · 2018-03-27T08:44:07Z

This needs to be rebased now. Also, that other PR which introduced USE_GASMAN and renamed BOEHM_GC to USE_BOEHM_GC has landed.

fingolfin · 2018-03-27T08:48:01Z

src/gasman.c

+
+UInt   GetMembufCount(void);
+void * GetMembuf(UInt i);
+UInt GetMembufSize(void);


I am a bit wary about declaring external function prototypes outside a .h file -- this way, it is very easy to change the function implementation, while forgetting to adjust the prototype here, and there will be no warning. So I'd feel better if these were declared in e.g. system.h, inside an #ifdef GAP_MEM_CHECK / #endif block.

This allows for a seperation between the end of the master pointers, and the beginning of the bag data area

fingolfin

LGTM. Nice work!

This adds a new configure option to GAP which enables an extremely slow debug mode, which can find problems where references to memory are incorrectly kept across garbage collections.

ChrisJefferson force-pushed the new-canary-2 branch 2 times, most recently from 1832e2d to fb085fa Compare March 24, 2018 10:24

ChrisJefferson mentioned this pull request Mar 24, 2018

Avoid holding reference to a GAP bag when NewBag is called. #2294

Merged

ChrisJefferson force-pushed the new-canary-2 branch 2 times, most recently from 8088880 to 4d0c41d Compare March 25, 2018 11:39

fingolfin reviewed Mar 25, 2018

View reviewed changes

ChrisJefferson force-pushed the new-canary-2 branch 2 times, most recently from 485d291 to 1c33c19 Compare March 25, 2018 18:50

ChrisJefferson force-pushed the new-canary-2 branch from 1c33c19 to 928e4ab Compare March 25, 2018 19:00

ChrisJefferson mentioned this pull request Mar 26, 2018

GASMAN problem in BIPART_LEFT_BLOCKS / BIPART_RIGHT_BLOCKS semigroups/Semigroups#469

Closed

fingolfin reviewed Mar 27, 2018

View reviewed changes

Add MptrEndBags

0a2b06e

This allows for a seperation between the end of the master pointers, and the beginning of the bag data area

ChrisJefferson force-pushed the new-canary-2 branch from 928e4ab to fa302e7 Compare March 27, 2018 12:52

fingolfin approved these changes Mar 27, 2018

View reviewed changes

Add memory debugging

9550010

This adds a new configure option to GAP which enables an extremely slow debug mode, which can find problems where references to memory are incorrectly kept across garbage collections.

ChrisJefferson force-pushed the new-canary-2 branch from fa302e7 to 9550010 Compare March 27, 2018 14:26

fingolfin merged commit 562896c into gap-system:master Mar 28, 2018

ChrisJefferson deleted the new-canary-2 branch March 28, 2018 22:23

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel labels May 2, 2018

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory canary #2293

Memory canary #2293

ChrisJefferson commented Mar 24, 2018

codecov bot commented Mar 25, 2018 •

edited

Loading

fingolfin commented Mar 25, 2018

fingolfin left a comment

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

ChrisJefferson Mar 25, 2018

fingolfin Mar 25, 2018

ChrisJefferson Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

ChrisJefferson Mar 25, 2018

fingolfin Mar 25, 2018

ChrisJefferson Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

fingolfin Mar 25, 2018

ChrisJefferson commented Mar 25, 2018

fingolfin commented Mar 27, 2018

fingolfin Mar 27, 2018

fingolfin left a comment


		MoveBagMemory(GetMembuf(oldBase), GetMembuf(newBase));

		// Block access to old memory, provide access to new memory.

Memory canary #2293

Memory canary #2293

Conversation

ChrisJefferson commented Mar 24, 2018

codecov bot commented Mar 25, 2018 • edited Loading

Codecov Report

fingolfin commented Mar 25, 2018

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson commented Mar 25, 2018

fingolfin commented Mar 27, 2018

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2018 •

edited

Loading