Closed Bug 753046 (bsdipc) Opened 9 years ago Closed 9 years ago

Add support for DragonFly/NetBSD

Categories

(Core :: IPC, defect)

x86_64
NetBSD
defect
Not set
normal

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?
Attachment #622084 - Attachment is patch: true
I've already merged most of my patches in bug #648735 although i still have a bunch of local patches under ipc/.
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
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)
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)
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.
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.
Attached patch ipc bsd, v3 (obsolete) — Splinter Review
(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
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.
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.
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 ?
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.
(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.
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.
Attached patch define MOZ_HAVE_SHAREDMEMORYSYSV (obsolete) — Splinter Review
use SysV shared memory on BSDs like bug 548437
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)
Indeed it is posix - and we don't have it (yuck! will fix...).
So: yes, that is a good test.
Interestingly patch v3 as-is builds fine on OpenBSD/i386. So it's probably only a 64 bits/ipc_message_utils.h issue.
Attached patch ipc bsd, v4 (obsolete) — Splinter Review
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)
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)
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)
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
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
*-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.
(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.
OK, I'm leaving EXTRA_DSO_LDOPTS hunk as is. It should still work in case OpenBSD cruft is removed/updated.
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)
Attached file dir_reader_bsd.h (obsolete) —
Attachment #649437 - Flags: review?(jones.chris.g)
Attachment #649437 - Attachment is patch: false
Attached file process_util_bsd.cc (obsolete) —
Attachment #649439 - Flags: review?(jones.chris.g)
Attached patch dom/plugins part (obsolete) — Splinter Review
Attachment #649448 - Flags: review?(jones.chris.g)
Attached patch ipc/glue part (obsolete) — Splinter Review
Attachment #649452 - Flags: review?(jones.chris.g)
Attached patch ipc/chromium part (obsolete) — Splinter Review
Attached patch build glue (obsolete) — Splinter Review
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)
Can you provide the new files as changesets too, so they can properly be imported in a mercurial queue ?
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.
Attachment #649437 - Attachment is obsolete: true
Attachment #649437 - Flags: review?(jones.chris.g)
Attachment #649720 - Flags: review?(jones.chris.g)
Attached patch process_util_bsd.cc file (obsolete) — Splinter Review
Attachment #649439 - Attachment is obsolete: true
Attachment #649439 - Flags: review?(jones.chris.g)
Attachment #649721 - Flags: review?(jones.chris.g)
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.
Attached patch ipc/chromium part, v2 (obsolete) — Splinter Review
(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
(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 :)
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)
Attached patch openbsd fixesSplinter Review
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.
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
Attached patch ipc/chromium part, v3 (obsolete) — Splinter Review
Attached patch libc++ fixesSplinter Review
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)
Attached patch gcc 4.2.1 fixes (obsolete) — Splinter Review
(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.
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
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
Can someone add to the bug Alias: bsdipc ?
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+
Attached patch build glue, v2 (obsolete) — Splinter Review
(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)
(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).
Depends on: 782521
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)
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+
Attachment #650453 - Flags: review?(jones.chris.g) → review+
Attachment #650454 - Flags: review?(jones.chris.g) → review+
Attachment #651064 - Flags: review?(jones.chris.g) → review+
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)
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)
Attached patch cumulative for checkin (obsolete) — Splinter Review
Attached patch cumulative diff for checkin, v2 (obsolete) — Splinter Review
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)
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'
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
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)
Depends on: 785026
changing ifndef in file_util_posix.cc to use HAVE_STAT64 from mozilla-config.h
Attachment #650453 - Attachment is obsolete: true
(In reply to Chris Jones [:cjones] [:warhammer] from comment #60)
> Parentheses around (&&) operands please.

Done.
Attached patch build glue, v3Splinter Review
no longer depends on file_util_linux.cc after bug 785026
Attachment #651064 - Attachment is obsolete: true
Attachment #649452 - Attachment is obsolete: true
Attached patch dom/plugins partSplinter Review
Attachment #649448 - Attachment is obsolete: true
Attachment #655368 - Flags: review?(jones.chris.g)
Attached patch process_util_bsd.cc file (obsolete) — Splinter Review
Attachment #649721 - Attachment is obsolete: true
Attachment #655370 - Flags: review?(jones.chris.g)
Assignee: nobody → jbeich
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+
Attachment #655370 - Flags: review?(jones.chris.g) → review+
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]
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.
Removing checkin-needed per ryanvm's request over irc.
Keywords: checkin-needed
(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.
(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....
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
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.
Nevermind, sys/param.h includes sys/types.h on all BSDs. So, drop ifdef and include sys/param.h unconditionally.
Sure. My build with --disable-pedantic succeeded, so going to push the 7 csets...
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
(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..
\o/
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [leave open for remaining patches]
Blocks: 683879
Target Milestone: --- → mozilla18
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....
Attached patch ref_counted.h, fix for -pedantic (obsolete) — Splinter Review
(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)
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.
Blocks: 787588
Attachment #656842 - Attachment is obsolete: true
Attachment #656842 - Flags: feedback?(landry)
Blocks: 791838
Blocks: 904848
Depends on: 973176
You need to log in before you can comment on or make changes to this bug.