580603 - adversarial mmgc: dispersive w.r.t. address space

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P3)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

For testing, we need at least one platform (and preferably several or all) that supports a mmgc mode that allocates pages in a manner that distributes them widely across the address space, in order to test that our code will handle such conditions reasonably if it encounters them in the wild.

I encountered this requirement while working on bug 445780.

I have come up with a technique that seems to work reasonably well on OS X and is probably portable to any host that has virtual memory support.  Patch and req for feedback will be coming soon.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: adds dispersive mode for emulating semi-adversarial env.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: selftest exercising the adversarial mode.

Note that I did have to pump up the fill_sz (making the adversary even
more ridiculous than you could really deal with in practical testing)
and ratchet down the heapLimit to a value that is not quite as absurd
as the current kDefaultHeapLimit (which is effectively infinite).

(We should fix the selftest infrastructure to allow one to override
those sorts of configuration settings; see also Bug 546677.)

Comment 3

[Tommy Reilly]

Comment on attachment 459293 [details] [diff] [review]
adds dispersive mode for emulating semi-adversarial env.

Cool technique.  If the intention is to land I think ifdef DEBUG would be appropriate.   Also it seems like this would probably crash in a 32 bit system.

Comment 4

[Steven Johnson]

(In reply to comment #3)
> Cool technique.  If the intention is to land I think ifdef DEBUG would be
> appropriate. 

Agree about ifdef'ing, but probably with a separate flag (MMGC_ADVERSIAL or whatnot) that defaults to on for DEBUG.... (1) better hygiene and (2) might be useful to be able to flip it on in nondebug builds.

>  Also it seems like this would probably crash in a 32 bit system.

Why is that?

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #3)
> Comment on attachment 459293 [details] [diff] [review]
> adds dispersive mode for emulating semi-adversarial env.
> 
> Cool technique.  If the intention is to land I think ifdef DEBUG would be
> appropriate.   

I would indeed like to land some variant of this, after I clean it up a touch.

On ifdef DEBUG: Its a selftest; you think it would be better to skip the test entirely if we run this particular selftest in release mode?  (I know its not guaranteed to do anything bad, but it seems to me like it would be better to run through the exercise, even in release mode.)

> Also it seems like this would probably crash in a 32 bit system.

Very good point.  I cannot remember if I tried it out in 32-bit mode; it might crash there, especially since I am not consistently checking for a NULL return value from ReserveSomeRegion, and I also am not checking whether mem0 and mem1 are NULL, but it also might succeed, since reserveSomeRegionDispersively backs off and reattempts the allocation (after releasing mem0 and mem1) if the first attempt failed.

I'll clean it up a bit, double-check the behavior on 32-bit systems (and maybe add a conditional code that uses a smaller fill_sz on 32 bit systems, in order to still attempt to get the effect there.)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #5)
> (In reply to comment #3)
> > Comment on attachment 459293 [details] [diff] [review] [details]
> > adds dispersive mode for emulating semi-adversarial env.
> > 
> > Cool technique.  If the intention is to land I think ifdef DEBUG would be
> > appropriate.   
> 
> I would indeed like to land some variant of this, after I clean it up a touch.
> 
> On ifdef DEBUG: Its a selftest; you think it would be better to skip the test
> entirely if we run this particular selftest in release mode?  (I know its not
> guaranteed to do anything bad, but it seems to me like it would be better to
> run through the exercise, even in release mode.)

Oh, sorry, never mind; I forgot that this was the change to GCHeap, not the selftest.  Yes yes, it definitely needs some preprocessor symbol around it making it clear that this is not some sort of feature that we're exporting from GCHeap.

I like Steven's proposed middle ground of a new flag (MMGC_ADVERSARIAL?  MMGC_ADVERSE?) that gets flipped on by DEBUG mode.

Comment 7

[Tommy Reilly]

(In reply to comment #6)
> I like Steven's proposed middle ground of a new flag (MMGC_ADVERSARIAL? 
> MMGC_ADVERSE?) that gets flipped on by DEBUG mode.

+1

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #7)
> (In reply to comment #6)
> > I like Steven's proposed middle ground of a new flag (MMGC_ADVERSARIAL? 
> > MMGC_ADVERSE?) that gets flipped on by DEBUG mode.
> 
> +1

On second thought, having DEBUG mode imply ADVERSARIAL may be a bad idea, since the differences in memory addresses that would probably introduce further differences between release and debug modes of the virtual machine.

Could we have the setup be that in the acceptance test infrastructure, DEBUG and ADVERSARIAL get turned on for the debug builds, but the --enable-debug flag does not imply ADVERSARIAL?

Comment 9

[Steven Johnson]

How about:
-- code is compiled in (or not) by MMGC_ADVERSARIAL 
-- MMGC_ADVERSARIAL is enabled (by default) in Debug builds
-- adversarial is enabled (or not) at runtime, so builds with it present still have to opt-in for adversarial mode

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #2)
> (We should fix the selftest infrastructure to allow one to override
> those sorts of configuration settings; see also Bug 546677.)

Actually I now realize I can get the effect I want by imperatively modifying the already allocated heap's GCHeapConfig.  So Bug 546677 does not need to be resolved in order to write this particular selftest.

Comment 11

[Tommy Reilly]

Comment on attachment 459293 [details] [diff] [review]
adds dispersive mode for emulating semi-adversarial env.

My preference would be for adversarial code to be in an ifdef only defined for special DEBUG builds.

Comment 12

[Tommy Reilly]

Comment on attachment 459298 [details] [diff] [review]
selftest exercising the adversarial mode.

This relies on the previous patch I didn't like.   Also you can change heapLimit in the test itself instead of in the GCHeapConfig structure (ie heap->config.heapLimit is a live value that can be changed on the fly).

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: adds dispersive mode for emulating semi-adversarial env.

Revised to put newly added code under DEBUG preprocessor guard.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: selftest exercising the adversarial mode.

Revised to set heap limit dynamically rather than patching the source.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: generated cpp for selftest and necessary manifest update.

(the generated code and necessary xplat manifest changes; putting it up on ticket for ease of testing the earlier patch, if that's your thing.  Xcode and Visual Studio project updates soon to follow.)

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: xcode project file changes to add selftest

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: visual studio 2008 project file changes to add selftest

Comment 18

[Tommy Reilly]

Comment on attachment 486104 [details] [diff] [review]
generated cpp for selftest and necessary manifest update.

I don't think generated code and project file updates need review.

Comment 19

[Tommy Reilly]

Comment on attachment 486106 [details] [diff] [review]
visual studio 2008 project file changes to add selftest

noisy diff, I think folks usually revert and hand apply the parts that matter when doing visual studio project file updates.

Comment 20

[Lars T Hansen]

I'm unlikely to get to these patches this week.

Comment 21

[Lars T Hansen]

Comment on attachment 486103 [details] [diff] [review]
selftest exercising the adversarial mode.

Style:

fflush(NULL) much preferred to fflush(0).

"avoid waste time" presumably better as "avoid wasting time"?

Arguably max_addr is miscomputed because it tracks the max starting address of a block, not the max ending address of a block?

The computation of size in do_allocs is extremely mysterious, the kind of phrase about which songs will be written in the future.  Some of those songs will not be kind - a line of two of documentation may be helpful here.

Comment 22

[Lars T Hansen]

Comment on attachment 486106 [details] [diff] [review]
visual studio 2008 project file changes to add selftest

Rubber stamp.

Comment 23

[Lars T Hansen]

As a general comment, the last three patches here were probably not required for review.

Comment 24

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #23)
> As a general comment, the last three patches here were probably not required
> for review.

Okay.  I'll probably continue uploading such patches to tickets (at least when the patches aren't grossly large) since it aids use of the qimportbz hg extension, but I'll stop putting the review requests on them.

Comment 25

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #19)
> Comment on attachment 486106 [details] [diff] [review]
> visual studio 2008 project file changes to add selftest
> 
> noisy diff, I think folks usually revert and hand apply the parts that matter
> when doing visual studio project file updates.

I'll try to do the hand application suggested, though I worry when it comes to screwing around with project files that were generated by the IDEs.

(i do find it ironic that comment 18 and comment 23 said that typically these things aren't put up for review, but there was useful feedback from Tommy in his review... but I do see how in general such reviews are not going to be necessary.)

Comment 26

[Felix S. Klock II [:pnkfelix, :fklock]]

attachment 486101 [details] [diff] [review] : TR changeset - 5529:cd45b9440ce4
  http://hg.mozilla.org/tamarin-redux/rev/cd45b9440ce4

attachment 486103 [details] [diff] [review], revised wrt comment 21 : TR changeset - 5530:f562763e596e
  http://hg.mozilla.org/tamarin-redux/rev/f562763e596e

attachment 486104 [details] [diff] [review],
attachment 486105 [details] [diff] [review], and 
attachment 486106 [details] [diff] [review] folded into : TR changeset - 5531:3e1e0b1b8d65
  http://hg.mozilla.org/tamarin-redux/rev/3e1e0b1b8d65