Closed Bug 1275204 Opened 8 years ago Closed 7 years ago

JSValue overloading and crunching on Linux sparc64 causes segfaults

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: glaubitz, Assigned: glaubitz)

References

Details

Attachments

(8 files, 19 obsolete files)

1.42 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.23 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.90 KB, patch
glandium
: review+
martin
: review+
Details | Diff | Splinter Review
886 bytes, patch
glandium
: review+
martin
: review+
Details | Diff | Splinter Review
2.98 KB, patch
martin
: review+
Details | Diff | Splinter Review
1.43 KB, patch
jesup
: review+
martin
: review+
Details | Diff | Splinter Review
2.29 KB, patch
martin
: review+
fitzgen
: feedback+
Details | Diff | Splinter Review
2.81 KB, patch
ehoogeveen
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160508001106

Steps to reproduce:

Hi!

We're currently working to shape up sparc64 in Debian again and I have had a look at Firefox in the past days. The work is being done on Debian unstable with an up-to-date toolchain and kernel.

After fixing some issues with the compiler definitions [1], I was actually able to run "make" without problems and build Firefox 46 all the way through, however, "make install" crashes when running xpcshell [2]:

Executing /<<PKGBUILDDIR>>/build-browser/dist/bin/xpcshell -g /<<PKGBUILDDIR>>/build-browser/dist/bin/ -a /<<PKGBUILDDIR>>/build-browser/dist/bin/ -f /<<PKGBUILDDIR>>/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache("resource://gre/");
Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/toolkit/mozapps/installer/packager.py", line 410, in <module>
    main()
  File "/<<PKGBUILDDIR>>/toolkit/mozapps/installer/packager.py", line 404, in main
    args.source, gre_path, base)
  File "/<<PKGBUILDDIR>>/toolkit/mozapps/installer/packager.py", line 161, in precompile_cache
    errors.fatal('Error while running startup cache precompilation')
  File "/<<PKGBUILDDIR>>/python/mozbuild/mozpack/errors.py", line 103, in fatal
    self._handle(self.FATAL, msg)
  File "/<<PKGBUILDDIR>>/python/mozbuild/mozpack/errors.py", line 98, in _handle
    raise ErrorMessage(msg)
mozpack.errors.ErrorMessage: Error: Error while running startup cache precompilation

Recompiling with --enable-debug-symbols and running xpcshell manually yields this backtrace:

(gdb) bt
#0  0xfff8000102a117c4 in js::gc::Cell::storeBuffer (this=<optimizedout>) at /build/firefox-F4xXKx/firefox-46.0.1/js/src/gc/Heap.h:1339
#1  js::HeapSlot::post (target=..., slot=0, kind=js::HeapSlot::Slot,owner=0xfff8000112523070, this=0xfff8000112523090)
    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/gc/Barrier.h:692#2  js::HeapSlot::set (this=0xfff8000112523090,
owner=owner@entry=0xfff8000112523070, slot=slot@entry=0, v=...,kind=js::HeapSlot::Slot)    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/gc/Barrier.h:679
#3  0xfff8000102a13634 in js::NativeObject::setSlot(this=0xfff8000112523070, value=..., slot=0)    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/NativeObject.h:824
#4  0xfff8000102a13758 in js::NativeObject::setReservedSlot (v=...,index=0, this=<optimized out>)    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/NativeObject.h:890
#5  js::ClonedBlockObject::create (cx=cx@entry=0xfff800010a73a400,block=..., enclosing=...)    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/ScopeObject.cpp:945
#6  0xfff8000102a18f1c in js::ClonedBlockObject::createGlobal(cx=cx@entry=0xfff800010a73a400, global=...)    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/ScopeObject.cpp:976
#7  0xfff80001029b43fc in js::GlobalObject::createInternal(cx=cx@entry=0xfff800010a73a400,    clasp=0xfff8000103e07028<JSRuntime::createSelfHostingGlobal(JSContext*)::shgClass>) at/build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/GlobalObject.cpp:278
#8  0xfff8000102a062bc in JSRuntime::createSelfHostingGlobal(cx=cx@entry=0xfff800010a73a400)    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/SelfHosting.cpp:1956
#9  0xfff8000102a08988 in JSRuntime::initSelfHosting
(this=this@entry=0xfff800010c3d2000, cx=cx@entry=0xfff800010a73a400)
    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/vm/SelfHosting.cpp:1988
#10 0xfff8000102876f74 in js::NewContext (rt=0xfff800010c3d2000,rt@entry=0x0, stackChunkSize=stackChunkSize@entry=8192)
    at /build/firefox-F4xXKx/firefox-46.0.1/js/src/jscntxt.cpp:121
#11 0xfff8000102876fbc in JS_NewContext (rt=0x0,stackChunkSize=stackChunkSize@entry=8192) at/build/firefox-F4xXKx/firefox-46.0.1/js/src/jsapi.cpp:573
#12 0xfff8000101058308 in XPCJSContextStack::InitSafeJSContext(this=0xfff800010b248240)    at/build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/XPCJSContextStack.cpp:122
#13 0xfff80001010738ac in nsXPConnect::InitStatics () at/build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/nsXPConnect.cpp:126
#14 0xfff800010105ccd4 in xpcModuleCtor () at/build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/XPCModule.cpp:13
#15 0xfff800010229b818 in Initialize () at/build/firefox-F4xXKx/firefox-46.0.1/layout/build/nsLayoutModule.cpp:436
#16 0xfff8000100bf9070 in nsComponentManagerImpl::KnownModule::Load(this=0xfff800010b22f280)    at/build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:898
#17 0xfff8000100bfa018 in nsFactoryEntry::GetFactory(this=0xfff800010b23a180)    at/build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:1934
#18 0xfff8000100bfa8d4 in nsComponentManagerImpl::CreateInstanceByContractID (this=0xfff800010a7f60c0,    aContractID=0xfff8000102bb8b98 "@mozilla.org/moz/jsloader;1",aDelegate=0x0, aIID=..., Result=0x7feffffe930)    at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:1232
#19 0xfff8000100bfc298 in nsComponentManagerImpl::GetServiceByContractID (this=0xfff800010a7f60c0,    aContractID=0xfff8000102bb8b98 "@mozilla.org/moz/jsloader;1",aIID=..., aResult=0x7feffffeac0)    at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/components/nsComponentManager.cpp:1592
#20 0xfff8000100c28514 in CallGetService (aContractID=0xfff8000102bab760 <nsISupports::COMTypeInfo<nsISupports, void>::kIID> "", aIID=..., aResult=0x0,     aResult@entry=0x7feffffeac0) at
/build/firefox-F4xXKx/firefox-46.0.1/xpcom/glue/nsComponentManagerUtils.cpp:69
#21 0xfff8000100c28538 in nsGetServiceByContractID::operator() (this=this@entry=0x7feffffeb58, aIID=..., aInstancePtr=aInstancePtr@entry=0x7feffffeac0)    at
/build/firefox-F4xXKx/firefox-46.0.1/xpcom/glue/nsComponentManagerUtils.cpp:280
#22 0xfff8000100c202b0 in nsCOMPtr_base::assign_from_gs_contractid (this=this@entry=0x7feffffeba0, aGS=..., aIID=...)    at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/glue/nsCOMPtr.cpp:103
#23 0xfff8000100c24c28 in nsCOMPtr<nsISupports>::nsCOMPtr (aGS=..., this=0x7feffffeba0)    at /build/firefox-F4xXKx/firefox-46.0.1/build-browser/dist/include/nsCOMPtr.h:855
#24 NS_InitXPCOM2 (aResult=0x7feffffedd8, aBinDirectory=<optimized out>, aAppFileLocationProvider=<optimized out>)    at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/build/XPCOMInit.cpp:724

---Type <return> to continue, or q <return> to quit---
#25 0xfff8000100c24de0 in NS_InitXPCOM2 (aResult=0x1, aBinDirectory=0x7fefffff598, aAppFileLocationProvider=0x7fefffff5a8, aAppFileLocationProvider@entry=0x7feffffee90)    at /build/firefox-F4xXKx/firefox-46.0.1/xpcom/build/XPCOMInit.cpp:481
#26 0xfff800010107a794 in XRE_XPCShellMain (argc=1, argv=0x7fefffff598, envp=0x7fefffff5a8) at /build/firefox-F4xXKx/firefox-46.0.1/js/xpconnect/src/XPCShellImpl.cpp:1424
#27 0x0000010000004710 in main (argc=1, argv=0x7fefffff598, envp=0x7fefffff5a8) at /build/firefox-ORgwZM/firefox-46.0.1/js/xpconnect/shell/xpcshell.cpp:54
(gdb)

When I print (this), I get the following:

(gdb) print *this
Cannot access memory at address 0x116622060

and

(gdb) print/x *this
$4 = {<js::WriteBarrieredBase<JS::Value>> = {<js::BarrieredBase<JS::Value>> = {<js::BarrieredBaseMixins<JS::Value>> = {<js::ValueOperations<js::WriteBarrieredBase<JS::Value> >> = {<No data fields>}, <No data fields>}, value = {data = {asBits = 0xfffc000116622060, debugView = {tag = 0x1fff8, payload47 = 0x116622060}, s = {padding = 0xfffc0001, payload = {              i32 = 0x16622060, u32 = 0x16622060, why = 0x16622060}}, asDouble = 0x8000000000000000, asPtr = 0xfffc000116622060, asWord = 0xfffc000116622060,           asUIntPtr = 0xfffc000116622060}}}, <No data fields>}, <No data fields>}

Thus, an issue with JSValue again like it has been seen on ia64 [3] and NetBSD/sparc64 [4].

As a first hack, I extended all the ia64 #ifdefs from [3] to apply on Linux/sparc64 but then xpcshell crashes with an out of memory error. This is caused by the fact that mmap on Linux completely ignores the hint when the memory area has already been allocated and unlike NetBSD, it will just allocate memory from the upper region again.

This can be demonstrated with a small piece of code both run on Linux and NetBSD:

On Linux:

glaubitz@ikarus:~$ cat vmtest.c 
#include <stdio.h>
#include <sys/mman.h>

int main(void)
{
	void *ptr1, *ptr2, *ptr3, *ptr4, *ptr5, *ptr6;
	ptr1 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr2 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr3 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr4 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr5 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr6 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	printf("%p %p %p %p %p %p\n", ptr1, ptr2, ptr3, ptr4, ptr5, ptr6);
}
glaubitz@ikarus:~$ ./vmtest 
0x7f80c32f6000 0x70000000000 0x7f80c2d5d000 0x7f80c2c5d000 0x7f80c2b5d000 0x7f80c2a5d000
glaubitz@ikarus:~$ uname -vm
#1 SMP Debian 4.5.1-1 (2016-04-14) x86_64
glaubitz@ikarus:~$

On NetBSD:

testbsd# cat vmtest.c
#include <stdio.h>
#include <sys/mman.h>

int main(void)
{
	void *ptr1, *ptr2, *ptr3, *ptr4, *ptr5, *ptr6;
	ptr1 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr2 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr3 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr4 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr5 = mmap((void*)0x0000070000000000ULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	ptr6 = mmap(NULL, 1024*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	printf("%p %p %p %p %p %p\n", ptr1, ptr2, ptr3, ptr4, ptr5, ptr6);
}
testbsd# ./vmtest
0x7f7ff7ef8000 0x70000000000 0x6fffff00000 0x6ffffe00000 0x6ffffd00000 0x7f7ff7b00000
testbsd# uname -vm
NetBSD 7.0 (GENERIC.201509250726Z) amd64
testbsd#

As you can see, on NetBSD, mmmap with a hint will always return memory in the requested region, even if the memory has already been allocated before. On Linux, the hint is ignored when the memory pointed to by it is already allocated.

As a crude hack, I subtracted the value of random() from "hint" in the corresponding mmap() calls which are found in the ia64 #ifdefs which I have activated for Linux/sparc64, too.

With this crude hack, I can actually xpcshell without crashing:

(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# ./build-browser/dist/bin/xpcshell 

(process:186356): Gtk-WARNING **: Locale not supported by C library.
	Using the fallback 'C' locale.
js> print ("Hello World!");
Hello World!
js> ^C
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1#

But that works for simple applications only. Running something more complex will still make xpcshell crash, I assume because there is much more memory being allocated with mmap and at some point, subtracting random() from "hint" will again yields a value in the upper memory area:

(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/xpcshell -g /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -a /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -f /build/firefox-ORgwZM/firefox-46.0.1/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache\("resource://gre/"\);

(process:186399): Gtk-WARNING **: Locale not supported by C library.
	Using the fallback 'C' locale.
Bus error
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1# /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/xpcshell -g /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -a /build/firefox-ORgwZM/firefox-46.0.1/build-browser/dist/bin/ -f /build/firefox-ORgwZM/firefox-46.0.1/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache\("resource://gre/"\);

(process:186442): Gtk-WARNING **: Locale not supported by C library.
	Using the fallback 'C' locale.
Segmentation fault
(sid)root@deb4g:/build/firefox-ORgwZM/firefox-46.0.1#

In any case, we have now found the actual reason why JavaScript crashes on Linux/sparc64. But I have no idea how to fix this issue, honestly, given the fact that mmap() doesn't behave as nicely as it does on NetBSD. But I think the problem should be tackled, not just for sparc64, but in general since it too much depends on the underlying hardware and the behavior of mmap().

I have also tracked some of the debugging in a corresponding Debian bug report [5].

Adrian

> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1273764
> [2] https://buildd.debian.org/status/fetch.php?pkg=firefox&arch=sparc64&ver=46.0.1-1&stamp=1463370916
> [3] https://bugzilla.mozilla.org/show_bug.cgi?id=589735
> [4] https://bugzilla.mozilla.org/show_bug.cgi?id=994133
> [5] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=824449
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
1.  OK, so just to get the most basic thing out of the way first, can you build with --enable-debug
    and --disable-optimize? We would expect to see an assertion failure before you get this far.

2.  On ia64, we rely on the fact that addresses are really only 48-bit:

        0000 0000 0000 0000 to 0000 7fff ffff ffff, and
        ffff 8000 0000 0000 to ffff ffff ffff ffff

    (All bits to the left of bit 48 are the same as bit 48.)

    Is this the case on SPARC 64?

3.  Furthermore, we need addresses in the upper half of the range.

    The actual allocation occurs in js/src/gc/Memory.cpp, function MapMemory(); and/or 
    mfbt/TaggedAnonymousMemory.cpp, MozTaggedAnonymousMmap().

    The comment in MapMemory() makes it sound like you removing the `defined(__NetBSD__)`
    part of the #ifdef condition might be worth a shot.
Are you interested in getting an account on a sparc64 porterbox? Might be easier and faster.

This machine is fast enough to build Firefox in around 2 hours.
As for the pointers: All the bits left of bit 52 are the same as bit 52 on this particular SPARC machine (SPARC-T5). Older SPARC CPUs may use less address bits.
We don't have a Value representation scheme that can support 52-bit addresses, so this might be a lost cause. However, option #3 is your best bet if you're willing to try something.

I don't have time to look into it myself.
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> We don't have a Value representation scheme that can support 52-bit
> addresses, so this might be a lost cause.

Well, chances are that x86_64 will use more address space in the future, too, as 48 bits are already exhausted at 256 TiB RAM, so I'm not sure whether this is a safe bet.

> However, option #3 is your best bet if you're willing to try something.

As I said, I have tried the NetBSD fix. However, that doesn't work on Linux since mmap() doesn't behave as nicely on Linux as it does on NetBSD. The "hint" is only honored when the memory region pointed to is not already allocated.
In order to do that, we'd have to move Number back to the heap. That's not necessarily the end of the world, now that most performance critical code is using TypedArrays and TypedObject. It is, however, also not our highest priority at the moment, given that none of our tier 1 platforms have this issue.

In the short term, you can probably paper over the issue by pre-mmaping bits 48 to 52 at process startup.
Hi Terrence!

(In reply to Terrence Cole [:terrence] from comment #6)
> In the short term, you can probably paper over the issue by pre-mmaping bits
> 48 to 52 at process startup.

Sorry, I just saw your comment now.

Can you elaborate more on how that would work? Wouldn't this actually eat up quite some memory? Or is it possible to pre-mmap the memory without actually occupying any memory?

Adrian
Mapping memory doesn't actually allocate the physical pages unless you touch them, so assuming something else isn't using an address with those bits set, a simple mmap call should work. If something *is* using addresses with the high bit set, it'll probably be pretty annoying to map everything around it, but I guess you could do some sort of binary search.
I have come up with the attached patch which first fixes the pre-processor definitions for sparc64 [1]. Then I tried using the allocator for ARM64 [2] on sparc64 as well in the hope that this would fix the problem. Unfortunately, I still see the crashes on sparc64. But maybe I'm just missing something obvious.

> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1273764
Version: 46 Branch → Trunk
Attached patch linux-sparc64-fixes.patch (obsolete) — Splinter Review
Hi!

The attached patch fixes the build on Linux/sparc64 for me. With the patch applied, I can successfully build Firefox 50.0 on Debian/sparc64.

I assume the patch cannot be merged "as is" and I particularly think that it could be split into parts. However, I would like to get some feedback first.

This patch also fixes #1273764 [1].

Thanks,
Adrian

> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1273764
Attachment #8817844 - Attachment is obsolete: true
Attachment #8818000 - Flags: review?(nfroyd)
Attachment #8818000 - Flags: review?(mh+mozilla)
Attachment #8818000 - Flags: review?(martin)
Attachment #8818000 - Flags: review?(emanuel.hoogeveen)
Attachment #8818000 - Flags: review?(martin) → review+
Attached patch linux-sparc64-fixes.patch (obsolete) — Splinter Review
Updated patch, cleaned up some of the comments.
Attachment #8818006 - Flags: review?(nfroyd)
Attachment #8818006 - Flags: review?(mh+mozilla)
Attachment #8818006 - Flags: review?(martin)
Attachment #8818006 - Flags: review?(emanuel.hoogeveen)
Attachment #8818000 - Attachment is obsolete: true
Attachment #8818000 - Flags: review?(nfroyd)
Attachment #8818000 - Flags: review?(mh+mozilla)
Attachment #8818000 - Flags: review?(emanuel.hoogeveen)
Attached patch linux-sparc64-fixes.patch (obsolete) — Splinter Review
Another update. With this update, the ARM64 allocator is now only used on Linux only (defined(__linux__)). *BSD doesn't need to use the allocator as on *BSD, mmap() always honours the hint parameter and returns memory in the vicinity of the address hint is pointing to. Linux doesn't do that, unfortunately which is why the manual allocator is necessary.
Attachment #8818006 - Attachment is obsolete: true
Attachment #8818006 - Flags: review?(nfroyd)
Attachment #8818006 - Flags: review?(mh+mozilla)
Attachment #8818006 - Flags: review?(martin)
Attachment #8818006 - Flags: review?(emanuel.hoogeveen)
Attachment #8818007 - Flags: review?(nfroyd)
Attachment #8818007 - Flags: review?(mh+mozilla)
Attachment #8818007 - Flags: review?(martin)
Attachment #8818007 - Flags: review?(emanuel.hoogeveen)
Comment on attachment 8818007 [details] [diff] [review]
linux-sparc64-fixes.patch

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

Thank you for these fixes.

r=me on the ipc/ and xpcom/ changes; please split those changes out into separate patches for commit purposes.  If you split out the toolkit/ changes and address the comments there, I can review that patch.  I will leave other changes for the appropriate people to review.

::: firefox-50.0.2.orig/ipc/chromium/src/build/build_config.h
@@ +83,4 @@
>  #elif defined(__ppc__) || defined(__powerpc__)
>  #define ARCH_CPU_PPC 1
>  #define ARCH_CPU_32_BITS 1
> +#elif defined(__sparc__) && defined(__arch64__)

This change is OK.

::: firefox-50.0.2.orig/media/webrtc/trunk/build/build_config.h
@@ +129,4 @@
>  #define ARCH_CPU_PPC 1
>  #define ARCH_CPU_32_BITS 1
>  #define ARCH_CPU_BIG_ENDIAN 1
> +#elif defined(__sparc__) && defined(__arch64__)

The changes in media/webrtc/ should be split out into their own patch and reviewed by :jesup.

::: firefox-50.0.2.orig/toolkit/components/protobuf/src/google/protobuf/stubs/platform_macros.h
@@ +67,4 @@
>  #define GOOGLE_PROTOBUF_ARCH_32_BIT 1
>  #elif defined(sparc)
>  #define GOOGLE_PROTOBUF_ARCH_SPARC 1
> +#if defined(__sparc_v9__) || defined(__sparcv9) || defined(__arch64__)

In addition to this change, any changes in toolkit/components/protobuf/ need to be added to the m-c-changes.patch file in that directory.

::: firefox-50.0.2.orig/widget/gonk/libui/sha1.c
@@ +87,4 @@
>  
>  /* old sparc64 gcc could not compile this */
>  #undef SPARC64_GCC_WORKAROUND
> +#if defined(__sparc__) && defined(__arch64__) && defined(__GNUC__) && __GNUC__ < 3

Is this change just due to __sparc64__ cleanup?  widget/gonk/ is probably never going to be built for sparc64...

::: firefox-50.0.2.orig/xpcom/reflect/xptcall/md/unix/moz.build
@@ +232,4 @@
>          'xptcstubs_ppc_openbsd.cpp',
>      ]
>  
> +if CONFIG['OS_ARCH'] == 'Linux' and CONFIG['OS_TEST'] == 'sparc':

The changes in this file are OK.
Attachment #8818007 - Flags: review?(nfroyd) → review+
Attachment #8818007 - Flags: review?(martin) → review+
(In reply to Nathan Froyd [:froydnj] from comment #14)
> r=me on the ipc/ and xpcom/ changes; please split those changes out into
> separate patches for commit purposes.  If you split out the toolkit/ changes
> and address the comments there, I can review that patch.  I will leave other
> changes for the appropriate people to review.

Ok, thanks. Will do!

> ::: firefox-50.0.2.orig/media/webrtc/trunk/build/build_config.h
> @@ +129,4 @@
> >  #define ARCH_CPU_PPC 1
> >  #define ARCH_CPU_32_BITS 1
> >  #define ARCH_CPU_BIG_ENDIAN 1
> > +#elif defined(__sparc__) && defined(__arch64__)
> 
> The changes in media/webrtc/ should be split out into their own patch and
> reviewed by :jesup.

Noted, thanks.

> :::
> firefox-50.0.2.orig/toolkit/components/protobuf/src/google/protobuf/stubs/
> platform_macros.h
> @@ +67,4 @@
> >  #define GOOGLE_PROTOBUF_ARCH_32_BIT 1
> >  #elif defined(sparc)
> >  #define GOOGLE_PROTOBUF_ARCH_SPARC 1
> > +#if defined(__sparc_v9__) || defined(__sparcv9) || defined(__arch64__)
> 
> In addition to this change, any changes in toolkit/components/protobuf/ need
> to be added to the m-c-changes.patch file in that directory.

Ah, that makes sense. Although the protobuf was actually copied verbatim from
upstream. Otherwise I had used defined(__sparc__) && defined(__arch64__).

See: https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/platform_macros.h#L68

> ::: firefox-50.0.2.orig/widget/gonk/libui/sha1.c
> @@ +87,4 @@
> >  
> >  /* old sparc64 gcc could not compile this */
> >  #undef SPARC64_GCC_WORKAROUND
> > +#if defined(__sparc__) && defined(__arch64__) && defined(__GNUC__) && __GNUC__ < 3
> 
> Is this change just due to __sparc64__ cleanup?  widget/gonk/ is probably
> never going to be built for sparc64...

Yeah, just for consistency reasons.

> ::: firefox-50.0.2.orig/xpcom/reflect/xptcall/md/unix/moz.build
> @@ +232,4 @@
> >          'xptcstubs_ppc_openbsd.cpp',
> >      ]
> >  
> > +if CONFIG['OS_ARCH'] == 'Linux' and CONFIG['OS_TEST'] == 'sparc':
> 
> The changes in this file are OK.

Ok.

I think I will split out the xpcom and ipc changes first in a separate patch, then we can have a look how to proceed with the remaining changes.

Thanks,
Adrian
Hi Nathan!

Here's the first split-out patch.

Adrian
Attachment #8818015 - Flags: review?(nfroyd)
Attachment #8818015 - Attachment is obsolete: true
Attachment #8818015 - Flags: review?(nfroyd)
Attachment #8818016 - Flags: review?(nfroyd)
Attachment #8818016 - Flags: review?(nfroyd) → review+
Attachment #8818017 - Flags: review?(nfroyd) → review+
Attachment #8818007 - Attachment is obsolete: true
Attachment #8818007 - Flags: review?(mh+mozilla)
Attachment #8818007 - Flags: review?(emanuel.hoogeveen)
Attachment #8818026 - Flags: review?(mh+mozilla)
Attachment #8818026 - Flags: review?(martin)
Attachment #8818027 - Flags: review?(mh+mozilla)
Attachment #8818027 - Flags: review?(martin)
Attachment #8818028 - Flags: review?(mh+mozilla)
Attachment #8818028 - Flags: review?(martin)
Attachment #8818029 - Flags: review?(mh+mozilla)
Attachment #8818029 - Flags: review?(martin)
Attachment #8818030 - Flags: review?(mh+mozilla)
Attachment #8818030 - Flags: review?(martin)
Attachment #8818031 - Flags: review?(mh+mozilla)
Attachment #8818031 - Flags: review?(martin)
Attachment #8818032 - Flags: review?(mh+mozilla)
Attachment #8818032 - Flags: review?(martin)
Split everything out. Please note 0002-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch must be applied after 0001-js-Use-better-pre-processor-defines-for-sparc64.patch, but the rest can be applied out of order.
Added the change for widget/gonk for consistency reasons.
Attachment #8818036 - Flags: review?(mh+mozilla)
Attachment #8818036 - Flags: review?(martin)
Oops, I accidentally messed up this patch when splitting it out.
Attachment #8818027 - Attachment is obsolete: true
Attachment #8818027 - Flags: review?(mh+mozilla)
Attachment #8818027 - Flags: review?(martin)
Attachment #8818045 - Flags: review?(mh+mozilla)
Attachment #8818045 - Flags: review?(martin)
Attachment #8818016 - Attachment is obsolete: true
Attachment #8818064 - Flags: review?(nfroyd)
Attachment #8818017 - Attachment is obsolete: true
Attachment #8818065 - Flags: review?(nfroyd)
Attachment #8818026 - Attachment is obsolete: true
Attachment #8818026 - Flags: review?(mh+mozilla)
Attachment #8818026 - Flags: review?(martin)
Attachment #8818068 - Flags: review?(mh+mozilla)
Attachment #8818068 - Flags: review?(martin)
Attachment #8818068 - Flags: review?(emanuel.hoogeveen)
Attachment #8818030 - Attachment is obsolete: true
Attachment #8818030 - Flags: review?(mh+mozilla)
Attachment #8818030 - Flags: review?(martin)
Attachment #8818070 - Flags: review?(mh+mozilla)
Attachment #8818070 - Flags: review?(martin)
Attachment #8818070 - Flags: review?(emanuel.hoogeveen)
Attachment #8818028 - Attachment is obsolete: true
Attachment #8818028 - Flags: review?(mh+mozilla)
Attachment #8818028 - Flags: review?(martin)
Attachment #8818029 - Attachment is obsolete: true
Attachment #8818029 - Flags: review?(mh+mozilla)
Attachment #8818029 - Flags: review?(martin)
Comment on attachment 8818064 [details] [diff] [review]
0001-Use-OpenBSD-sparc64-xptcall-stubs-on-Linux-sparc64.patch

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

Didn't I already review this?  You may want to look into using https://github.com/mozilla/git-bz-moz so you can upload individual patches and so you don't have to export the whole series everytime...
Attachment #8818064 - Flags: review?(nfroyd) → review+
Attachment #8818031 - Attachment is obsolete: true
Attachment #8818031 - Flags: review?(mh+mozilla)
Attachment #8818031 - Flags: review?(martin)
Attachment #8818065 - Flags: review?(nfroyd) → review+
Attachment #8818032 - Attachment is obsolete: true
Attachment #8818032 - Flags: review?(mh+mozilla)
Attachment #8818032 - Flags: review?(martin)
Attachment #8818036 - Attachment is obsolete: true
Attachment #8818036 - Flags: review?(mh+mozilla)
Attachment #8818036 - Flags: review?(martin)
Attachment #8818045 - Attachment is obsolete: true
Attachment #8818045 - Flags: review?(mh+mozilla)
Attachment #8818045 - Flags: review?(martin)
Attachment #8818073 - Flags: review?(mh+mozilla)
Attachment #8818073 - Flags: review?(martin)
Attachment #8818073 - Flags: review?(emanuel.hoogeveen)
Attachment #8818074 - Flags: review?(mh+mozilla)
Attachment #8818074 - Flags: review?(martin)
Attachment #8818074 - Flags: review?(emanuel.hoogeveen)
Attachment #8818076 - Flags: review?(mh+mozilla)
Attachment #8818076 - Flags: review?(martin)
Attachment #8818076 - Flags: review?(emanuel.hoogeveen)
Attachment #8818077 - Flags: review?(mh+mozilla)
Attachment #8818077 - Flags: review?(martin)
Attachment #8818077 - Flags: review?(emanuel.hoogeveen)
Attachment #8818078 - Flags: review?(mh+mozilla)
Attachment #8818078 - Flags: review?(martin)
Attachment #8818078 - Flags: review?(emanuel.hoogeveen)
Attachment #8818079 - Flags: review?(mh+mozilla)
Attachment #8818079 - Flags: review?(martin)
Attachment #8818079 - Flags: review?(emanuel.hoogeveen)
Sorry for the noise, but I forgot to add the bug number for some of the patches.
Attachment #8818068 - Flags: review?(martin) → review+
Attachment #8818079 - Flags: review?(martin) → review+
Attachment #8818078 - Flags: review?(martin) → review+
Attachment #8818077 - Flags: review?(martin) → review+
Attachment #8818076 - Flags: review?(martin) → review+
Attachment #8818074 - Flags: review?(martin) → review+
Attachment #8818073 - Flags: review?(martin) → review+
Attachment #8818070 - Flags: review?(martin) → review+
Attachment #8818068 - Flags: review?(mh+mozilla)
Attachment #8818068 - Flags: review?(emanuel.hoogeveen)
Attachment #8818068 - Flags: review+
Comment on attachment 8818070 [details] [diff] [review]
0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch

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

This seems reasonable, but Emanuel should probably take another look.
Attachment #8818070 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8818073 [details] [diff] [review]
0005-Bug-1273764-jemalloc-Use-better-pre-processor-define.patch

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

::: memory/jemalloc/src/include/jemalloc/internal/mb.h
@@ +76,4 @@
>  	    : "memory" /* Clobbers. */
>  	    );
>  }
> +#elif defined(__sparc__) && defined(__arch64__)

Please submit this upstream to https://github.com/jemalloc/jemalloc.

We avoid patching directly our copy.
Attachment #8818073 - Flags: review?(mh+mozilla)
Attachment #8818073 - Flags: review?(emanuel.hoogeveen)
Attachment #8818073 - Flags: review-
Attachment #8818074 - Flags: review?(mh+mozilla)
Attachment #8818074 - Flags: review?(emanuel.hoogeveen)
Attachment #8818074 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #42)
> Please submit this upstream to https://github.com/jemalloc/jemalloc.
> 
> We avoid patching directly our copy.

No problem. Doing that right away.
(In reply to John Paul Adrian Glaubitz from comment #43)
> (In reply to Mike Hommey [:glandium] from comment #42)
> > Please submit this upstream to https://github.com/jemalloc/jemalloc.
> > 
> > We avoid patching directly our copy.
> 
> No problem. Doing that right away.

Opened a PR: https://github.com/jemalloc/jemalloc/pull/529
Comment on attachment 8818076 [details] [diff] [review]
0007-Bug-1275204-mozjemalloc-Use-the-JS-arm64-allocator-o.patch

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

It rubs me in the wrong direction that this essentially duplicates code from the js engine. As in, it actually rubs me in the wrong direction that the js engine has logic to deal with things like this on its own.

It also seems like this could quickly create mmap loops depending on how the kernel actually allocates VMAs on those architectures.

::: memory/mozjemalloc/jemalloc.c
@@ +2440,5 @@
>  	}
>  #endif
>  
> +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__)
> +    const uintptr_t start = UINT64_C(0x0000070000000000);

The style in this file would be to use the ULL suffix instead of using some macro from mfbt/decimal.

@@ +2442,5 @@
>  
> +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__)
> +    const uintptr_t start = UINT64_C(0x0000070000000000);
> +    const uintptr_t end   = UINT64_C(0x0000800000000000);
> +    const uintptr_t step  = 8 << 20; /* This is supposed to be ChunkSize */

This should be the mozjemalloc chunk size here. AKA chunksize (or a multiple of it).

@@ +2444,5 @@
> +    const uintptr_t start = UINT64_C(0x0000070000000000);
> +    const uintptr_t end   = UINT64_C(0x0000800000000000);
> +    const uintptr_t step  = 8 << 20; /* This is supposed to be ChunkSize */
> +
> +    /* Copied from js/src/Memory.cpp and adapted for this source */

It's js/src/gc/Memory.cpp. Note the implementation in that file also says that it should be merged with the ia64/sparc64 netbsd implementation, and instead of adding this here, I'd rather try that.
Attachment #8818076 - Flags: review?(mh+mozilla)
Attachment #8818076 - Flags: review?(emanuel.hoogeveen)
Attachment #8818077 - Flags: review?(rjesup)
Attachment #8818077 - Flags: review?(mh+mozilla)
Attachment #8818077 - Flags: review?(emanuel.hoogeveen)
Comment on attachment 8818078 [details] [diff] [review]
0009-Bug-1273764-protobuf-Sync-sparc64-pre-processor-defi.patch

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

This was fixed upstream almost a year ago. Time to upgrade?
Attachment #8818078 - Flags: review?(mh+mozilla)
Attachment #8818078 - Flags: review?(emanuel.hoogeveen)
Attachment #8818078 - Flags: feedback?(nfitzgerald)
Comment on attachment 8818079 [details] [diff] [review]
0010-Bug-1273764-widget-gonk-Use-better-pre-processor-def.patch

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

widget/gonk is only built for b2g, which only really builds for arm/x86/mips.
Attachment #8818079 - Flags: review?(mh+mozilla)
Attachment #8818079 - Flags: review?(emanuel.hoogeveen)
Attachment #8818079 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #45)
> Comment on attachment 8818076 [details] [diff] [review]
> 0007-Bug-1275204-mozjemalloc-Use-the-JS-arm64-allocator-o.patch
> 
> Review of attachment 8818076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It rubs me in the wrong direction that this essentially duplicates code from
> the js engine. As in, it actually rubs me in the wrong direction that the js
> engine has logic to deal with things like this on its own.

True. But this is still a separate allocator and it essentially has the same problem as the JS engine, doesn't it?

> It also seems like this could quickly create mmap loops depending on how the
> kernel actually allocates VMAs on those architectures.

I haven't had any issue with that though. With all my current patches applied and the two fixes from #1322660 and #1322112, Firefox works absolutely fine on sparc64 even on Javascript-heavy websites.

> ::: memory/mozjemalloc/jemalloc.c
> @@ +2440,5 @@
> >  	}
> >  #endif
> >  
> > +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__)
> > +    const uintptr_t start = UINT64_C(0x0000070000000000);
> 
> The style in this file would be to use the ULL suffix instead of using some
> macro from mfbt/decimal.

Ok, noted.

> @@ +2442,5 @@
> >  
> > +#if defined(__sparc__) && defined(__arch64__) && defined(__linux__)
> > +    const uintptr_t start = UINT64_C(0x0000070000000000);
> > +    const uintptr_t end   = UINT64_C(0x0000800000000000);
> > +    const uintptr_t step  = 8 << 20; /* This is supposed to be ChunkSize */
> 
> This should be the mozjemalloc chunk size here. AKA chunksize (or a multiple
> of it).

Yeah, definitely. I just didn't know where to take it from, so I was hard-coding it for the time being and hoping for some additional comment.

> @@ +2444,5 @@
> > +    const uintptr_t start = UINT64_C(0x0000070000000000);
> > +    const uintptr_t end   = UINT64_C(0x0000800000000000);
> > +    const uintptr_t step  = 8 << 20; /* This is supposed to be ChunkSize */
> > +
> > +    /* Copied from js/src/Memory.cpp and adapted for this source */
> 
> It's js/src/gc/Memory.cpp. Note the implementation in that file also says
> that it should be merged with the ia64/sparc64 netbsd implementation, and
> instead of adding this here, I'd rather try that.


No, merging it with NetBSD/sparc64 would not be a good idea because the manual allocator is actually a dirty work-around and necessary on Linux only. The mmap() function in NetBSD always honors the hint parameter so that you don't have the allocating yourself to avoid getting memory from the region that is not compatible with JSValue. The manual allocator will most likely always have worse performance than anything offered by the kernel so I think we should definitely still leave NetBSD/sparc64 untouched.
(In reply to Mike Hommey [:glandium] from comment #47)
> widget/gonk is only built for b2g, which only really builds for arm/x86/mips.

I understand that. However, the defines used are still incorrect and I think it's generally better to have these fixed so if someone, for whatever reason, decided to use the code or pieces of it on sparc64, they would run into this problem again. I don't think it hurts at all to fix this bug.
Comment on attachment 8818078 [details] [diff] [review]
0009-Bug-1273764-protobuf-Sync-sparc64-pre-processor-defi.patch

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

LGTM, thanks!
Attachment #8818078 - Flags: feedback?(nfitzgerald) → feedback+
Comment on attachment 8818070 [details] [diff] [review]
0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch

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

This seems fine assuming it does what you want. r+ with the comments below addressed.

::: js/src/gc/Memory.cpp
@@ +501,4 @@
>  MapMemoryAt(void* desired, size_t length, int prot = PROT_READ | PROT_WRITE,
>              int flags = MAP_PRIVATE | MAP_ANON, int fd = -1, off_t offset = 0)
>  {
> +#if defined(__ia64__) || (defined(__sparc__) && defined(__arch64__) && (defined(__NetBSD__) || defined(__linux__))) || defined(__aarch64__)

Please split this line so it fits inside the 99 column limit. I suggest the following for clarity:

#if defined(__ia64__) || defined(__aarch64__) || \
    (defined(__sparc__) && defined(__arch64__) && (defined(__NetBSD__) || defined(__linux__)))

::: js/src/jsapi-tests/testGCAllocator.cpp
@@ +312,4 @@
>  void*
>  mapMemoryAt(void* desired, size_t length)
>  {
> +#if defined(__ia64__) || (defined(__sparc__) && defined(__arch64__) && (defined(__NetBSD__) || defined(__linux__))) || defined(__aarch64__)

Same here.
Attachment #8818070 - Flags: review?(emanuel.hoogeveen) → review+
(In reply to Mike Hommey [:glandium] from comment #45)
> It rubs me in the wrong direction that this essentially duplicates code from
> the js engine. As in, it actually rubs me in the wrong direction that the js
> engine has logic to deal with things like this on its own.

Yeah, I believe njn suggested once to switch to _aligned_malloc for all of this (I can't find the bug now), but jemalloc still doesn't deal with chunk exhaustion as well as the GC allocator does.

> It also seems like this could quickly create mmap loops depending on how the
> kernel actually allocates VMAs on those architectures.

It's not a workaround I'm particularly happy with, especially since it also potentially adds a lot of overhead on every allocation. The only ways I can see to fix it properly are to either support 48-bit addresses in the js engine (which might be doable, but IIUC it's a fair bit of work), or map large (e.g. 4GB) areas up front and allocate from those (only on 64-bit obviously).

I've considered doing the latter for the GC since it would also effectively give us separate heaps, but it would be a fairly complex change and I don't see it happening for jemalloc unless they deal with it upstream.
Attachment #8818077 - Flags: review?(rjesup) → review+
Hi!

Sorry for the long delay. I assume that after adding the requested changes to 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch, we can have this merged?

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #52)
> It's not a workaround I'm particularly happy with, especially since it also
> potentially adds a lot of overhead on every allocation. The only ways I can
> see to fix it properly are to either support 48-bit addresses in the js
> engine (which might be doable, but IIUC it's a fair bit of work), or map
> large (e.g. 4GB) areas up front and allocate from those (only on 64-bit
> obviously).

Please keep in mind that sparc64 has a 52-bit address space, so 48 free bits wouldn't be enough on this architecture.
Updated 0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch with the requested changes.
Attachment #8818070 - Attachment is obsolete: true
Attachment #8821752 - Flags: review?(emanuel.hoogeveen)
Comment on attachment 8821752 [details] [diff] [review]
0004-Bug-1275204-js-Use-the-arm64-allocator-on-Linux-spar.patch

Thanks. Looking at the raw patch, it looks like you aren't using the usual diff format. Could you check https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_diff_and_patch_files.3F and maybe https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch to make sure you're using the standard format?
Attachment #8821752 - Flags: review?(emanuel.hoogeveen) → review+
I have been working with a read-only git repository to create the patches. This has been fine in the past so far with previous patches I sent in and I can naturally use Mercurial for future patches. I hope these patches are fine for the time being though.
Alright, if you've had similar patches checked in before then it's probably fine :)
(In reply to John Paul Adrian Glaubitz from comment #49)
> (In reply to Mike Hommey [:glandium] from comment #47)
> > widget/gonk is only built for b2g, which only really builds for arm/x86/mips.
> 
> I understand that. However, the defines used are still incorrect and I think
> it's generally better to have these fixed so if someone, for whatever
> reason, decided to use the code or pieces of it on sparc64, they would run
> into this problem again. I don't think it hurts at all to fix this bug.

If someone does something with that code, they should remove that copy of sha1.c and use one of the three other SHA1 implementations we have in mozilla-central (picking the one from MFBT would be the logical choice).
Attachment #8818079 - Attachment is obsolete: true
Attachment #8818073 - Attachment is obsolete: true
Assignee: nobody → glaubitz
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b253c9b16d
Use OpenBSD/sparc64 xptcall stubs on Linux/sparc64. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/079ab3960351
ipc:chromium: Use better pre-processor defines for sparc64. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9307a83e555
js: Use better pre-processor defines for sparc64. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb0fe7e92f6
js: Use the arm64 allocator on Linux/sparc64. r=ehoogeveen
https://hg.mozilla.org/integration/mozilla-inbound/rev/71d3800a2ef3
mozjemalloc: Use better pre-processor defines for sparc64. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed065d780ed3
mozjemalloc: Use the JS arm64 allocator on Linux/sparc64. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/c320f09ac65a
media:webrtc: Use better pre-processor defines for sparc64. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d692e82582
protobuf: Sync sparc64 pre-processor defines from upstream. r=fitzgen
(In reply to John Paul Adrian Glaubitz from comment #44)
> (In reply to John Paul Adrian Glaubitz from comment #43)
> > (In reply to Mike Hommey [:glandium] from comment #42)
> > > Please submit this upstream to https://github.com/jemalloc/jemalloc.
> > > 
> > > We avoid patching directly our copy.
> > 
> > No problem. Doing that right away.
> 
> Opened a PR: https://github.com/jemalloc/jemalloc/pull/529

Update: This has been merged upstream now, see: https://github.com/jemalloc/jemalloc/commit/77de5f27d848414a6d26e86e2812339ffe1062d3

Can we pull this last patch in as well?
(In reply to John Paul Adrian Glaubitz from comment #63)
> We're still missing this patch:
> https://bug1275204.bmoattachments.org/attachment.cgi?id=8818073

Comment 42 said to submit this upstream as we don't patch the in-tree copy of jemalloc directly.
Probably best to open a new bug about pulling in the latest version of jemalloc4 in case the new version requires more local changes (I don't know when we last updated the in-tree version).
(In reply to Ryan VanderMeulen [:RyanVM] from comment #64)
> (In reply to John Paul Adrian Glaubitz from comment #63)
> > We're still missing this patch:
> > https://bug1275204.bmoattachments.org/attachment.cgi?id=8818073
> 
> Comment 42 said to submit this upstream as we don't patch the in-tree copy
> of jemalloc directly.

Ok, that happened yesterday: https://github.com/jemalloc/jemalloc/commit/77de5f27d848414a6d26e86e2812339ffe1062d3

So, I'll just have to wait until Mozilla pulls in the updated jemalloc version.
We just updated to 4.4.0 recently. Looks like your commit landed on the dev branch currently tracking jemalloc5, which AFAIK doesn't have any sort of targeted release date. However, I see that a 4.4.1 milestone was recently created as well, so you might be able to ask Jason to backport your fix to that as well.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #67)
> We just updated to 4.4.0 recently. Looks like your commit landed on the dev
> branch currently tracking jemalloc5, which AFAIK doesn't have any sort of
> targeted release date. However, I see that a 4.4.1 milestone was recently
> created as well, so you might be able to ask Jason to backport your fix to
> that as well.

I just asked: https://github.com/jemalloc/jemalloc/pull/529#issuecomment-271906286
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: