Bug 753046 (bsdipc)

Add support for DragonFly/NetBSD

RESOLVED FIXED in mozilla18

Status

()

Core
IPC
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Martin Husemann, Assigned: Jan Beich)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
x86_64
NetBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 23 obsolete attachments)

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
(Reporter)

Description

5 years ago
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?
(Reporter)

Updated

5 years ago
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 ?

Comment 4

5 years ago
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

5 years ago
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.
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.

Comment 7

5 years ago
Can you push it to tryserver? I want to see how it affects linux/macosx builds, too.

Comment 8

5 years ago
Created attachment 646836 [details] [diff] [review]
resolve conflict after Bug 776647
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 9

5 years ago
Created attachment 646844 [details] [diff] [review]
sync with process_util_linux.cc after bug 776647
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.
(Assignee)

Comment 12

5 years ago
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
Attachment #646908 - Flags: review?(martin)
Attachment #646908 - Flags: review?(landry)

Updated

5 years ago
Attachment #646836 - Attachment is obsolete: true
Attachment #646836 - Flags: review?(martin)
Attachment #646836 - Flags: review?(landry)

Updated

5 years ago
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.
(Reporter)

Comment 14

5 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.
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.
(Assignee)

Comment 17

5 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

5 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

5 years ago
Created attachment 647670 [details] [diff] [review]
define MOZ_HAVE_SHAREDMEMORYSYSV

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)
(Reporter)

Comment 21

5 years ago
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.
(Assignee)

Comment 23

5 years ago
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.
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

5 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

5 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)
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

5 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

5 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.
(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

5 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

5 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

5 years ago
Created attachment 649437 [details]
dir_reader_bsd.h
Attachment #649437 - Flags: review?(jones.chris.g)
(Assignee)

Updated

5 years ago
Attachment #649437 - Attachment is patch: false
(Assignee)

Comment 33

5 years ago
Created attachment 649439 [details]
process_util_bsd.cc
Attachment #649439 - Flags: review?(jones.chris.g)
(Assignee)

Comment 34

5 years ago
Created attachment 649448 [details] [diff] [review]
dom/plugins part
Attachment #649448 - Flags: review?(jones.chris.g)
(Assignee)

Comment 35

5 years ago
Created attachment 649452 [details] [diff] [review]
ipc/glue part
Attachment #649452 - Flags: review?(jones.chris.g)
(Assignee)

Comment 36

5 years ago
Created attachment 649458 [details] [diff] [review]
ipc/chromium part
(Assignee)

Comment 37

5 years ago
Created attachment 649459 [details] [diff] [review]
build glue
(Assignee)

Comment 38

5 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)
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.
(Assignee)

Comment 41

5 years ago
Created attachment 649720 [details] [diff] [review]
dir_reader_bsd.h file
Attachment #649437 - Attachment is obsolete: true
Attachment #649437 - Flags: review?(jones.chris.g)
Attachment #649720 - Flags: review?(jones.chris.g)
(Assignee)

Comment 42

5 years ago
Created attachment 649721 [details] [diff] [review]
process_util_bsd.cc file
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.
(Assignee)

Comment 44

5 years ago
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.
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 :)
(Assignee)

Comment 46

5 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)
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.
(Assignee)

Comment 48

5 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

5 years ago
Created attachment 650453 [details] [diff] [review]
ipc/chromium part, v3
(Assignee)

Comment 50

5 years ago
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));
		   ^
Attachment #650454 - Flags: review?(jones.chris.g)
(Assignee)

Comment 51

5 years ago
Created attachment 650455 [details] [diff] [review]
gcc 4.2.1 fixes
(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

5 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
(Assignee)

Updated

5 years ago
Attachment #649796 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #650453 - Attachment description: ipc/chromium part → ipc/chromium part, v3
(Assignee)

Comment 54

5 years ago
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
Attachment #650455 - Attachment is obsolete: true
(Assignee)

Comment 55

5 years ago
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+
(Assignee)

Comment 57

5 years ago
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
Attachment #649459 - Attachment is obsolete: true
Attachment #649459 - Flags: review?(jones.chris.g)
Attachment #651064 - Flags: review?(jones.chris.g)
(Assignee)

Comment 58

5 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)

Updated

5 years ago
Depends on: 782521
(Assignee)

Comment 59

5 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)
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+
(Assignee)

Comment 63

5 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

5 years ago
Comment on attachment 649721 [details] [diff] [review]
process_util_bsd.cc file

Oops, wrong file
Attachment #649721 - Flags: review?(landry)
(Assignee)

Updated

5 years ago
Attachment #649720 - Flags: review?(landry)
(Assignee)

Comment 65

5 years ago
Created attachment 652061 [details] [diff] [review]
cumulative for checkin
(Assignee)

Comment 66

5 years ago
Created attachment 652063 [details] [diff] [review]
cumulative diff for checkin, v2

Corrected autho after git-rebase (again) and added r=khuey (from build glue).
Attachment #652061 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #652063 - Flags: checkin?(landry)
(Assignee)

Updated

5 years ago
Attachment #650454 - Flags: checkin?(landry)
(Assignee)

Comment 67

5 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

5 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

5 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)
(Assignee)

Updated

5 years ago
Attachment #652063 - Attachment is obsolete: true
Attachment #652063 - Flags: checkin?(landry)
(Assignee)

Updated

5 years ago
Attachment #650454 - Flags: checkin?(landry)
(Assignee)

Updated

5 years ago
Depends on: 785026
(Assignee)

Comment 70

5 years ago
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
Attachment #650453 - Attachment is obsolete: true
(Assignee)

Comment 71

5 years ago
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.
(Assignee)

Comment 72

5 years ago
Created attachment 655370 [details] [diff] [review]
build glue, v3

no longer depends on file_util_linux.cc after bug 785026
Attachment #651064 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #649452 - Attachment is obsolete: true
(Assignee)

Comment 73

5 years ago
Created attachment 655372 [details] [diff] [review]
dom/plugins part
Attachment #649448 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #655368 - Flags: review?(jones.chris.g)
(Assignee)

Comment 74

5 years ago
Created attachment 655373 [details] [diff] [review]
process_util_bsd.cc file
Attachment #649721 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #655370 - Flags: review?(jones.chris.g)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 77

5 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]
Are attachment 622084 [details] [diff] [review] and attachment 650321 [details] [diff] [review] obsolete? If so, please mark them as such.
https://tbpl.mozilla.org/?tree=Try&rev=a770790e9ac3
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
(Assignee)

Comment 82

5 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.
(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....
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..)
Attachment #655373 - Attachment is obsolete: true
(Assignee)

Comment 85

5 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

5 years ago
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...
(Assignee)

Comment 88

5 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
(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..
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
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
\o/
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open for remaining patches]
(Assignee)

Updated

5 years ago
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....
(Assignee)

Comment 94

5 years ago
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?
(Assignee)

Updated

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 787588
(Assignee)

Updated

5 years ago
Attachment #656842 - Attachment is obsolete: true
Attachment #656842 - Flags: feedback?(landry)
(Assignee)

Updated

5 years ago
Blocks: 791838
(Assignee)

Updated

4 years ago
Blocks: 904848
(Assignee)

Updated

4 years ago
Depends on: 973176
You need to log in before you can comment on or make changes to this bug.