Lars T Hansen [:lth] Assignee
	Description • 6 years ago

testGCAllocator
/home/lhansen/m-i/js/src/jsapi-tests/testGCAllocator.cpp:147:CHECK failed: positionIsCorrect("x--xx---x-oo--x-", stagingArea, chunkPool, tempChunks)
TEST-UNEXPECTED-FAIL | testGCAllocator | /home/lhansen/m-i/js/src/jsapi-tests/testGCAllocator.cpp:147:CHECK failed: positionIsCorrect("x--xx---x-oo--x-", stagingArea, chunkPool, tempChunks)

Does not fail on simulator.

I'm testing m-i 404637:57d02e04afd5 without any local patches.  Fails both debug and release builds with the same error message.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 1 • 6 years ago

That's an odd place for it to fail. The allocator is supposed to allocate a 1MiB region offset 6MiB from the start of the staging area in this test (skipping over the alignable region starting at the 2.5MiB offset).

Could you check what offset the region allocated actually has (|result - base| after [1])?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi-tests/testGCAllocator.cpp#258

	Lars T Hansen [:lth] Assignee
	Comment 2 • 6 years ago

Running on simulator, the successive values for result-base are (addresses grow down):

 = 6291456
 = 6291456
 = 5242880
 = 3145728
 = 2097152
 = 2097152

Running on hardware, the values are (addresses grow up):

 = 1048576
 = 1048576
 = 2097152
 = 4194304
 = 3145728

I think this is a bug in the test case.  The string that fails is this:

    x--xx---x-oo--x-

but is there not space for the block here at position 6 (ie 3MB), as the allocation indicates there should be?  Do addresses grow up on any other platform?

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 3 • 6 years ago

> but is there not space for the block here at position 6 (ie 3MB), as the allocation indicates there should be?

There is, but because of limitations on what we can do with system calls, our allocator shouldn't be able to find it. The expected behavior is:

1) Try to allocate a 1MiB region. If it's aligned, we're done. Otherwise:
2) See if we can map enough space above/below it to produce an aligned 1MiB region.
3) Map a 2MiB region (|2 * chunksize - pagesize|, technically) and unmap the unaligned portions
4) If there are no 2MiB regions left, try (1) and (2) but keep hold of the unalignable regions to force the OS to give us something new

This test is trying to ensure that we don't hit (1) or (2) when we expect to hit (3). Getting a better result than we expect isn't a bad thing necessarily, but I'd like to understand it. Is the memory allocator doing something to prevent handing out the same region each time?

> Do addresses grow up on any other platform?

IIRC addresses grow up on Windows and OSX, and down on Linux (but apparently not here!).

	Lars T Hansen [:lth] Assignee
	Comment 4 • 6 years ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #3)
> > but is there not space for the block here at position 6 (ie 3MB), as the allocation indicates there should be?
> 
> There is, but because of limitations on what we can do with system calls,
> our allocator shouldn't be able to find it. The expected behavior is:
> 
> 1) Try to allocate a 1MiB region. If it's aligned, we're done. Otherwise:
> 2) See if we can map enough space above/below it to produce an aligned 1MiB
> region.
> 3) Map a 2MiB region (|2 * chunksize - pagesize|, technically) and unmap the
> unaligned portions
> 4) If there are no 2MiB regions left, try (1) and (2) but keep hold of the
> unalignable regions to force the OS to give us something new
> 
> This test is trying to ensure that we don't hit (1) or (2) when we expect to
> hit (3). Getting a better result than we expect isn't a bad thing
> necessarily, but I'd like to understand it. Is the memory allocator doing
> something to prevent handing out the same region each time?

I don't understand.  We hit case 1 here - we try to allocate a 1MB region on a 1MB boundary, and we find one at position 6.  Nothing that I see prevents that from happening.  In gc::MapMemory, while the hint points into the reserved region, we receive pages with the high bits set that we discard (actually the same chunk every time); then when the hint matches the 1MB open region at position 6 we get the 1MB chunk that is there.

Why should step 1 fail?  What are the limitations on system calls that would normally prevent this outcome?  How can this test ensure that step 1 fails without reserving the memory at position 6?

(Because of how we search for heap memory with proper bits I get the same heap addresses on every run; this basically defeats ASLR, which might be a concern, but I'll make that the subject of a different bug :)

	Lars T Hansen [:lth] Assignee
	Comment 5 • 6 years ago

Summary of discussion with Emanuel:

The constraint on mmap that should allow this test to succeed is that we expect mmap to give us the first contiguous 1MB region that is available, and since that won't be aligned, we'll fall through to (3).  But on ARM64 this is not the case since we pass an explicit hint to mmap about the address that we want, and that hint is 1MB-aligned the way things are set up currently.  So we get the chunk at position 6, since that's what we've asked for.

Also, addresses grow up here - despite it being Linux - because the hint loop grows addresses upward in memory.

	Lars T Hansen [:lth] Assignee
	Comment 6 • 6 years ago

Oh, and we should probably just comment the test case out for arm64 for the time being.

	Lars T Hansen [:lth] Assignee
	Comment 7 • 6 years ago

As discussed on IRC.  With this test disabled the test module passes on ARM64 hardware.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 8 • 6 years ago

Comment on attachment 8954343 [details] [diff] [review]
bug1440330-disable-jsapi-test-arm64.patch

Review of attachment 8954343 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, one comment nit.

::: js/src/jsapi-tests/testGCAllocator.cpp
@@ +143,5 @@
>      CHECK(positionIsCorrect("x--xooxxx-------", stagingArea, chunkPool, tempChunks));
>      // Check that we fall back to the slow path after two unalignable chunks.
>      CHECK(positionIsCorrect("x--xx--xoo--xxx-", stagingArea, chunkPool, tempChunks));
> +#ifndef __aarch64__
> +    // Bug 1440330 - this test is incorrect for aarch64 because mmap only looks for

Perhaps it would be better to say s/mmap/MapMemory/ here as the difference comes from our own implementation.

That code is also used `#if (defined(__sparc__) && defined(__arch64__) && defined(__NetBSD__))` but I don't really care that much about making the test match that ugly condition. Hopefully this will go away soon.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 9 • 6 years ago

Heh, I actually copied the wrong condition there; meant to say `#if (defined(__sparc__) && defined(__arch64__) && defined(__linux__))`.

	Pulsebot
	Comment 10 • 6 years ago

Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7b22a78379
Disable a test on ARM64 that is invalid on that platform.  r=ehoogeveen

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ef7b22a78379