Bug 753046 (bsdipc)

Add support for DragonFly/NetBSD

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: martin, Assigned: jbeich)

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

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

7 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

7 years ago
It seems the initial patch misses quite a few files needed to build on BSDs.

Comment 5

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

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

Comment 8

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

Comment 12

7 years ago
Posted 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)

Updated

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

Updated

7 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

7 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

7 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

7 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

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

7 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

7 years ago
Posted 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)
Reporter

Comment 24

7 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

7 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

7 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

7 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

7 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

7 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

7 years ago
Posted file dir_reader_bsd.h (obsolete) —
Attachment #649437 - Flags: review?(jones.chris.g)
Assignee

Updated

7 years ago
Attachment #649437 - Attachment is patch: false
Assignee

Comment 33

7 years ago
Posted file process_util_bsd.cc (obsolete) —
Attachment #649439 - Flags: review?(jones.chris.g)
Assignee

Comment 34

7 years ago
Posted patch dom/plugins part (obsolete) — Splinter Review
Attachment #649448 - Flags: review?(jones.chris.g)
Assignee

Comment 35

7 years ago
Posted patch ipc/glue part (obsolete) — Splinter Review
Attachment #649452 - Flags: review?(jones.chris.g)
Assignee

Comment 36

7 years ago
Posted patch ipc/chromium part (obsolete) — Splinter Review
Assignee

Comment 37

7 years ago
Posted patch build glue (obsolete) — Splinter Review
Assignee

Comment 38

7 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

7 years ago
Attachment #649437 - Attachment is obsolete: true
Attachment #649437 - Flags: review?(jones.chris.g)
Attachment #649720 - Flags: review?(jones.chris.g)
Assignee

Comment 42

7 years ago
Posted 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.
Assignee

Comment 44

7 years ago
Posted 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 :)
Assignee

Comment 46

7 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)
Posted 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.
Assignee

Comment 48

7 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

7 years ago
Posted patch ipc/chromium part, v3 (obsolete) — Splinter Review
Assignee

Comment 50

7 years ago
Posted 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)
Assignee

Comment 51

7 years ago
Posted 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.
Assignee

Comment 53

7 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

7 years ago
Attachment #649796 - Attachment is obsolete: true
Assignee

Updated

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

Comment 54

7 years ago
ref_counted.h:31: error: extra ';'
file_descriptor_set_posix.h:33: error: comma at end of enumerat or list
Attachment #650455 - Attachment is obsolete: true
Assignee

Comment 55

7 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

7 years ago
Posted 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)
Assignee

Comment 58

7 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

7 years ago
Depends on: 782521
Assignee

Comment 59

7 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

7 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

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

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

Updated

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

Comment 65

7 years ago
Posted patch cumulative for checkin (obsolete) — Splinter Review
Assignee

Comment 66

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

Updated

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

Updated

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

Comment 67

7 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

7 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

7 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

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

Updated

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

Updated

7 years ago
Depends on: 785026
Assignee

Comment 70

7 years ago
changing ifndef in file_util_posix.cc to use HAVE_STAT64 from mozilla-config.h
Attachment #650453 - Attachment is obsolete: true
Assignee

Comment 71

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

Done.
Assignee

Comment 72

7 years ago
no longer depends on file_util_linux.cc after bug 785026
Attachment #651064 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #649452 - Attachment is obsolete: true
Assignee

Comment 73

7 years ago
Attachment #649448 - Attachment is obsolete: true
Assignee

Updated

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

Comment 74

7 years ago
Posted patch process_util_bsd.cc file (obsolete) — Splinter Review
Attachment #649721 - Attachment is obsolete: true
Assignee

Updated

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

Updated

7 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

7 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]
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

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

7 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

7 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

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

Updated

7 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

7 years ago
(In reply to Landry Breuil (:gaston) from comment #93)
> I'd like to fix them but if that breaks --enable-debug builds per comment
> 67....

That's easy. But what about string16.h? Have you tried following comment 53?
Assignee

Updated

7 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

7 years ago
Blocks: 787588
Assignee

Updated

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

Updated

7 years ago
Blocks: 791838
Assignee

Updated

6 years ago
Blocks: 904848
Assignee

Updated

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