User-Agent: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.9.2.2) Gecko/20100325 IceCat/3.6.2 (like Firefox/3.6.2) Build Identifier: http://releases.mozilla.org/pub/mozilla.org/firefox/releases/3.6.4/source/firefox-3.6.4.source.tar.bz2 ../../ipc/chromium/src/build/build_config.h:61:2: error: #error Please add support for your architecture in build/build_config.h from nsXPComInit.cpp:152: ../../ipc/chromium/src/base/atomicops.h:136:2: error: #error "Atomic operations are not supported on your platform" make[4]: *** [nsXPComInit.o] Error 1 Firefox 3.6.2 builds perfectly. Your System Requirements page doesn't declare GNU/Linux version as x86 (and x86_64, amd64, em64t) only. But you're using x86-only thing from chromium called "Out-of-process plugins" (http://benjamin.smedbergs.us/blog/2010-01-27/multi-process-plugins-on-by-default/). Currently it can be only disabled by adding "ac_add_options --disable-ipc" to .mozconfig (really very hard way). Your configuration scripts don't bother about target's CPU architecture. Solution: if you don't want to support anything onther than x86 on GNU/Linux, then at least add auto-disabling of ipc to your configuration scripts. Reproducible: Always Steps to Reproduce: 1. Get vanilla 3.6.4 (and maybe later version too) tarball. 2. Use PowerPC GNU/Linux at least as target platfrom in your toolchain. 3. ./configure && make Actual Results: make[4]: *** [nsXPComInit.o] Error 1 Expected Results: Firefox builds normally.

We want somebody to provide the correct atomic primitives for the IPC code: we're going to use this code more and more for out-of-process tabs and extensions. I would accept a patch which errors out at configure time and informs you of the --disable-ipc workaround until somebody has written that patch, but I definitely do not want to automatically --disable-ipc.

Maybe this may help: http://lxr.free-electrons.com/source/include/asm-powerpc/atomic.h?v=2.6.25;a=powerpc ---- Also, patch which tells the truth: @@ -133,7 +133,7 @@ #elif defined(COMPILER_GCC) && defined(ARCH_CPU_ARM_FAMILY) #include "base/atomicops_internals_arm_gcc.h" #else -#error "Atomic operations are not supported on your platform" +#error "Atomic operations are not supported by Google Chrome/Chromium team on your platform, despite the fact your platform maybe much more superior in real-time atomic operations that the supported one(s)" #endif #endif // BASE_ATOMICOPS_H_ ---- > errors out at configure time and informs you of the --disable-ipc workaround Yep, it will be sufficient too. Just don't leave users without any information and possible workaround(s).

See attachment 443622 [details] [diff] [review] in bug 563605, and bug 563605 comment 11. Note that all platforms on trunk have a fallback using mutexes, so while this is not perfect in that it doesn't use atomic operations, it works.

Hi, I made a patch for atomic operation on powerpc. The only thing left is making data_pack.cc to be endianness-independent: If someone could help me... Thanks

In the linker phase I got this: `.text._ZN4base6subtle23Barrier_AtomicIncrementEPVii' referenced in section `__lwsync_fixup' of ../../staticlib/libchromium_s.a(thread_collision_warner.o): defined in discarded section `.text._ZN4base6subtle23Barrier_AtomicIncrementEPVii[base::subtle::Barrier_AtomicIncrement(int volatile*, int)]' of ../../staticlib/libchromium_s.a(thread_collision_warner.o) collect2: ld returned 1 exit status Compiling phase is OK. How could I fix? Is it a gcc problem?

Now Xulrunner and Firefox compile fine. Someone needs to check endianness in data_pack.cc. There is a testcase for this?

data_pack.cc is not used as stated in bug 587188. It'll get removed from compilation there. NB: For trunk or aurora the patch is not enough as the atomicops*mutex stuff needs to get removed from build otherwise the header inclusion conflicts with the source files.

(In reply to comment #7) > data_pack.cc is not used as stated in bug 587188. It'll get removed from > compilation there. > > NB: For trunk or aurora the patch is not enough > as the atomicops*mutex stuff needs to get removed from build otherwise the > header inclusion conflicts with the source files. Actually, on trunk or aurora, ipc works on PPC because there is now a mutex fallback for platforms we don't have atomic ops for. So on trunk or aurora, all that is required, really, is to add the ppc specific atomic ops.

err, I already had said so in comment 3.

Should I rewrote my patch to fit on trunk, ignoring data_pack.cc ? thanks

Yes, please. At this point I don't think we're not going to take patches for 3.6, 4.0, or even aurora.

I rewrote the patch. It works here, ArchLinux PPC, Firefox 4.0.1 and Firefox Trunk. Should I open a new bug report for trunk, as this reported for 3.6.x series? Thanks

(In reply to comment #12) > Created attachment 530442 [details] [diff] [review] [review] > Rewrote previous patch targeting trunk > > I rewrote the patch. > It works here, ArchLinux PPC, Firefox 4.0.1 and Firefox Trunk. > > Should I open a new bug report for trunk, as this reported for 3.6.x series? > > Thanks All patches need to be against trunk, there are a few issues I believe need to be addressed tho. diff -r 3ff945bdace7 ipc/chromium/src/base/data_pack.cc --- a/ipc/chromium/src/base/data_pack.cc Wed May 04 15:39:12 2011 +0800 +++ b/ipc/chromium/src/base/data_pack.cc Wed May 04 11:20:40 2011 +0200 @@ -91,15 +91,7 @@ bool DataPack::Get(uint32_t resource_id, StringPiece* data) { // It won't be hard to make this endian-agnostic, but it's not worth // bothering to do right now. -#if defined(__BYTE_ORDER) - // Linux check - COMPILE_ASSERT(__BYTE_ORDER == __LITTLE_ENDIAN, - datapack_assumes_little_endian); -#elif defined(__BIG_ENDIAN__) - // Mac check - #error DataPack assumes little endian -#endif - +#warning DoTheRightThingMakingThisEndianAgnostic! DataPackEntry* target = reinterpret_cast<DataPackEntry*>( bsearch(&resource_id, mmap_->data() + kHeaderLength, resource_count_, sizeof(DataPackEntry), DataPackEntry::CompareById)); This is not needed, data_pack.cc is not used in the build system, the build on trunk has already removed the file from building. diff -r 3ff945bdace7 ipc/chromium/src/base/debug_util_posix.cc --- a/ipc/chromium/src/base/debug_util_posix.cc Wed May 04 15:39:12 2011 +0800 +++ b/ipc/chromium/src/base/debug_util_posix.cc Wed May 04 11:20:40 2011 +0200 @@ -110,8 +110,10 @@ // static void DebugUtil::BreakDebugger() { -#if defined(ARCH_CPU_X86_FAMILY) +#if defined(ARCH_CPU_X86_FAMILY) && !defined(ARCH_CPU_PPC) asm ("int3"); +elif defined(ARCH_CPU_PPC) + asm ("trap"); #endif } #elif define(ARCH_CPU_PPC) would be proper, without will result in failure. --- a/ipc/chromium/src/build/build_config.h 2011-04-14 07:28:30.000000000 +0200 +++ b/ipc/chromium/src/build/build_config.h 2011-05-04 21:01:32.000000000 +0200 @@ -57,7 +57,7 @@ #define ARCH_CPU_ARMEL 1 #define ARCH_CPU_32_BITS 1 #define WCHAR_T_IS_UNSIGNED 1 -#elif defined(__ppc__) +#elif defined(__ppc__) || defined(__powerpc__) #define ARCH_CPU_PPC 1 #define ARCH_CPU_32_BITS 1 #else Already been addressed on trunk. Other then that patch works as expected for ppc users :) Great work.

Is this patch reviewable? I'd like to see it land so we can work with it in TenFourFox. DaNiMoTh, if you need help setting review flags, say so (or if your patch isn't ready yet, I'll grab it myself and work with it internally). Since this is technically NPOTB, it should be straightforward to approve.

I don't have much time to work on this, but (as you see) the modifications to do are little and easy (sync to trunk and delete the data_pack.cc part). So, I'm happy if you could fix, repost here for inclusion into main tree, and then apply back to TenFourFox :) Many thanks

(In reply to comment #15) > I don't have much time to work on this, but (as you see) the modifications > to do are little and easy (sync to trunk and delete the data_pack.cc part). > > So, I'm happy if you could fix, repost here for inclusion into main tree, > and then apply back to TenFourFox :) > > Many thanks I will bring the patch up to speed and have it up for review in 30 minutes.

All up to date, with minor corrections that I have pointed out in previous post.

Comment on attachment 534400 [details] [diff] [review] add atomic operations for ppc I don't know PPC at all. cjones may be able to review this, or we may look for a PPC expert.

I haven't started working with this yet, but the instructions look correct to me, for the benefit of whomever reviews this (Mike or Chris?).

(bugspam, sorry) I just noticed you're using lwsync. This isn't something all PPCs have; a plain sync is slower but probably safer. See, also, http://code.google.com/p/google-perftools/source/browse/trunk/src/base/atomicops-internals-linuxppc.h

Yes, lwsync is something out-of-ppc; just use nop for it; there's no need to do anything if you don't want to be isync'ed. Use any ALU nop you like (nor r0,r0,r0; addi r0,r0,0; etc.) instead.

Comment on attachment 534400 [details] [diff] [review] add atomic operations for ppc Yeah, I'm not comfortable enough with the PPC ISA to review this without spending some in the manual. Two questions (1) What's wrong with the mutex fallback? Do you have profile data showing we need these atomic ops? The mutex fallback is safer... (2) Is there any reason for us not to take http://code.google.com/p/google-perftools/source/browse/trunk/src/base/atomicops-internals-linuxppc.h wholesale? Is it really linux specific? This patch looks different. I'm OK rubber-stamping that code. Clearing review request pending answers.

(3) why must be the atomic ops re-implemented every time in assembler instead of using existing primitives provided by GCC (in earlier versions ) or by C/C++ standards (in recent GCC)?

I think we can close this now, due to the work in bug 643112.