Last Comment Bug 753046 - (bsdipc) Add support for DragonFly/NetBSD
(bsdipc)
: Add support for DragonFly/NetBSD
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: x86_64 NetBSD
: -- normal (vote)
: mozilla18
Assigned To: Jan Beich
:
Mentors:
Depends on: 782521 785026 973176
Blocks: 904848 683879 787588 791838
  Show dependency treegraph
 
Reported: 2012-05-08 12:19 PDT by Martin Husemann
Modified: 2014-02-15 00:33 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch adding support for various BSD OSes (NetBSD and DragonFly tested) (29.20 KB, patch)
2012-05-08 12:19 PDT, Martin Husemann
no flags Details | Diff | Splinter Review
modified to work with FreeBSD on Nightly (36.04 KB, patch)
2012-07-03 18:38 PDT, bugmenot
no flags Details | Diff | Splinter Review
check-in friendly; tested only on FreeBSD, mainly 9.x/8.x and also clang/libc++ on 10.0-CURRENT (38.69 KB, patch)
2012-07-26 04:27 PDT, bugmenot
no flags Details | Diff | Splinter Review
resolve conflict after Bug 776647 (50.11 KB, patch)
2012-07-28 04:38 PDT, bugmenot
no flags Details | Diff | Splinter Review
sync with process_util_linux.cc after bug 776647 (2.35 KB, patch)
2012-07-28 05:52 PDT, bugmenot
no flags Details | Diff | Splinter Review
ipc bsd, v3 (51.81 KB, patch)
2012-07-28 14:54 PDT, Jan Beich
no flags Details | Diff | Splinter Review
define MOZ_HAVE_SHAREDMEMORYSYSV (1009 bytes, patch)
2012-07-31 13:57 PDT, Jan Beich
no flags Details | Diff | Splinter Review
ipc bsd, v4 (52.00 KB, patch)
2012-08-01 06:03 PDT, Jan Beich
no flags Details | Diff | Splinter Review
dir_reader_bsd.h (2.18 KB, text/plain)
2012-08-06 15:36 PDT, Jan Beich
no flags Details
process_util_bsd.cc (10.84 KB, text/plain)
2012-08-06 15:39 PDT, Jan Beich
no flags Details
dom/plugins part (2.97 KB, patch)
2012-08-06 16:02 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
ipc/glue part (4.41 KB, patch)
2012-08-06 16:05 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
ipc/chromium part (23.50 KB, patch)
2012-08-06 16:08 PDT, Jan Beich
no flags Details | Diff | Splinter Review
build glue (4.44 KB, patch)
2012-08-06 16:09 PDT, Jan Beich
khuey: review+
Details | Diff | Splinter Review
dir_reader_bsd.h file (2.61 KB, patch)
2012-08-07 11:42 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
process_util_bsd.cc file (11.54 KB, patch)
2012-08-07 11:43 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
ipc/chromium part, v2 (23.20 KB, patch)
2012-08-07 13:58 PDT, Jan Beich
no flags Details | Diff | Splinter Review
openbsd fixes (5.81 KB, patch)
2012-08-08 14:38 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
ipc/chromium part, v3 (22.75 KB, patch)
2012-08-09 00:02 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
libc++ fixes (1.59 KB, patch)
2012-08-09 00:03 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
gcc 4.2.1 fixes (1.83 KB, patch)
2012-08-09 00:04 PDT, Jan Beich
no flags Details | Diff | Splinter Review
gcc 4.2.1 fixes, author *is* Landry (1.84 KB, patch)
2012-08-09 00:33 PDT, Jan Beich
no flags Details | Diff | Splinter Review
build glue, v2 (4.44 KB, patch)
2012-08-10 17:50 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
cumulative for checkin (48.13 KB, patch)
2012-08-15 03:48 PDT, Jan Beich
no flags Details | Diff | Splinter Review
cumulative diff for checkin, v2 (48.14 KB, patch)
2012-08-15 03:54 PDT, Jan Beich
no flags Details | Diff | Splinter Review
ipc/chromium part, v4 (22.15 KB, patch)
2012-08-25 15:43 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
ipc/glue part, v2 (4.42 KB, patch)
2012-08-25 15:45 PDT, Jan Beich
no flags Details | Diff | Splinter Review
build glue, v3 (4.43 KB, patch)
2012-08-25 15:47 PDT, Jan Beich
cjones.bugs: review+
Details | Diff | Splinter Review
dom/plugins part (2.95 KB, patch)
2012-08-25 15:52 PDT, Jan Beich
no flags Details | Diff | Splinter Review
process_util_bsd.cc file (11.55 KB, patch)
2012-08-25 15:53 PDT, Jan Beich
no flags Details | Diff | Splinter Review
process_util_bsd.cc file, fixed for openbsd (11.78 KB, patch)
2012-08-28 11:48 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
ref_counted.h, fix for -pedantic (725 bytes, patch)
2012-08-30 06:36 PDT, Jan Beich
no flags Details | Diff | Splinter Review

Description Martin Husemann 2012-05-08 12:19:59 PDT
Created attachment 622084 [details] [diff] [review]
patch adding support for various BSD OSes (NetBSD and DragonFly tested)

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?
Comment 1 Landry Breuil (:gaston) 2012-05-08 13:46:57 PDT
I've already merged most of my patches in bug #648735 although i still have a bunch of local patches under ipc/.
Comment 2 Landry Breuil (:gaston) 2012-05-08 23:33:01 PDT
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.
Comment 3 Landry Breuil (:gaston) 2012-05-09 11:06:17 PDT
See also bug #544377, corollary for FreeBSD. Can we coordinate both ?
Comment 4 bugmenot 2012-07-03 18:38:52 PDT
Created attachment 638936 [details] [diff] [review]
modified to work with FreeBSD on Nightly

It seems the initial patch misses quite a few files needed to build on BSDs.
Comment 5 bugmenot 2012-07-26 04:27:27 PDT
Created attachment 646085 [details] [diff] [review]
check-in friendly; tested only on FreeBSD, mainly 9.x/8.x and also clang/libc++ on 10.0-CURRENT

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.
Comment 6 Landry Breuil (:gaston) 2012-07-26 08:34:38 PDT
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.
Comment 7 bugmenot 2012-07-28 04:30:12 PDT
Can you push it to tryserver? I want to see how it affects linux/macosx builds, too.
Comment 8 bugmenot 2012-07-28 04:38:18 PDT
Created attachment 646836 [details] [diff] [review]
resolve conflict after Bug 776647
Comment 9 bugmenot 2012-07-28 05:52:36 PDT
Created attachment 646844 [details] [diff] [review]
sync with process_util_linux.cc after bug 776647
Comment 10 Landry Breuil (:gaston) 2012-07-28 06:47:19 PDT
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 Landry Breuil (:gaston) 2012-07-28 08:42:46 PDT
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.
Comment 12 Jan Beich 2012-07-28 14:54:16 PDT
Created attachment 646908 [details] [diff] [review]
ipc bsd, v3

(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
Comment 13 Landry Breuil (:gaston) 2012-07-31 13:11:43 PDT
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.
Comment 14 Martin Husemann 2012-07-31 13:16:49 PDT
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 Landry Breuil (:gaston) 2012-07-31 13:35:41 PDT
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 Landry Breuil (:gaston) 2012-07-31 13:47:56 PDT
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.
Comment 17 Jan Beich 2012-07-31 13:56:00 PDT
(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.
Comment 18 Martin Husemann 2012-07-31 13:57:09 PDT
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.
Comment 19 Jan Beich 2012-07-31 13:57:57 PDT
Created attachment 647670 [details] [diff] [review]
define MOZ_HAVE_SHAREDMEMORYSYSV

use SysV shared memory on BSDs like bug 548437
Comment 20 Landry Breuil (:gaston) 2012-08-01 00:25:26 PDT
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)
Comment 21 Martin Husemann 2012-08-01 00:39:10 PDT
Indeed it is posix - and we don't have it (yuck! will fix...).
So: yes, that is a good test.
Comment 22 Landry Breuil (:gaston) 2012-08-01 02:38:10 PDT
Interestingly patch v3 as-is builds fine on OpenBSD/i386. So it's probably only a 64 bits/ipc_message_utils.h issue.
Comment 23 Jan Beich 2012-08-01 06:03:43 PDT
Created attachment 647924 [details] [diff] [review]
ipc bsd, v4

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.
Comment 24 Martin Husemann 2012-08-01 06:16:43 PDT
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)
Comment 25 Jan Beich 2012-08-01 06:20:03 PDT
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 Landry Breuil (:gaston) 2012-08-01 13:01:48 PDT
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
Comment 27 Jan Beich 2012-08-01 14:01:18 PDT
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
Comment 28 Jan Beich 2012-08-01 14:36:54 PDT
*-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 Landry Breuil (:gaston) 2012-08-03 01:46:38 PDT
(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.
Comment 30 Jan Beich 2012-08-03 07:03:47 PDT
OK, I'm leaving EXTRA_DSO_LDOPTS hunk as is. It should still work in case OpenBSD cruft is removed/updated.
Comment 31 Jan Beich 2012-08-06 15:35:29 PDT
Comment on attachment 647924 [details] [diff] [review]
ipc bsd, v4

Time to split the patch.
Comment 32 Jan Beich 2012-08-06 15:36:53 PDT
Created attachment 649437 [details]
dir_reader_bsd.h
Comment 33 Jan Beich 2012-08-06 15:39:30 PDT
Created attachment 649439 [details]
process_util_bsd.cc
Comment 34 Jan Beich 2012-08-06 16:02:33 PDT
Created attachment 649448 [details] [diff] [review]
dom/plugins part
Comment 35 Jan Beich 2012-08-06 16:05:07 PDT
Created attachment 649452 [details] [diff] [review]
ipc/glue part
Comment 36 Jan Beich 2012-08-06 16:08:27 PDT
Created attachment 649458 [details] [diff] [review]
ipc/chromium part
Comment 37 Jan Beich 2012-08-06 16:09:12 PDT
Created attachment 649459 [details] [diff] [review]
build glue
Comment 38 Jan Beich 2012-08-06 16:23:31 PDT
Comment on attachment 649459 [details] [diff] [review]
build glue

Makefiles are unlikely to change drastically with more discussion.
Comment 39 Landry Breuil (:gaston) 2012-08-07 08:44:26 PDT
Can you provide the new files as changesets too, so they can properly be imported in a mercurial queue ?
Comment 40 Landry Breuil (:gaston) 2012-08-07 11:36:13 PDT
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.
Comment 41 Jan Beich 2012-08-07 11:42:20 PDT
Created attachment 649720 [details] [diff] [review]
dir_reader_bsd.h file
Comment 42 Jan Beich 2012-08-07 11:43:51 PDT
Created attachment 649721 [details] [diff] [review]
process_util_bsd.cc file
Comment 43 Landry Breuil (:gaston) 2012-08-07 12:55:20 PDT
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.
Comment 44 Jan Beich 2012-08-07 13:58:42 PDT
Created attachment 649796 [details] [diff] [review]
ipc/chromium part, v2

(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.
Comment 45 Landry Breuil (:gaston) 2012-08-07 14:05:37 PDT
(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 46 Jan Beich 2012-08-07 14:24:04 PDT
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.
Comment 47 Landry Breuil (:gaston) 2012-08-08 14:38:06 PDT
Created attachment 650321 [details] [diff] [review]
openbsd fixes

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 48 Jan Beich 2012-08-08 16:40:37 PDT
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
Comment 49 Jan Beich 2012-08-09 00:02:12 PDT
Created attachment 650453 [details] [diff] [review]
ipc/chromium part, v3
Comment 50 Jan Beich 2012-08-09 00:03:30 PDT
Created attachment 650454 [details] [diff] [review]
libc++ fixes

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));
		   ^
Comment 51 Jan Beich 2012-08-09 00:04:19 PDT
Created attachment 650455 [details] [diff] [review]
gcc 4.2.1 fixes
Comment 52 Landry Breuil (:gaston) 2012-08-09 00:06:48 PDT
(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.
Comment 53 Jan Beich 2012-08-09 00:14:18 PDT
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
Comment 54 Jan Beich 2012-08-09 00:33:44 PDT
Created attachment 650461 [details] [diff] [review]
gcc 4.2.1 fixes, author *is* Landry

ref_counted.h:31: error: extra ';'
file_descriptor_set_posix.h:33: error: comma at end of enumerat or list
Comment 55 Jan Beich 2012-08-09 02:34:17 PDT
Can someone add to the bug Alias: bsdipc ?
Comment 56 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-10 09:56:46 PDT
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.
Comment 57 Jan Beich 2012-08-10 17:50:24 PDT
Created attachment 651064 [details] [diff] [review]
build glue, v2

(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
Comment 58 Jan Beich 2012-08-13 21:32:29 PDT
(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).
Comment 59 Jan Beich 2012-08-14 17:56:14 PDT
Comment on attachment 650453 [details] [diff] [review]
ipc/chromium part, v3

Still works on NetBSD. I'll change author back to martin@ after review.
Comment 60 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-15 02:00:19 PDT
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.
Comment 61 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-15 02:00:52 PDT
Comment on attachment 649720 [details] [diff] [review]
dir_reader_bsd.h file

rs=me, I didn't review the code.
Comment 62 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-15 02:01:21 PDT
Comment on attachment 649721 [details] [diff] [review]
process_util_bsd.cc file

rs=me for this too
Comment 63 Jan Beich 2012-08-15 03:02:18 PDT
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.
Comment 64 Jan Beich 2012-08-15 03:03:18 PDT
Comment on attachment 649721 [details] [diff] [review]
process_util_bsd.cc file

Oops, wrong file
Comment 65 Jan Beich 2012-08-15 03:48:16 PDT
Created attachment 652061 [details] [diff] [review]
cumulative for checkin
Comment 66 Jan Beich 2012-08-15 03:54:24 PDT
Created attachment 652063 [details] [diff] [review]
cumulative diff for checkin, v2

Corrected autho after git-rebase (again) and added r=khuey (from build glue).
Comment 67 Jan Beich 2012-08-20 16:13:16 PDT
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 68 Jan Beich 2012-08-20 16:22:50 PDT
Comment on attachment 650461 [details] [diff] [review]
gcc 4.2.1 fixes, author *is* Landry

Let Landry push his -pedantic fixes himself.
Comment 69 Jan Beich 2012-08-22 15:20:48 PDT
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.
Comment 70 Jan Beich 2012-08-25 15:43:09 PDT
Created attachment 655368 [details] [diff] [review]
ipc/chromium part, v4

changing ifndef in file_util_posix.cc to use HAVE_STAT64 from mozilla-config.h
Comment 71 Jan Beich 2012-08-25 15:45:39 PDT
Created attachment 655369 [details] [diff] [review]
ipc/glue part, v2

(In reply to Chris Jones [:cjones] [:warhammer] from comment #60)
> Parentheses around (&&) operands please.

Done.
Comment 72 Jan Beich 2012-08-25 15:47:46 PDT
Created attachment 655370 [details] [diff] [review]
build glue, v3

no longer depends on file_util_linux.cc after bug 785026
Comment 73 Jan Beich 2012-08-25 15:52:00 PDT
Created attachment 655372 [details] [diff] [review]
dom/plugins part
Comment 74 Jan Beich 2012-08-25 15:53:55 PDT
Created attachment 655373 [details] [diff] [review]
process_util_bsd.cc file
Comment 75 Landry Breuil (:gaston) 2012-08-27 01:57:49 PDT
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 76 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-28 03:53:01 PDT
Comment on attachment 655368 [details] [diff] [review]
ipc/chromium part, v4

rs=me
Comment 77 Jan Beich 2012-08-28 06:41:17 PDT
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.
Comment 78 Ryan VanderMeulen [:RyanVM] 2012-08-28 07:11:23 PDT
Are attachment 622084 [details] [diff] [review] and attachment 650321 [details] [diff] [review] obsolete? If so, please mark them as such.
Comment 79 Ryan VanderMeulen [:RyanVM] 2012-08-28 07:17:40 PDT
https://tbpl.mozilla.org/?tree=Try&rev=a770790e9ac3
Comment 80 Landry Breuil (:gaston) 2012-08-28 08:51:38 PDT
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 Landry Breuil (:gaston) 2012-08-28 08:56:30 PDT
Removing checkin-needed per ryanvm's request over irc.
Comment 82 Jan Beich 2012-08-28 09:34:33 PDT
(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 Landry Breuil (:gaston) 2012-08-28 10:51:27 PDT
(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 Landry Breuil (:gaston) 2012-08-28 11:48:26 PDT
Created attachment 656134 [details] [diff] [review]
process_util_bsd.cc file, fixed for openbsd

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..)
Comment 85 Jan Beich 2012-08-28 12:03:47 PDT
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.
Comment 86 Jan Beich 2012-08-28 12:10:09 PDT
Nevermind, sys/param.h includes sys/types.h on all BSDs. So, drop ifdef and include sys/param.h unconditionally.
Comment 87 Landry Breuil (:gaston) 2012-08-28 12:53:25 PDT
Sure. My build with --disable-pedantic succeeded, so going to push the 7 csets...
Comment 88 Jan Beich 2012-08-28 13:11:20 PDT
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 Landry Breuil (:gaston) 2012-08-28 14:20:20 PDT
(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 92 Landry Breuil (:gaston) 2012-08-29 13:31:37 PDT
\o/
Comment 93 Landry Breuil (:gaston) 2012-08-30 06:16:16 PDT
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....
Comment 94 Jan Beich 2012-08-30 06:36:27 PDT
Created attachment 656842 [details] [diff] [review]
ref_counted.h, fix for -pedantic

(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?
Comment 95 Landry Breuil (:gaston) 2012-08-30 06:53:01 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.