Bug 178130 - [ARM64][Linux] Re-enable Gigacage
Summary: [ARM64][Linux] Re-enable Gigacage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on: 178654
Blocks: 108645
  Show dependency treegraph
 
Reported: 2017-10-10 06:52 PDT by Zan Dobersek
Modified: 2018-03-22 13:39 PDT (History)
12 users (show)

See Also:


Attachments
Patch (4.97 KB, patch)
2017-10-10 12:13 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (4.11 KB, patch)
2017-10-22 23:33 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-10-10 06:52:03 PDT
Disabled after general support for ARM64 was landed in bug #177586.

ELF-specific relocation specifiers have to be used in globaladdr opcode in offlineasm, and the .loh directive has to be avoided since it's not supported in GCC.
Comment 1 Zan Dobersek 2017-10-10 12:13:07 PDT
Created attachment 323330 [details]
Patch
Comment 2 Michael Catanzaro 2017-10-10 12:45:16 PDT
Comment on attachment 323330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323330&action=review

> Source/bmalloc/ChangeLog:11
> +        (bmalloc::api::tryLargeMemalignVirtual): Call Heap::tryAllocateLarge(),
> +        which won't intentionally crash if the allocation fails.

Why is this important? I see that it is correct, because it's called in tryLargeMemalignVirtual, but seems odd to change cross-platform behavior in an ARM64 commit.
Comment 3 Zan Dobersek 2017-10-10 23:53:21 PDT
Comment on attachment 323330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323330&action=review

>> Source/bmalloc/ChangeLog:11
>> +        which won't intentionally crash if the allocation fails.
> 
> Why is this important? I see that it is correct, because it's called in tryLargeMemalignVirtual, but seems odd to change cross-platform behavior in an ARM64 commit.

I don't mind doing it in a separate patch. But it avoids ~500 WASM crashes that occur because WASM::Memory tries to allocate 4GB of virtual memory while the current ARM64 Gigacage implementation doesn't support such large allocations.
Comment 4 Michael Catanzaro 2017-10-11 05:40:07 PDT
I would just mention that in the changelog, then.
Comment 5 JF Bastien 2017-10-11 08:54:38 PDT
(In reply to Zan Dobersek from comment #3)
> Comment on attachment 323330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323330&action=review
> 
> >> Source/bmalloc/ChangeLog:11
> >> +        which won't intentionally crash if the allocation fails.
> > 
> > Why is this important? I see that it is correct, because it's called in tryLargeMemalignVirtual, but seems odd to change cross-platform behavior in an ARM64 commit.
> 
> I don't mind doing it in a separate patch. But it avoids ~500 WASM crashes
> that occur because WASM::Memory tries to allocate 4GB of virtual memory
> while the current ARM64 Gigacage implementation doesn't support such large
> allocations.

That sounds like a bug?
Comment 6 Yusuke Suzuki 2017-10-11 09:14:03 PDT
Comment on attachment 323330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323330&action=review

>>>> Source/bmalloc/ChangeLog:11
>>>> +        which won't intentionally crash if the allocation fails.
>>> 
>>> Why is this important? I see that it is correct, because it's called in tryLargeMemalignVirtual, but seems odd to change cross-platform behavior in an ARM64 commit.
>> 
>> I don't mind doing it in a separate patch. But it avoids ~500 WASM crashes that occur because WASM::Memory tries to allocate 4GB of virtual memory while the current ARM64 Gigacage implementation doesn't support such large allocations.
> 
> That sounds like a bug?

Yeah, that sounds like a bug. It is not intended behavier, right?
I think that the correct fix is fixing bmalloc side.
BTW, is this specific to ARM64 Gugacage? Or can we reproduce this in x64?
I think it is ARM64 specific since we do not see such crashes in x64 test bot.
Comment 7 Carlos Alberto Lopez Perez 2017-10-11 10:19:37 PDT
(In reply to Yusuke Suzuki from comment #6)
> I think it is ARM64 specific since we do not see such crashes in x64 test
> bot.

The GTK and WPE bots are not running the JSC tests marked as memory-limited since bug 175140
Comment 8 Yusuke Suzuki 2017-10-11 10:49:40 PDT
(In reply to Carlos Alberto Lopez Perez from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > I think it is ARM64 specific since we do not see such crashes in x64 test
> > bot.
> 
> The GTK and WPE bots are not running the JSC tests marked as memory-limited
> since bug 175140

Oh, I've just run trunk JSCOnly on Linux x64 with `run-jsc-stress-tests -j path/to/jsc JSTests/wasm.yaml` and all the tests (1010 tests) pass.
Comment 9 Michael Catanzaro 2017-10-11 10:57:51 PDT
Comment on attachment 323330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323330&action=review

> Source/bmalloc/bmalloc/bmalloc.h:74
> -    return heap.allocateLarge(lock, alignment, size, AllocationKind::Virtual);
> +    return heap.tryAllocateLarge(lock, alignment, size, AllocationKind::Virtual);

I don't know much about bmalloc, but this function is named tryLargeMemalignVirtual, so it looks to be clearly broken currently.
Comment 10 Zan Dobersek 2017-10-13 09:21:52 PDT
Comment on attachment 323330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323330&action=review

>>>>> Source/bmalloc/ChangeLog:11
>>>>> +        which won't intentionally crash if the allocation fails.
>>>> 
>>>> Why is this important? I see that it is correct, because it's called in tryLargeMemalignVirtual, but seems odd to change cross-platform behavior in an ARM64 commit.
>>> 
>>> I don't mind doing it in a separate patch. But it avoids ~500 WASM crashes that occur because WASM::Memory tries to allocate 4GB of virtual memory while the current ARM64 Gigacage implementation doesn't support such large allocations.
>> 
>> That sounds like a bug?
> 
> Yeah, that sounds like a bug. It is not intended behavier, right?
> I think that the correct fix is fixing bmalloc side.
> BTW, is this specific to ARM64 Gugacage? Or can we reproduce this in x64?
> I think it is ARM64 specific since we do not see such crashes in x64 test bot.

In ensureGigacage(), the sizes for different kinds are summed up and that value is then used to do a single allocation. On x86_64 this sum exceeds 80GB, while on ARM64 it's for now limited to around 3GB (not counting alignment adjustments). Bug #177605 will increase those values.

Crash occurs under MemoryManager::tryAllocateVirtualPages(), in WasmMemory implementation file. The Memory::fastMappedBytes() value is used as the size of allocation, which translates to 4GB. The Gigacage on x86_64 has no problems to accommodate that, while on ARM64 the allocation fails and produces a crash because of Heap::allocateLarge() being used in bmalloc::tryLargeMemalignVirtual().
Comment 11 Zan Dobersek 2017-10-22 23:26:55 PDT
bmalloc part spun off into bug #178654.
Comment 12 Zan Dobersek 2017-10-22 23:33:10 PDT
Created attachment 324546 [details]
Patch
Comment 13 Zan Dobersek 2017-10-30 01:16:47 PDT
Comment on attachment 324546 [details]
Patch

Clearing flags on attachment: 324546

Committed r224171: <https://trac.webkit.org/changeset/224171>
Comment 14 Zan Dobersek 2017-10-30 01:16:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Saam Barati 2017-10-30 11:34:08 PDT
BTW, right now, shouldBeEnabled inside Gigacage.cpp returns false for ARM64. You may want to enable that for Linux or change the guard to return false if IOS.
Comment 16 Michael Catanzaro 2017-10-30 13:10:55 PDT
Reopening in light of that.
Comment 17 Michael Catanzaro 2018-03-18 18:46:11 PDT
Zan, do you want to look at this?
Comment 18 Zan Dobersek 2018-03-22 13:36:37 PDT
It's been enabled back for ARM64 in r225701.
https://trac.webkit.org/changeset/225701/webkit
Comment 19 Radar WebKit Bug Importer 2018-03-22 13:39:00 PDT
<rdar://problem/38765155>