Closed
Bug 753046
(bsdipc)
Opened 12 years ago
Closed 12 years ago
Add support for DragonFly/NetBSD
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: martin, Assigned: jbeich)
References
(Blocks 1 open bug)
Details
Attachments
(9 files, 23 obsolete files)
29.20 KB,
patch
|
Details | Diff | Splinter Review | |
2.61 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
5.81 KB,
patch
|
Details | Diff | Splinter Review | |
1.59 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
22.15 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
4.42 KB,
patch
|
Details | Diff | Splinter Review | |
4.43 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
Details | Diff | Splinter Review | |
11.78 KB,
patch
|
Details | Diff | Splinter Review |
This patch introduces a OS_BSD and tries to add support common to the various BSDs to the ipc/process controll classes. We use this in pkgsrc for NetBSD and DragonFly successfully. Landry, you probably have something similar and we can merge?
Reporter | ||
Updated•12 years ago
|
Attachment #622084 -
Attachment is patch: true
Comment 1•12 years ago
|
||
I've already merged most of my patches in bug #648735 although i still have a bunch of local patches under ipc/.
Comment 2•12 years ago
|
||
Note that for proper acceptation the patch has to be against mozilla-central (so version = trunk), and it's better if it's an hg changeset.
Version: 12 Branch → Trunk
Comment 3•12 years ago
|
||
See also bug #544377, corollary for FreeBSD. Can we coordinate both ?
It seems the initial patch misses quite a few files needed to build on BSDs.
After poking sources a bit I think OpenBSD needs to link against -lkvm for kvm_open(). Can you confirm that it still works and makes sense on DragonFly/NetBSD + OpenBSD before asking for general review? FreeBSD support is implicitly reviewed by gecko@FreeBSD.org.
Attachment #638936 -
Attachment is obsolete: true
Attachment #646085 -
Flags: review?(martin)
Attachment #646085 -
Flags: review?(landry)
Comment 6•12 years ago
|
||
As it is now it doesn't build on OpenBSD: In file included from /home/landry/src/mozilla-central/ipc/chromium/src/base/hash_tables.h:20, from /home/landry/src/mozilla-central/ipc/chromium/src/base/file_path.h:72, from /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:12, from ../../dist/include/IPC/IPCMessageUtils.h:10, from /home/landry/src/mozilla-central/xpcom/io/nsMultiplexInputStream.cpp:11: /home/landry/src/mozilla-central/ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of 'extern' on explicit i nstantiations /home/landry/src/mozilla-central/ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of 'extern' on explicit i nstantiations /home/landry/src/mozilla-central/ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of 'extern' on explicit i nstantiations /home/landry/src/mozilla-central/ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of 'extern' on explicit i nstantiations In file included from /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/file_descriptor_set_posix.h:12, from /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:18, from ../../dist/include/IPC/IPCMessageUtils.h:10, from /home/landry/src/mozilla-central/xpcom/io/nsMultiplexInputStream.cpp:11: /home/landry/src/mozilla-central/ipc/chromium/src/base/ref_counted.h:31: error: extra ';' In file included from /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:18, from ../../dist/include/IPC/IPCMessageUtils.h:10, from /home/landry/src/mozilla-central/xpcom/io/nsMultiplexInputStream.cpp:11: /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/file_descriptor_set_posix.h:33: error: comma at end of enumerat or list In file included from ../../dist/include/IPC/IPCMessageUtils.h:10, from /home/landry/src/mozilla-central/xpcom/io/nsMultiplexInputStream.cpp:11: /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:277: error: redefinition of 'struct IPC::Pa ramTraits<long int>' /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:145: error: previous definition of 'struct IPC::ParamTraits<long int>' /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:291: error: redefinition of 'struct IPC::Pa ramTraits<long unsigned int>' /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:159: error: previous definition of 'struct IPC::ParamTraits<long unsigned int>' Most of those look trivial to fix; i'll see what i can do.
Can you push it to tryserver? I want to see how it affects linux/macosx builds, too.
Attachment #646085 -
Attachment is obsolete: true
Attachment #646085 -
Flags: review?(martin)
Attachment #646085 -
Flags: review?(landry)
Attachment #646836 -
Flags: review?(martin)
Attachment #646836 -
Flags: review?(landry)
Comment 10•12 years ago
|
||
Pushed att 646836 & 646844 to https://tbpl.mozilla.org/?tree=Try&rev=91a0cf25d391. Note that i didnt try it on openbsd, but it should still fail, i'll look into it.
Comment 11•12 years ago
|
||
Breaks all android/b2g builds at: /builds/slave/try-gb-armv7a-gecko/build/ipc/chromium/src/base/debug_util_posix.cc:26:24: error: sys/sysctl.h: No such file or directory #define MOZ_HAVE_EXECINFO_H change looks suspicious.
Assignee | ||
Comment 12•12 years ago
|
||
(gonna stop abusing temporary account) sysctl.h is unused on linux, not only Android. Upstream Chromium already has #if defined(OS_MACOSX) || defined(OS_BSD) #include <sys/sysctl.h> #endif I'm also switching other BSDs to posix_spawn similar to NetBSD 6. Not yet sure if the method needs dropping privs like bug 776647 (setuid, etc). ps, v2 is exposed as http://www.freebsd.org/cgi/cvsweb.cgi/ports/www/firefox/files/patch-bug753046
Attachment #646908 -
Flags: review?(martin)
Attachment #646908 -
Flags: review?(landry)
Attachment #646836 -
Attachment is obsolete: true
Attachment #646836 -
Flags: review?(martin)
Attachment #646836 -
Flags: review?(landry)
Attachment #646844 -
Attachment is obsolete: true
Comment 13•12 years ago
|
||
Note sure about three things : - what's that EXTRA_DSO_LDOPTS += -Wl,--warn-unresolved-symbols in toolkit/library/Makefile.in for ? - on netbsd and openbsd, nobody is 32767. Only dragonflybsd and freebsd seem to share 65534. - how do you actually test the posix_spawn thing ? - 'OpenBSD' #define comes from sys/param.h and is 201211 atm, but i'm told that noone should rely on it. We've had posix_spawn since less than a year (it first appeared in 5.1), but dont bother, just use defined (__OpenBSD__) imo, we dont care about backward compat with stuff older than a year anyway. Pushed that v3 to https://tbpl.mozilla.org/?tree=Try&rev=eabaec3d844d. cc'ing cjones for feedback/comments since he's the ipc maintainer.. I'm taking some time tonight to get a diff that doesnt break OpenBSD.
Reporter | ||
Comment 14•12 years ago
|
||
Testing the posix_spawn part is easy: if firefox can spawn the plugin-wrapper process, everything is fine, so if you can see any plugin displaying content, it works.
Comment 15•12 years ago
|
||
Note: There is a ipc/chromium/src/base/platform_file_posix.c change adding #include <sys/stat.h> but it's already included 10 lines upper... so is it really needed ?
Comment 16•12 years ago
|
||
The new errors i'm seeing on OpenBSD look like it's reaching previously unreached code (gotta find the cause of that) : ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of 'extern' on explicit instantiations -> that one, no idea. Probably a gcc 4.2.1 boo-boo. /home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/file_descriptor_set_posix.h:33: error: comma at end of enumerator list /home/landry/src/mozilla-central/ipc/chromium/src/base/ref_counted.h:31: error: extra ';' Those ones are easy, i've already fixed several of them in the past, because we're still on gcc 4.2.1/strict. I'd just like to understand why they appear with the v3 patchset. And finally, for those : ipc/chromium/src/chrome/common/ipc_message_utils.h:277: error: redefinition of 'struct IPC::ParamTraits<long int>' ipc/chromium/src/chrome/common/ipc_message_utils.h:145: error: previous definition of 'struct IPC::ParamTraits<long int>' ipc/chromium/src/chrome/common/ipc_message_utils.h:291: error: redefinition of 'struct IPC::ParamTraits<long unsigned int>' ipc/chromium/src/chrome/common/ipc_message_utils.h:159: error: previous definition of 'struct IPC::ParamTraits<long unsigned int>' It all comes down to that insane template maze, intermixed with an insane #ifdef maze, I don't understand why the change you make there affects openbsd/amd64. I can make the failure disappear if using #if !((defined(OS_BSD) || defined(OS_LINUX)) && defined(ARCH_CPU_64_BITS)) around the int64/uint64 templates But still, i'm not sure it wont break openbsd/i386.
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #13) > Note sure about three things : > > - what's that EXTRA_DSO_LDOPTS += -Wl,--warn-unresolved-symbols in > toolkit/library/Makefile.in for ? libxul.so has -Wl,-z,defs in LDFLAGS which makes it fail to link due to undefined symbol: environ (needed for posix_spawn). > - on netbsd and openbsd, nobody is 32767. Only dragonflybsd and freebsd seem > to share 65534. Thanks for spotting. > - how do you actually test the posix_spawn thing ? linuxflash via nspluginwrapper and gecko-mediaplayer > - 'OpenBSD' #define comes from sys/param.h and is 201211 atm, but i'm told > that noone should rely on it. We've had posix_spawn since less than a year > (it first appeared in 5.1), but dont bother, just use defined (__OpenBSD__) > imo, we dont care about backward compat with stuff older than a year anyway. > OK. (In reply to Landry Breuil (:gaston) from comment #15) > Note: There is a ipc/chromium/src/base/platform_file_posix.c change adding > #include <sys/stat.h> but it's already included 10 lines upper... so is it > really needed ? No, I've missed your commit.
Reporter | ||
Comment 18•12 years ago
|
||
The mix is indeed insane, and depends (IIRC) on the machine dependend definition of size_t. AFAICT there is no proper direct #ifdef to select all needed templates, so we will likely have to do it on a break-by-case basis and refine the ifdefs untill everyone is happy.
Assignee | ||
Comment 19•12 years ago
|
||
use SysV shared memory on BSDs like bug 548437
Comment 20•12 years ago
|
||
Note that instead of #if __FreeBSD_version > 800039 || __DragonFly_version > 200201 \ || __NetBSD_Version__ >= 599006500 || OpenBSD > 201205 #define HAVE_POSIX_SPAWN 1 #endif You can use the _POSIX_SPAWN (>1) #define from unistd.h to check for posix_spawn() presence on OpenBSD, and it seems this should be in POSIX (so also working on the other BSDs)
Reporter | ||
Comment 21•12 years ago
|
||
Indeed it is posix - and we don't have it (yuck! will fix...). So: yes, that is a good test.
Comment 22•12 years ago
|
||
Interestingly patch v3 as-is builds fine on OpenBSD/i386. So it's probably only a 64 bits/ipc_message_utils.h issue.
Assignee | ||
Comment 23•12 years ago
|
||
Martin, why not add --ignore-unresolved-symbol upstream? http://sourceware.org/bugzilla/show_bug.cgi?id=14426 (In reply to Landry Breuil (:gaston) from comment #20) > You can use the _POSIX_SPAWN (>1) #define from unistd.h to check for > posix_spawn() presence on OpenBSD, and it seems this should be in POSIX (so > also working on the other BSDs) I'm going with #if defined(_POSIX_SPAWN) && _POSIX_SPAWN > 0 it's either -1 for stub and >0 for anything else.
Attachment #646908 -
Attachment is obsolete: true
Attachment #647670 -
Attachment is obsolete: true
Attachment #646908 -
Flags: review?(martin)
Attachment #646908 -
Flags: review?(landry)
Attachment #647924 -
Flags: review?(martin)
Attachment #647924 -
Flags: review?(landry)
Reporter | ||
Comment 24•12 years ago
|
||
Interesting suggestion: --ignore-unresolved-symbol. Guess we need to wait for an official binutils version with that before we can rely on it. (Will test and review ASAP, unfortunately I am realy busy right now)
Assignee | ||
Comment 25•12 years ago
|
||
Oops, there is no Android on BSD #if (defined(OS_LINUX) || defined(OS_BSD)) && !defined(ANDROID) should be spelled as #if defined(OS_LINUX) && !defined(ANDROID) || defined(OS_BSD)
Comment 26•12 years ago
|
||
It seems -Wl,-z,defs is not used on OpenBSD. I don't see it in my build logs when linking libxul.so. I only see Wl,-z,noexecstack
Assignee | ||
Comment 27•12 years ago
|
||
Does your CC define __GNUC__ and LD has 'GNU' word in -v? Even Clang passes the following check if test "$GNU_CC"; then DSO_LDOPTS='-shared' if test "$GCC_USE_GNU_LD"; then if test -z "$MOZ_NO_WLZDEFS"; then DSO_LDOPTS="$DSO_LDOPTS -Wl,-z,defs" fi fi fi
Assignee | ||
Comment 28•12 years ago
|
||
*-freebsd*) if test `test -x /usr/bin/objformat && /usr/bin/objformat || echo elf` != "elf"; then DSO_LDOPTS="-shared" fi *-openbsd*) DSO_LDOPTS='-shared -fPIC' So, DSO_LDOPTS are not overriden for ELF binaries on FreeBSD because test elf != elf always returns 1. I'll cleanup that cruft later, support for building aout binaries was removed 10 years ago.
Comment 29•12 years ago
|
||
(In reply to Jan Beich from comment #27) > Does your CC define __GNUC__ and LD has 'GNU' word in -v? Even Clang passes > the following check > > if test "$GNU_CC"; then > DSO_LDOPTS='-shared' > if test "$GCC_USE_GNU_LD"; then > if test -z "$MOZ_NO_WLZDEFS"; then > DSO_LDOPTS="$DSO_LDOPTS -Wl,-z,defs" > fi > fi > fi $cpp -dM < /dev/null | grep GNUC__ #define __GNUC__ 4 $ld -v GNU ld version 2.15 so -z,defs is not added in the end because DSO_LDOPTS is overriden by -shared assignation in the *-openbsd*) case.
Assignee | ||
Comment 30•12 years ago
|
||
OK, I'm leaving EXTRA_DSO_LDOPTS hunk as is. It should still work in case OpenBSD cruft is removed/updated.
Assignee | ||
Comment 31•12 years ago
|
||
Comment on attachment 647924 [details] [diff] [review] ipc bsd, v4 Time to split the patch.
Attachment #647924 -
Attachment is obsolete: true
Attachment #647924 -
Flags: review?(martin)
Attachment #647924 -
Flags: review?(landry)
Assignee | ||
Comment 32•12 years ago
|
||
Attachment #649437 -
Flags: review?(jones.chris.g)
Attachment #649437 -
Attachment is patch: false
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #649439 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 34•12 years ago
|
||
Attachment #649448 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #649452 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 36•12 years ago
|
||
Assignee | ||
Comment 37•12 years ago
|
||
Assignee | ||
Comment 38•12 years ago
|
||
Comment on attachment 649459 [details] [diff] [review] build glue Makefiles are unlikely to change drastically with more discussion.
Attachment #649459 -
Flags: review?(jones.chris.g)
Comment 39•12 years ago
|
||
Can you provide the new files as changesets too, so they can properly be imported in a mercurial queue ?
Comment 40•12 years ago
|
||
Some notes : diff --git a/ipc/chromium/src/base/file_util.h b/ipc/chromium/src/base/file_util.h +#include <sys/stat.h> #include <fts.h> -#include <sys/stat.h> Is this reordering really needed ? After some brain burning, i realized that the ipc_message_utils.h failure on amd64 happened because with the current unpatched code, OS_LINUX is 1 on OpenBSD because it's the fallback in ipc/chromium/chromium-config.mk. With the change, OS_LINUX is not defined anymore.. This also leads to having PlatformThreadId in src/base/platform_thread.h undefined on OS_OPENBSD. ipc/chromium/src/base/platform_thread.h:40: error: 'PlatformThreadId' does not name a type The inconsistency/duplicity of defines in chromium-config.mk + build/build_config.h is annoying.. for example OS_BSD is not defined in the latter, and i don't know if the flags from chromium-config.mk gets propagated everywhere.
Assignee | ||
Comment 41•12 years ago
|
||
Attachment #649437 -
Attachment is obsolete: true
Attachment #649437 -
Flags: review?(jones.chris.g)
Attachment #649720 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 42•12 years ago
|
||
Attachment #649439 -
Attachment is obsolete: true
Attachment #649439 -
Flags: review?(jones.chris.g)
Attachment #649721 -
Flags: review?(jones.chris.g)
Comment 43•12 years ago
|
||
Another nit in ipc/chromium/src/chrome/common/transport_dib.h : 723 -#elif defined(OS_MACOSX) 724 +#elif defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_BSD) I'm not sure you wanted to add OS_WIN here.
Assignee | ||
Comment 44•12 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #40) > Some notes : > diff --git a/ipc/chromium/src/base/file_util.h > b/ipc/chromium/src/base/file_util.h > +#include <sys/stat.h> > #include <fts.h> > -#include <sys/stat.h> > > Is this reordering really needed ? FreeBSD used to be sensitive about include order. I'll keep the hack in the ports if it fails on earlier releases. PkgSrc people should probably do the same. Also, unistd.h in ipc_message_posix.cc is used by clang/libc++ which is planned to be default on FreeBSD 10.0-RELEASE. mozilla-central-2637d896de91/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:127:12: error: use of undeclared identifier 'dup' return dup(fd); ^ mozilla-central-2637d896de91/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:156:18: error: use of undeclared identifier 'close' HANDLE_EINTR(close(fd)); ^ ../../ipc/chromium/src/base/eintr_wrapper.h:20:10: note: expanded from macro 'HANDLE_EINTR' typeof(x) __eintr_result__; \ ^ mozilla-central-2637d896de91/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:156:18: error: use of undeclared identifier 'close' HANDLE_EINTR(close(fd)); > After some brain burning, i realized that the ipc_message_utils.h failure on > amd64 happened because with the current unpatched code, OS_LINUX is 1 on > OpenBSD because it's the fallback in ipc/chromium/chromium-config.mk. With > the change, OS_LINUX is not defined anymore.. OS_LINUX being defined on OS_OPENBSD was a bug, see https://bugzilla.mozilla.org/show_bug.cgi?id=544377#c4 As for templates I don't have an OpenBSD box to test the fix. > This also leads to having PlatformThreadId in src/base/platform_thread.h > undefined on OS_OPENBSD. > ipc/chromium/src/base/platform_thread.h:40: error: 'PlatformThreadId' does > not name a type Does intptr_t makes sense? > The inconsistency/duplicity of defines in chromium-config.mk + > build/build_config.h is annoying.. for example OS_BSD is not defined in the > latter, and i don't know if the flags from chromium-config.mk gets > propagated everywhere. OS_BSD in build_config.h is already upstream. And removing OS_FOO from build_config.h (only Mozilla) probably belongs in a separate bug. https://src.chromium.org/viewvc/chrome?view=rev&revision=109986 (In reply to Landry Breuil (:gaston) from comment #43) > Another nit in ipc/chromium/src/chrome/common/transport_dib.h : > > 723 -#elif defined(OS_MACOSX) > 724 +#elif defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_BSD) > > I'm not sure you wanted to add OS_WIN here. Thanks for spotting.
Attachment #649458 -
Attachment is obsolete: true
Comment 45•12 years ago
|
||
(In reply to Jan Beich from comment #44) > Created attachment 649796 [details] [diff] [review] > ipc/chromium part, v2 > > (In reply to Landry Breuil (:gaston) from comment #40) > > After some brain burning, i realized that the ipc_message_utils.h failure on > > amd64 happened because with the current unpatched code, OS_LINUX is 1 on > > OpenBSD because it's the fallback in ipc/chromium/chromium-config.mk. With > > the change, OS_LINUX is not defined anymore.. > > OS_LINUX being defined on OS_OPENBSD was a bug, see > > https://bugzilla.mozilla.org/show_bug.cgi?id=544377#c4 I know, i know. At that time it was an easy fix. > As for templates I don't have an OpenBSD box to test the fix. I'm on it, i have a cset that works on amd64. > > This also leads to having PlatformThreadId in src/base/platform_thread.h > > undefined on OS_OPENBSD. > > ipc/chromium/src/base/platform_thread.h:40: error: 'PlatformThreadId' does > > not name a type > > Does intptr_t makes sense? I'll use pid_t and syscall(getthrid). Again in a wip cset here. > > The inconsistency/duplicity of defines in chromium-config.mk + > > build/build_config.h is annoying.. for example OS_BSD is not defined in the > > latter, and i don't know if the flags from chromium-config.mk gets > > propagated everywhere. > > OS_BSD in build_config.h is already upstream. And removing OS_FOO from > build_config.h (only Mozilla) probably belongs in a separate bug. > > https://src.chromium.org/viewvc/chrome?view=rev&revision=109986 Given how the ipc/chromium code changed compared to upstream, i doubt we'll ever sync it again, but i hope someone (cjones ?) can prove me wrong :)
Assignee | ||
Comment 46•12 years ago
|
||
Comment on attachment 649459 [details] [diff] [review] build glue Let's see if a build peer likes config/system-headers and toolkit/library/Makefile.in changes.
Attachment #649459 -
Flags: review?(khuey)
Comment 47•12 years ago
|
||
Here's my wip patch on top of yours allowing m-c to build with clang on amd64. It fails with gcc 4.2.1 (our default) on two points : ipc/chromium/src/base/string16.h: extern template class std::basic_string<char16, base::string16_char_traits>; ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of 'extern' on explicit instantiations apparently that one 'extern template' is a c++11 feature. What i dont understand is without that pile of diffs, string16.cc is properly compiled with gcc. And it also fails on all HANDLE_EINTR uses, errors out on the first one compiled : ipc/chromium/src/base/file_descriptor_shuffle.cc:82: error: ISO C++ forbids braced-groups within expressions Similarly gcc builds it fine without the patch stack.
Assignee | ||
Comment 48•12 years ago
|
||
Comment on attachment 650321 [details] [diff] [review] openbsd fixes Review of attachment 650321 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.h @@ +29,5 @@ > // > // In debugging mode, it's a fatal error to try and add more than this number > // of descriptors to a FileDescriptorSet. > enum { > + MAX_DESCRIPTORS_PER_MESSAGE = 4 Is the fix specific to clang? What version are you using? $ clang -v FreeBSD clang version 3.1 (branches/release_31 156863) 20120523 Target: x86_64-unknown-freebsd10.0 Thread model: posix
Assignee | ||
Comment 49•12 years ago
|
||
Assignee | ||
Comment 50•12 years ago
|
||
PluginModuleChild.cpp:669:5: error: use of undeclared identifier '_exit' _exit(0); ^ ipc_channel_posix.cc:127:12: error: use of undeclared identifier 'dup' return dup(fd); ^ ipc_channel_posix.cc:156:18: error: use of undeclared identifier 'close' HANDLE_EINTR(close(fd)); ^
Attachment #650454 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 51•12 years ago
|
||
Comment 52•12 years ago
|
||
(In reply to Jan Beich from comment #48) > Comment on attachment 650321 [details] [diff] [review] > openbsd fixes > > Review of attachment 650321 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.h > @@ +29,5 @@ > > // > > // In debugging mode, it's a fatal error to try and add more than this number > > // of descriptors to a FileDescriptorSet. > > enum { > > + MAX_DESCRIPTORS_PER_MESSAGE = 4 > > Is the fix specific to clang? What version are you using? No that one is gcc-only, as for the extra ; in DFAKE_MUTEX(add_release_) Fwiw i'm offline for a week, wont be able to do more on this.
Assignee | ||
Comment 53•12 years ago
|
||
I've incorporated your patch and split compiler-specific stuff into separate parts. gcc 4.2.1 build fix needs either to be complete or not be pushed at all. (In reply to Landry Breuil (:gaston) from comment #47) > ipc/chromium/src/base/string16.h:165: error: ISO C++ forbids the use of > 'extern' on explicit instantiations > > apparently that one 'extern template' is a c++11 feature. What i dont > understand is without that pile of diffs, string16.cc is properly compiled > with gcc. Can you do a diff(1) of the defined macros before (OS_LINUX) and after (OS_BSD)? $ gcc -E -dM ... nsMultiplexInputStream.cpp
Attachment #649796 -
Attachment is obsolete: true
Attachment #650453 -
Attachment description: ipc/chromium part → ipc/chromium part, v3
Assignee | ||
Comment 54•12 years ago
|
||
ref_counted.h:31: error: extra ';' file_descriptor_set_posix.h:33: error: comma at end of enumerat or list
Attachment #650455 -
Attachment is obsolete: true
Assignee | ||
Comment 55•12 years ago
|
||
Can someone add to the bug Alias: bsdipc ?
Updated•12 years ago
|
Alias: bsdipc
Comment on attachment 649459 [details] [diff] [review] build glue Review of attachment 649459 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/library/Makefile.in @@ +531,5 @@ > endif > > +ifneq (,$(filter DragonFly FreeBSD NetBSD OpenBSD,$(OS_ARCH))) > +OS_LIBS += $(call EXPAND_LIBNAME,kvm) > +#EXTRA_DSO_LDOPTS += -Wl,--ignore-unresolved-symbol,environ Don't leave commented out lines behind.
Attachment #649459 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 57•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #56) > Don't leave commented out lines behind. OK, changed to: keep `environ' unresolved, see bug 14426 for binutils
Attachment #649459 -
Attachment is obsolete: true
Attachment #649459 -
Flags: review?(jones.chris.g)
Attachment #651064 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 58•12 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #52) > No that one is gcc-only, as for the extra ; in DFAKE_MUTEX(add_release_) Ah, those errors only happen without --disable-pedantic (used by PkgSrc and FreeBSD ports).
Assignee | ||
Comment 59•12 years ago
|
||
Comment on attachment 650453 [details] [diff] [review] ipc/chromium part, v3 Still works on NetBSD. I'll change author back to martin@ after review.
Attachment #650453 -
Flags: review?(jones.chris.g)
Updated•12 years ago
|
Attachment #649448 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 649452 [details] [diff] [review] ipc/glue part >-#if defined(OS_LINUX) && !defined(ANDROID) >+#if defined(OS_LINUX) && !defined(ANDROID) || defined(OS_BSD) Parentheses around (&&) operands please. r=me with that.
Attachment #649452 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 649720 [details] [diff] [review] dir_reader_bsd.h file rs=me, I didn't review the code.
Attachment #649720 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 649721 [details] [diff] [review] process_util_bsd.cc file rs=me for this too
Attachment #649721 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #650453 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #650454 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #651064 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 63•12 years ago
|
||
Comment on attachment 649721 [details] [diff] [review] process_util_bsd.cc file (In reply to Chris Jones [:cjones] [:warhammer] from comment #61) > Comment on attachment 649720 [details] [diff] [review] > dir_reader_bsd.h file > > rs=me, I didn't review the code. Since OpenBSD is a special case there trying Landry.
Attachment #649721 -
Flags: review?(landry)
Assignee | ||
Comment 64•12 years ago
|
||
Comment on attachment 649721 [details] [diff] [review] process_util_bsd.cc file Oops, wrong file
Attachment #649721 -
Flags: review?(landry)
Attachment #649720 -
Flags: review?(landry)
Assignee | ||
Comment 65•12 years ago
|
||
Assignee | ||
Comment 66•12 years ago
|
||
Corrected autho after git-rebase (again) and added r=khuey (from build glue).
Attachment #652061 -
Attachment is obsolete: true
Attachment #652063 -
Flags: checkin?(landry)
Attachment #650454 -
Flags: checkin?(landry)
Assignee | ||
Comment 67•12 years ago
|
||
Comment on attachment 650321 [details] [diff] [review] openbsd fixes Review of attachment 650321 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/chromium/src/base/ref_counted.h @@ +27,5 @@ > #ifndef NDEBUG > bool in_dtor_; > #endif > > + DFAKE_MUTEX(add_release_) This breaks --enable-debug build. ipc/chromium/src/base/ref_counted.h:33: error: expected ';' before 'RefCountedBase'
Assignee | ||
Comment 68•12 years ago
|
||
Comment on attachment 650461 [details] [diff] [review] gcc 4.2.1 fixes, author *is* Landry Let Landry push his -pedantic fixes himself.
Attachment #650461 -
Attachment is obsolete: true
Assignee | ||
Comment 69•12 years ago
|
||
Comment on attachment 649720 [details] [diff] [review] dir_reader_bsd.h file Nevermind, not worth blocking over. I should have mentioned that getdirentries() worked with no issues on FreeBSD.
Attachment #649720 -
Flags: review?(landry)
Attachment #652063 -
Attachment is obsolete: true
Attachment #652063 -
Flags: checkin?(landry)
Attachment #650454 -
Flags: checkin?(landry)
Assignee | ||
Comment 70•12 years ago
|
||
changing ifndef in file_util_posix.cc to use HAVE_STAT64 from mozilla-config.h
Attachment #650453 -
Attachment is obsolete: true
Assignee | ||
Comment 71•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #60) > Parentheses around (&&) operands please. Done.
Assignee | ||
Comment 72•12 years ago
|
||
no longer depends on file_util_linux.cc after bug 785026
Attachment #651064 -
Attachment is obsolete: true
Attachment #649452 -
Attachment is obsolete: true
Assignee | ||
Comment 73•12 years ago
|
||
Attachment #649448 -
Attachment is obsolete: true
Attachment #655368 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 74•12 years ago
|
||
Attachment #649721 -
Attachment is obsolete: true
Attachment #655370 -
Flags: review?(jones.chris.g)
Comment 75•12 years ago
|
||
Fwiw, i've not lost track of this, i'm just swamped by other stuff. Will definitely retry all those patches on openbsd next weekend.
Comment on attachment 655368 [details] [diff] [review] ipc/chromium part, v4 rs=me
Attachment #655368 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #655370 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 77•12 years ago
|
||
The patchset seems to be ready. There's no strict order for checkin but I'd go with the following: 1. dir_reader_bsd.h 2. process_util_bsd.cc 3. dom/plugins 4. ipc/glue 5. ipc/chromium 6. build glue 7. libc++ fixes (In reply to Landry Breuil (:gaston) from comment #75) > Fwiw, i've not lost track of this, i'm just swamped by other stuff. Will > definitely retry all those patches on openbsd next weekend. It worked with clang before. I didn't touch OpenBSD code since then.
Keywords: checkin-needed
Whiteboard: [leave open for remaining patches]
Comment 78•12 years ago
|
||
Are attachment 622084 [details] [diff] [review] and attachment 650321 [details] [diff] [review] obsolete? If so, please mark them as such.
Comment 79•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a770790e9ac3
Comment 80•12 years ago
|
||
I will take care of pushing them this weekend after making sure it doesnt break me. Pretty sure att 622084 is obsolete, and some parts of att 650321 are needed on top of the patchset to unbreak openbsd. (the other parts are not really relevant since it was for gcc 4.2). Sorry for the delay but i'm currently swamped by bug 785738.
Comment 81•12 years ago
|
||
Removing checkin-needed per ryanvm's request over irc.
Keywords: checkin-needed
Assignee | ||
Comment 82•12 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #80) > Pretty sure att 622084 is obsolete, and some parts of att 650321 are > needed on top of the patchset to unbreak openbsd. I've included non-gcc42 fixes as part of attachment 650453 [details] [diff] [review]. Remove the note from whiteboard if you don't plan to fix gcc42 + -pedantic.
Comment 83•12 years ago
|
||
(In reply to Jan Beich from comment #82) > (In reply to Landry Breuil (:gaston) from comment #80) > > Pretty sure att 622084 is obsolete, and some parts of att 650321 are > > needed on top of the patchset to unbreak openbsd. > > I've included non-gcc42 fixes as part of attachment 650453 [details] [diff] [review] Oh sorry didnt catch that. > Remove the note from whiteboard if you don't plan to fix gcc42 + -pedantic. Trying first with --disable-pedantic, but the current patchset fails on OpenBSD with : from /home/landry/src/mozilla-central/ipc/chromium/src/base/process_util_bsd.cc:10: /usr/include/sys/proc.h:65: error: 'MAXLOGNAME' was not declared in this scope /usr/include/sys/proc.h:321: error: 'MAXCOMLEN' was not declared in this scope /home/landry/src/mozilla-central/ipc/chromium/src/base/process_util_bsd.cc: In constructor 'base::NamedProcessIterator::NamedProcessIterator(const std::wstring&, const base::ProcessFilter*)': /home/landry/src/mozilla-central/ipc/chromium/src/base/process_util_bsd.cc:333: error: invalid application of 'sizeof' to incomplete type 'base::kinfo_proc2' /home/landry/src/mozilla-central/ipc/chromium/src/base/process_util_bsd.cc:333: error: 'kvm_getproc2' was not declared in this scope MAXLOGNAME comes from sys/param.h. As for kinfo_proc2/kvm_getproc2 they've been deprecated on OpenBSD, one should use kinfo_proc and kvm_getprocs. Will post a fixed process_util_bsd.cc as soon as possible. No idea why this hasnt been catched before....
Comment 84•12 years ago
|
||
This one builds on OpenBSD. The only difference from att 655373 is : @@ -18,6 +19,9 @@ +#include "base/process_util.h" + +#include <sys/types.h> ++#if defined(OS_OPENBSD) ++#include <sys/param.h> ++#endif +#include <sys/sysctl.h> +#include <sys/wait.h> +#if defined(OS_DRAGONFLY) || defined(OS_FREEBSD) @@ -341,7 +345,11 @@ +# endif +#else + kvm = kvm_open(NULL, NULL, NULL, KVM_NO_FILES, NULL); ++#if defined(OS_OPENBSD) ++ struct kinfo_proc* procs = kvm_getprocs(kvm, KERN_PROC_UID, getuid(), sizeof(struct kinfo_proc), &numEntries); ++#else + struct kinfo_proc2* procs = kvm_getproc2(kvm, KERN_PROC_UID, getuid(), sizeof(struct kinfo_proc2), &numEntries); ++#endif + if (procs != NULL && numEntries > 0) { + for (int i = 0; i < numEntries; i++) { + if (exe != procs[i].p_comm) continue; Carrying cjones's r+.. when/if my build succeeds, i'll push the whole thing. -pedantic will be dealt with in a second time, if ever (since i need to get away from gcc 4.2 anyway..)
Attachment #655373 -
Attachment is obsolete: true
Assignee | ||
Comment 85•12 years ago
|
||
Comment on attachment 656134 [details] [diff] [review] process_util_bsd.cc file, fixed for openbsd Review of attachment 656134 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/chromium/src/base/process_util_bsd.cc @@ +7,5 @@ > +#include "base/process_util.h" > + > +#include <sys/types.h> > +#if defined(OS_OPENBSD) > +#include <sys/param.h> Hmm, is style(9) broken on OpenBSD? Kernel include files (i.e., <sys/*.h>) come first; normally, you'll need <sys/types.h> OR <sys/param.h>, but not both! <sys/types.h> includes <sys/cdefs.h>, and it's okay to depend on that.
Assignee | ||
Comment 86•12 years ago
|
||
Nevermind, sys/param.h includes sys/types.h on all BSDs. So, drop ifdef and include sys/param.h unconditionally.
Comment 87•12 years ago
|
||
Sure. My build with --disable-pedantic succeeded, so going to push the 7 csets...
Assignee | ||
Comment 88•12 years ago
|
||
FYI, you might want to push almost same changes into OpenBSD ports for Firefox 15. # it'll end up in official repo soon http://trillian.chruetertee.ch/freebsd-gecko/browser/branches/experimental/www/firefox/files/patch-bug753046 Note, that patch doesn't include hunks for config/system-headers and js/src/config/system-headers as FreeBSD ports populate them via bsd.gecko.mk, they can still be grabbed separately from PkgSrc. http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/devel/xulrunner/patches/patch-config_system-headers http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/devel/xulrunner/patches/patch-js_src_config_system-headers
Comment 89•12 years ago
|
||
(In reply to Jan Beich from comment #88) > FYI, you might want to push almost same changes into OpenBSD ports for > Firefox 15. I'll patiently wait for Fx 18 to be released :) The existing code in 15 already builds unpatched & runs on OpenBSD, since i pushed all my fixes in #648735..
Comment 90•12 years ago
|
||
And pushed to inbound : https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c6016ac378 https://hg.mozilla.org/integration/mozilla-inbound/rev/f98f7ce0f3bf https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f7fe8e2363 https://hg.mozilla.org/integration/mozilla-inbound/rev/aec398b6aeb5 https://hg.mozilla.org/integration/mozilla-inbound/rev/00a80ec972d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/99bb81d74a3d https://hg.mozilla.org/integration/mozilla-inbound/rev/e86006735510 Let's see if it sticks and migrates to m-c.. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e86006735510
Comment 91•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4c6016ac378 https://hg.mozilla.org/mozilla-central/rev/f98f7ce0f3bf https://hg.mozilla.org/mozilla-central/rev/b6f7fe8e2363 https://hg.mozilla.org/mozilla-central/rev/aec398b6aeb5 https://hg.mozilla.org/mozilla-central/rev/00a80ec972d5 https://hg.mozilla.org/mozilla-central/rev/99bb81d74a3d https://hg.mozilla.org/mozilla-central/rev/e86006735510
Comment 92•12 years ago
|
||
\o/
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [leave open for remaining patches]
Updated•12 years ago
|
Target Milestone: --- → mozilla18
Comment 93•12 years ago
|
||
As for the -pedantic errors they're now warnings with gcc 4.6. ipc/chromium/src/base/ref_counted.h:31:28: warning: extra ';' [-pedantic] ipc/chromium/src/chrome/common/file_descriptor_set_posix.h:33:36: warning: comma at end of enumerator list [-pedantic] I'd like to fix them but if that breaks --enable-debug builds per comment 67....
Assignee | ||
Comment 94•12 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #93) > I'd like to fix them but if that breaks --enable-debug builds per comment > 67.... That's easy. But what about string16.h? Have you tried following comment 53?
Attachment #656842 -
Flags: feedback?(landry)
Comment 95•12 years ago
|
||
I've opened bug 787040 to deal with the warnings/pedantic. Re string16.h, i think this was a gcc 4.2 only thing, since with 4.6 it seems to work fine.
Attachment #656842 -
Attachment is obsolete: true
Attachment #656842 -
Flags: feedback?(landry)
You need to log in
before you can comment on or make changes to this bug.
Description
•