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

RESOLVED FIXED in Q3 11 - Serrano

Status

P3
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: pnkfelix, Assigned: pnkfelix)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-qrb +
flashplayer-bug -
flashplayer-triage +

Details

(Whiteboard: has-patch)

Attachments

(5 attachments, 2 obsolete attachments)

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.
(Assignee)

Updated

8 years ago
Blocks: 445780
Created attachment 459293 [details] [diff] [review]
adds dispersive mode for emulating semi-adversarial env.
Created attachment 459298 [details] [diff] [review]
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.)
Assignee: nobody → fklockii
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Attachment #459293 - Flags: feedback?(treilly)
(Assignee)

Updated

8 years ago
Attachment #459298 - Flags: feedback?(treilly)

Comment 3

8 years ago
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

8 years ago
(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?
(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.)
(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

8 years ago
(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
(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

8 years ago
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
(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

8 years ago
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.
Attachment #459293 - Flags: feedback?(treilly) → feedback-

Comment 12

8 years ago
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).
Attachment #459298 - Flags: feedback?(treilly) → feedback-

Updated

8 years ago
Flags: flashplayer-triage+
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2.x-Spicy
Created attachment 486101 [details] [diff] [review]
adds dispersive mode for emulating semi-adversarial env.

Revised to put newly added code under DEBUG preprocessor guard.
Attachment #459293 - Attachment is obsolete: true
Attachment #486101 - Flags: superreview?(lhansen)
Attachment #486101 - Flags: review?(treilly)
Created attachment 486103 [details] [diff] [review]
selftest exercising the adversarial mode.

Revised to set heap limit dynamically rather than patching the source.
Attachment #459298 - Attachment is obsolete: true
Attachment #486103 - Flags: superreview?(lhansen)
Attachment #486103 - Flags: review?(treilly)
Created attachment 486104 [details] [diff] [review]
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.)
Attachment #486104 - Flags: superreview?(lhansen)
Attachment #486104 - Flags: review?(treilly)
Created attachment 486105 [details] [diff] [review]
xcode project file changes to add selftest
Created attachment 486106 [details] [diff] [review]
visual studio 2008 project file changes to add selftest
(Assignee)

Updated

8 years ago
Attachment #486105 - Flags: superreview?(lhansen)
Attachment #486105 - Flags: review?(treilly)
(Assignee)

Updated

8 years ago
Attachment #486106 - Flags: superreview?(lhansen)
Attachment #486106 - Flags: review?(treilly)

Updated

8 years ago
Attachment #486101 - Flags: review?(treilly) → review+

Updated

8 years ago
Attachment #486103 - Flags: review?(treilly) → review+

Comment 18

8 years ago
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.
Attachment #486104 - Flags: review?(treilly) → review+

Updated

8 years ago
Attachment #486105 - Flags: review?(treilly) → review+

Comment 19

8 years ago
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.
Attachment #486106 - Flags: review?(treilly) → review-

Comment 20

8 years ago
I'm unlikely to get to these patches this week.

Updated

8 years ago
Target Milestone: flash10.2.x-Spicy → flash10.x - Serrano

Updated

8 years ago
Whiteboard: has-patch

Updated

8 years ago
Attachment #486101 - Flags: superreview?(lhansen) → superreview+

Comment 21

8 years ago
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.

Updated

8 years ago
Attachment #486103 - Flags: superreview?(lhansen) → superreview+

Updated

8 years ago
Attachment #486104 - Flags: superreview?(lhansen) → superreview+

Updated

8 years ago
Attachment #486105 - Flags: superreview?(lhansen) → superreview+

Comment 22

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

Rubber stamp.
Attachment #486106 - Flags: superreview?(lhansen) → superreview+

Comment 23

8 years ago
As a general comment, the last three patches here were probably not required for review.
(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.
(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.)
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Flags: flashplayer-bug-
You need to log in before you can comment on or make changes to this bug.