648735 - Fix ipc/chromium build on OpenBSD

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Landry Breuil (:gaston)]

Since landing of https://bugzilla.mozilla.org/show_bug.cgi?id=639754 | https://bugzilla.mozilla.org/show_bug.cgi?id=638755, trunk won't build on 'weird platforms'.

This is a tracking bug for fixes to at least let ipc/chromium from firefo x 4.0 build on OpenBSD, but atm plugin-container is of no use for us, since silverlight/flash/quicktime plugins dont exist, and java plugin doesn't work. Icedtea-web would be the first test candidate, but it's still to be ported. Note that it's only build fixes for amd64, i didn't even test npapi test plugin. There might need more fixes for i386/ppc/sparc64 (even if the latter is broken now due to fatval not being 128bits, see https://bugzilla.mozilla.org/show_bug.cgi?id=577056), it's still to be tested.

Comment 1

[Landry Breuil (:gaston)]

Attachment: backtrace/backtrace_symbols_fd is glibc-only

Not sure the fix is correct, but bsd's libc dont have backtrace()/backtrace_symbols_fd().

Comment 2

[Landry Breuil (:gaston)]

Attachment: fts.h needs sys/types.h

Fails to build with missing typedefs otherwise

Comment 3

[Landry Breuil (:gaston)]

Comment on attachment 524828 [details] [diff] [review]
backtrace/backtrace_symbols_fd is glibc-only

Oh, and add missing includes, otherwise fails with:

/usr/include/sys/proc.h:64: error: 'MAXLOGNAME' was not declared in this scope
/usr/include/sys/proc.h:287: error: 'MAXCOMLEN' was not declared in this scope

Comment 4

[Landry Breuil (:gaston)]

Comment on attachment 524829 [details] [diff] [review]
fts.h needs sys/types.h

Actual error messages:
/usr/include/fts.h:42: error: 'dev_t' does not name a type
/usr/include/fts.h:45: error: 'size_t' does not name a type
/usr/include/fts.h:73: error: 'size_t' does not name a type
/usr/include/fts.h:74: error: 'size_t' does not name a type
/usr/include/fts.h:76: error: 'ino_t' does not name a type
/usr/include/fts.h:77: error: 'dev_t' does not name a type
/usr/include/fts.h:78: error: 'nlink_t' does not name a type

Comment 5

[Landry Breuil (:gaston)]

Attachment: stat64 doesn't exist on openbsd

Patch is horrible.

Comment 6

[Landry Breuil (:gaston)]

Attachment: include sys/stat.h on openbsd

Fails with
./src/base/platform_file_posix.cc:49: error: 'S_IRUSR' was not declared in this scope
./src/base/platform_file_posix.cc:49: error: 'S_IWUSR' was not declared in this scope

Comment 7

[Landry Breuil (:gaston)]

Attachment: use getpid() on OpenBSD

To be fixed as comment states, there's maybe a way to get thread id. we don't have kernel threads yet, to be worked on.

Comment 8

[Landry Breuil (:gaston)]

Attachment: OpenBSD doesn't have vswprintf() (yet)

use vsnprintf() instead

Comment 9

[Landry Breuil (:gaston)]

Attachment: cheat and include prcpucfg_linux.h

Ugly, but quick. Instead of shipping a prcpufg_openbsd.h file, use linux's file. Cant it use a file coming from systemwide nspr ?

Comment 10

[Landry Breuil (:gaston)]

Attachment: cheat and include prcpucfg_linux.h

mh, forgot to set file type.

Comment 11

[Landry Breuil (:gaston)]

Attachment: fix #error No usable tick clock function

Strange, OS_POSIX was defined but that file doesnt seem to obey it

Comment 12

[Landry Breuil (:gaston)]

Attachment: define OS_OPENBSD/OS_POSIX and add missing sparc64 cpu arch

maybe sparc should be added too, and other oses ?
Or it's better done in chromium-config.mk ?

Comment 13

[Landry Breuil (:gaston)]

Attachment: include sys/uio.h

build fails with error: variable 'iovec iov' has initializer but incomplete type

Comment 14

[Landry Breuil (:gaston)]

Attachment: define paramTraits for unsigned long long

This one is really horrible, but the build fails with
../../ipc/chromium/src/chrome/common/ipc_message_utils.h:124:
error: 'Write' is not a member of 'IPC::ParamTraits<long long unsigned int>'

I suppose there's probably a typedef error somewhere upper, as long long unsigned int is not found in other parts of the tree. Code copypasted from a bit below. Probably only 64-bits issue.

Comment 15

[Landry Breuil (:gaston)]

There, reported all raw patches, i'm open to feedback and guidance (esp from bsmedberg, bent and cjones :) on how to make them portable enough so that they don't break other platforms, but at least everything's here to fix the build. other bsd's would probably need those fixes too.

Comment 16

[Josh Matthews [:jdm]]

CCing the three IPC folk in case they don't get all IPC mails.

Comment 17

[Landry Breuil (:gaston)]

Comment on attachment 524839 [details] [diff] [review]
define paramTraits for unsigned long long

On OpenBSD/i386 (32 bits), adding the template for struct ParamTraits<unsigned long long> creates a conflict :

../../ipc/chromium/src/chrome/common/ipc_message_utils.h:249: error: redefinition of 'struct IPC::ParamTraits<long unsigned int>'
../../ipc/chromium/src/chrome/common/ipc_message_utils.h:209: error: previous definition of 'struct IPC::ParamTraits<long unsigned int>'
../../ipc/chromium/src/chrome/common/ipc_message_utils.h:320: error: redefinition of 'struct IPC::ParamTraits<long long unsigned int>'
../../ipc/chromium/src/chrome/common/ipc_message_utils.h:223: error: previous definition of 'struct IPC::ParamTraits<long long unsigned int>'

To let it build, i've commented out the unsigned long long template, and added || defined (OS_OPENBSD) to the #if just below. Sorry, i don't really know how to handle that case properly, all those templates+all typedefs are insane.

Comment 18

[Landry Breuil (:gaston)]

Attachment: typedef Atomic32 AtomicWord on OpenBSD/i386

Fails to build on i386 (and maybe other 32-bits platforms) otherwise with 
../../ipc/chromium/src/base/singleton.h:171: error: invalid conversion from 'base::subtle::AtomicWord*' to 'volatile base::subtle::Atomic32*'

Comment 19

[Landry Breuil (:gaston)]

Comment on attachment 524828 [details] [diff] [review]
backtrace/backtrace_symbols_fd is glibc-only

That patch is wrong... either the ifndef ANDROID at the top should also be changed to be #ifdef __GLIBC__, or the configure machinery should have some --with-libexecinfo flag so that other oses can point to it, so that execinfo.h is correctly included.

Comment 20

[Landry Breuil (:gaston)]

Attachment: typedef Atomic32 AtomicWord on OpenBSD/32bits

OpenBSD/macppc needs that fix too, so make that #if defined(OS_OPENBSD) && !defined(ARCH_CPU_64_BITS)

Comment 21

[Landry Breuil (:gaston)]

(In reply to comment #17)
> Comment on attachment 524839 [details] [diff] [review]
> define paramTraits for unsigned long long
> 
> On OpenBSD/i386 (32 bits), adding the template for struct ParamTraits<unsigned
> long long> creates a conflict :
> 
> ../../ipc/chromium/src/chrome/common/ipc_message_utils.h:249: error:
> redefinition of 'struct IPC::ParamTraits<long unsigned int>'
> ../../ipc/chromium/src/chrome/common/ipc_message_utils.h:209: error: previous
> definition of 'struct IPC::ParamTraits<long unsigned int>'
> ../../ipc/chromium/src/chrome/common/ipc_message_utils.h:320: error:
> redefinition of 'struct IPC::ParamTraits<long long unsigned int>'
> ../../ipc/chromium/src/chrome/common/ipc_message_utils.h:223: error: previous
> definition of 'struct IPC::ParamTraits<long long unsigned int>'
> 
> To let it build, i've commented out the unsigned long long template, and added
> || defined (OS_OPENBSD) to the #if just below. Sorry, i don't really know how
> to handle that case properly, all those templates+all typedefs are insane.

Fwiw, OpenBSD/sparc64 needs the same fix/hack as amd64, and OpenBSD/powerpc needs the same fix as i386. See http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/3/steps/build/logs/stdio for the sparc64 failure without ip_message_utils.h fix adding struct ParamTraits<unsigned long long>

I feel that this missing unsigned long long template is a deep typedef maze issue, it should not be needed. The 32-bits 'fix' looks sane and simple though.
Plz Halp!

Comment 22

[Marco Perez [:mpk]]

Landry, some of the bits and pieces in the patch for bug #544377 might help.
It's for FreeBSD, but parts of it may be easily portable to OpenBSD.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 524839 [details] [diff] [review]
define paramTraits for unsigned long long

>$OpenBSD$
>../../ipc/chromium/src/chrome/common/ipc_message_utils.h:124:
>error: 'Write' is not a member of 'IPC::ParamTraits<long long unsigned int>'
>--- ipc/chromium/src/chrome/common/ipc_message_utils.h.orig	Thu Apr  7 22:10:35 2011
>+++ ipc/chromium/src/chrome/common/ipc_message_utils.h	Thu Apr  7 22:39:47 2011
>@@ -219,6 +219,30 @@ struct ParamTraits<unsigned long> {
>   }
> };
> 
>+template <>
>+struct ParamTraits<unsigned long long> {

I think you don't need that. What you need IMHO is to replace OS_LINUX with __GNUC__ in the following line:

> #if !(defined(OS_MACOSX) || (defined(CHROMIUM_MOZILLA_BUILD) && (defined(OS_LINUX) || defined(OS_WIN)) && defined(ARCH_CPU_64_BITS)))

Comment 24

[Landry Breuil (:gaston)]

(In reply to comment #23)
> Comment on attachment 524839 [details] [diff] [review] [review]
> define paramTraits for unsigned long long
> 
> >$OpenBSD$
> >../../ipc/chromium/src/chrome/common/ipc_message_utils.h:124:
> >error: 'Write' is not a member of 'IPC::ParamTraits<long long unsigned int>'
> >--- ipc/chromium/src/chrome/common/ipc_message_utils.h.orig	Thu Apr  7 22:10:35 2011
> >+++ ipc/chromium/src/chrome/common/ipc_message_utils.h	Thu Apr  7 22:39:47 2011
> >@@ -219,6 +219,30 @@ struct ParamTraits<unsigned long> {
> >   }
> > };
> > 
> >+template <>
> >+struct ParamTraits<unsigned long long> {
> 
> I think you don't need that. What you need IMHO is to replace OS_LINUX with
> __GNUC__ in the following line:

I've just tried that, it still fails. I need the unsigned long long template on 64 bits archs, otherwise i'm still getting
../../ipc/chromium/src/chrome/common/ipc_message_utils.h:124:
error: 'Write' is not a member of 'IPC::ParamTraits<long long unsigned int>'

Oh, and now interestingly it seems i also need a similar template for long long, otherwise build fails with :
/home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h: In static member function 'static void IPC::ParamTraits<base::Time>::Write(IPC::Message*, const base::Time&)':
/home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:392: error: 'Write' is not a member of 'IPC::ParamTraits<long long int>'

This is getting uglier and uglier....

From my understanding the #if maze below only handles size_t (unsigned long ?) and uint32, not unsigned long long nor long long. Dont ask me why the template is needed for this type too.

Comment 25

[Mike Hommey [:glandium]]

Try changing
#if !(defined(CHROMIUM_MOZILLA_BUILD) && defined(OS_LINUX) && defined(ARCH_CPU_64_BITS))
around ParamTraits<int64> and ParamTraits<uint64>

Comment 26

[Landry Breuil (:gaston)]

Hm, if i change that for
#if defined(CHROMIUM_MOZILLA_BUILD) && defined(__GNUC__) && defined(ARCH_CPU_64_BITS)
(as CHROMIUM_MOZILLA_BUILD is defined, from my understanding)

i get conflicting typedefs, which make sense, since int64 = long and uint64 = unsigned long.

The missing templates are for long long and unsigned long long. The templates for those types are instanciated nowhere, hence i need to add them. But i suppose the code calling it with ull or ll is then wrong...

Comment 27

[Landry Breuil (:gaston)]

Fwiw, i've also tried the snippet from the freebsd patch in https://bug544377.bugzilla.mozilla.org/attachment.cgi?id=527621 and it still fails.

To narrow down the failure, the full error message is :
/home/landry/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h: In function 'void IPC::WriteParam(IPC::Message*, const P&) [with P = long long unsigned int]':
../../ipc/ipdl/_ipdlheaders/mozilla/plugins/PPluginModuleParent.h:373:   instantiated from 'void mozilla::plugins::PPluginModuleParent::Write(const T&, IPC::Message*) [with T = long long unsigned int]'
/usr/obj/m-c/ipc/ipdl/PPluginModuleParent.cpp:470:   instantiated from here

Looking into the generated code in ipdl/PPluginModuleParent.cpp, we have

PPluginModuleParent::CallNPP_ClearSiteData(
        const nsCString& site,
        const uint64_t& flags,
        const uint64_t& maxAge,
        NPError* rv)

And

Write(flags, __msg);

So it looks like the problem comes from uint64_t being unsigned long long. And now i have the bad feeling that it might be vaguely related to https://bugzilla.mozilla.org/show_bug.cgi?id=599764 ... or it's just a similar issue of intermixing/overlapping typedefs.

Comment 28

[Landry Breuil (:gaston)]

Comment on attachment 524833 [details] [diff] [review]
OpenBSD doesn't have vswprintf() (yet)

Not needed anymore, OpenBSD has vswprintf now.

Comment 29

[Landry Breuil (:gaston)]

Attachment: bug-648735.1-typedef-AtomicWord-to-Atomic32-on-OpenBSD32bits

Same patch as att 529048 with proper hg commit message, ready to commit. Patch actually lifted from our chromium port, needed for OpenBSD/32 bits.

Comment 30

[Landry Breuil (:gaston)]

Attachment: bug-648735.2-add-missing-includes-in-debug_util_posix.cpp

Add missing limits.h and sys/param.h, otherwise fails with
/usr/include/sys/proc.h:64: error: 'MAXLOGNAME' was not declared in this scope
/usr/include/sys/proc.h:287: error: 'MAXCOMLEN' was not declared in this scope

Don't try to include execinfo.h/use backtrace* funcs on OpenBSD too, otherwise fails with
../../dist/system_wrappers/execinfo.h:3:27: error: execinfo.h: No such file or directory
In constructor 'StackTrace::StackTrace()':
/home/landry/src/mozilla-central/ipc/chromium/src/base/debug_util_posix.cc:125: error: 'backtrace' was not declared in this scope
/home/landry/src/mozilla-central/ipc/chromium/src/base/debug_util_posix.cc: In member function 'void StackTrace::PrintBacktrace()':
/home/landry/src/mozilla-central/ipc/chromium/src/base/debug_util_posix.cc:144: error: 'backtrace_symbols_fd' was not declared in this scope

Ready to be commited... avoids dependency on libexecinfo, since there's nothing in build-system to detect it, better #def it out on OpenBSD. Taking bug while here.

Comment 31

[Landry Breuil (:gaston)]

Attachment: bug-648735.3-add-OpenBSD-and-sparc64-defines-to-build_config.h

add define for OS_OPENBSD, consider it as an OS_POSIX, and while here add #defines for sparc64 architecture. Ready to commit.

Comment 32

[Landry Breuil (:gaston)]

Attachment: bug-648735.4-add-missing-include-sys-types-in-file_util.h

Needed, otherwise lacks a bunch of typedefs. ready to commit.

Comment 33

[Landry Breuil (:gaston)]

Attachment: bug-648735.5-add-prcpucfg_openbsd.h

Better patch that att 524835, actually bundle prcpucfg.h from our OpenBSD's nspr and include it properly.

Comment 34

[Landry Breuil (:gaston)]

Comment on attachment 524837 [details] [diff] [review]
define OS_OPENBSD/OS_POSIX and add missing sparc64 cpu arch

Obsoleted/updated by bug-648735.3-add-OpenBSD-and-sparc64-defines-to-build_config.h

Comment 35

[Landry Breuil (:gaston)]

Attachment: bug-648735.6-define-stat64-to-stat-on-bsd

Better patch than att 524830, shamelessly borrowed from FreeBSD's https://bugzilla.mozilla.org/attachment.cgi?id=527621. Handle both oses while here, ready to commit.

Comment 36

[Landry Breuil (:gaston)]

Attachment: bug-648735.7-include-header-for-IUSR

Add missing sys/stat.h inclusion on OpenBSD to define S_IRUSR and S_IWUSR

Comment 37

[Landry Breuil (:gaston)]

Attachment: bug-648735.8-use_pthread_self_in_platform_thread_posix

Finally after some tinkering, the best i could come up with was to cast pthread_self() (which returns a pthread_t ptr) to intptr_t to get an integer...

Comment 38

[Landry Breuil (:gaston)]

Attachment: bug-648735.9-include-uio-header-in-ipc_channel_posix.cc

include sys/uio.h to get the definition of struct iovec, ready to commit

Comment 39

[Landry Breuil (:gaston)]

Attachment: bug-648735.10-clock-fix-on-openbsd

On OpenBSD, clock_gettime just works fine, so let's #ifdef it. No idea from where POSIX_MONOTONIC #define comes from (only defined to 0 on android in m-c ?), so maybe not the best fix.

Comment 40

[Landry Breuil (:gaston)]

Attachment: bug-648735.11-ipc-message-utils-templates-hack

And finally, the most controversial chunk, already discussed in some earlier comments, i'd rather found out why unsigned long long gets used on OpenBSD, and not on the other oses;.. but in the meantime, this patch fixes the build on OpenBSD/64/32 bits

Comment 41

[Landry Breuil (:gaston)]

Sorry for all the spam, but i've been maintaining all those patches locally for the last past 6 months and it was painful, so lets try to get the easy parts commited for mozilla9, and debate the 'hard parts'. Btw, those are only build fixes (didn't take all the FreeBSD fixes for example), i didn't try yet to do anything useful with plugin-container nor e10s, but since that's the future let's try to move forward at least on those parts.

Comment 42

[Landry Breuil (:gaston)]

Oh, and i've sent all of this to try : https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e207768551cd

Comment 43

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 561021 [details] [diff] [review]
bug-648735.1-typedef-AtomicWord-to-Atomic32-on-OpenBSD32bits

Why is the definition of intrptr_t not being pulled in for 32-bit OpenBSD?  You're better off fixing the general problem than working around every intptr_t use.

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 561022 [details] [diff] [review]
bug-648735.2-add-missing-includes-in-debug_util_posix.cpp

># HG changeset patch
># Parent ba55770574e785c3cc3c8202596c6051073c7354
># User Landry Breuil <[e-mail]>
>Add missing limits.h and sys/param.h, otherwise fails with
>/usr/include/sys/proc.h:64: error: 'MAXLOGNAME' was not declared in this scope
>/usr/include/sys/proc.h:287: error: 'MAXCOMLEN' was not declared in this scope
>
>Don't try to include execinfo.h/use backtrace* funcs on OpenBSD too, otherwise fails with
>../../dist/system_wrappers/execinfo.h:3:27: error: execinfo.h: No such file or directory
>In constructor 'StackTrace::StackTrace()':
>/home/landry/src/mozilla-central/ipc/chromium/src/base/debug_util_posix.cc:125: error: 'backtrace' was not declared in this scope
>/home/landry/src/mozilla-central/ipc/chromium/src/base/debug_util_posix.cc: In member function 'void StackTrace::PrintBacktrace()':
>/home/landry/src/mozilla-central/ipc/chromium/src/base/debug_util_posix.cc:144: error: 'backtrace_symbols_fd' was not declared in this scope
>
>diff --git a/ipc/chromium/src/base/debug_util_posix.cc b/ipc/chromium/src/base/debug_util_posix.cc
>--- a/ipc/chromium/src/base/debug_util_posix.cc
>+++ b/ipc/chromium/src/base/debug_util_posix.cc
>@@ -3,20 +3,22 @@
> // found in the LICENSE file.
> 
> #include "build/build_config.h"
> #include "base/debug_util.h"
> 
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
>+#include <limits.h>
> #include <sys/stat.h>
>+#include <sys/param.h>
> #include <sys/types.h>
> #include <unistd.h>
>-#ifndef ANDROID
>+#if (!defined(ANDROID) && !defined(__OpenBSD__))

Let's instead do

#define MOZ_HAVE_EXECINFO_H (!defined(ANDROID) && !defined(__OpenBSD__))

and then replace the conditions below with #if !MOZ_HAVE_EXECINFO_H

r=me with nitfix.

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 561029 [details] [diff] [review]
bug-648735.6-define-stat64-to-stat-on-bsd

># HG changeset patch
># Parent 630da21dcbacb411c34613277d8b7bc0f2e26832
># User Landry Breuil <[e-mail]>
>Define stat64 to stat on Open/FreeBSD, since they don't have stat64()
>but stat handles >2Gb files fine.
>
>lifted from freebsd's www/firefox/files/patch-ipc-chromium-src-base-file_util_posix.cc
>and https://bugzilla.mozilla.org/attachment.cgi?id=527621
>
>diff --git a/ipc/chromium/src/base/file_util_posix.cc b/ipc/chromium/src/base/file_util_posix.cc
>--- a/ipc/chromium/src/base/file_util_posix.cc
>+++ b/ipc/chromium/src/base/file_util_posix.cc
>@@ -25,16 +25,21 @@
> 
> #include "base/basictypes.h"
> #include "base/eintr_wrapper.h"
> #include "base/file_path.h"
> #include "base/logging.h"
> #include "base/string_util.h"
> #include "base/time.h"
> 
>+// FreeBSD/OpenBSD lacks stat64, but its stat handles files >2GB just fine
>+#if defined(OS_FREEBSD) || defined(OS_OPENBSD)
>+#define stat64 stat
>+#endif
>+

static int
stat64(const char *path, struct stat *buf) { return stat(path, buf); }

plz.

r=me with nitfix.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 561030 [details] [diff] [review]
bug-648735.7-include-header-for-IUSR

># HG changeset patch
># Parent d3d2315348af61d0121aae80865513fa08bfff6c
># User Landry Breuil <[e-mail]>
>Include sys/stat.h on OpenBSD, otherwise fails with :
>ipc/chromium/src/base/platform_file_posix.cc:49: error: 'S_IRUSR' was not declared in this scope
>ipc/chromium/src/base/platform_file_posix.cc:49: error: 'S_IWUSR' was not declared in this scope
>
>
>diff --git a/ipc/chromium/src/base/platform_file_posix.cc b/ipc/chromium/src/base/platform_file_posix.cc
>--- a/ipc/chromium/src/base/platform_file_posix.cc
>+++ b/ipc/chromium/src/base/platform_file_posix.cc
>@@ -1,14 +1,17 @@
> // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.
> 
> #include "base/platform_file.h"
> 
>+#ifdef __OpenBSD__
>+#include <sys/stat.h>
>+#endif

Drop the #ifdef __OpenBSD__ here, it's OK to include this unconditionally.

r=me with nitfix.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 561037 [details] [diff] [review]
bug-648735.11-ipc-message-utils-templates-hack

># HG changeset patch
># Parent f0b51e7e47ba956b90b5be99cf002cbda40d924c
># User Landry Breuil <[e-mail]>
>On OpenBSD/64 bits we need a typedef for unsigned long long, and reuse the
>unsigned long template on 32/64 bits.
>
>
>diff --git a/ipc/chromium/src/chrome/common/ipc_message_utils.h b/ipc/chromium/src/chrome/common/ipc_message_utils.h
>--- a/ipc/chromium/src/chrome/common/ipc_message_utils.h
>+++ b/ipc/chromium/src/chrome/common/ipc_message_utils.h
>@@ -163,18 +163,43 @@ struct ParamTraits<unsigned long> {
>   }
>   static bool Read(const Message* m, void** iter, param_type* r) {
>     return m->ReadULong(iter, r);
>   }
>   static void Log(const param_type& p, std::wstring* l) {
>     l->append(StringPrintf(L"%ul", p));
>   }
> };
>+#if (defined(OS_OPENBSD) && defined(ARCH_CPU_64_BITS))
>+//XXX HACK

If size_t is defined to |unsigned long long| there, this is what we have to do to serialize it properly.  That's not a hack so you can remove this comment.

r=me with nitfix.

Comment 48

[Landry Breuil (:gaston)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #45)
> Comment on attachment 561029 [details] [diff] [review]
> bug-648735.6-define-stat64-to-stat-on-bsd
> 
> ># HG changeset patch
> ># Parent 630da21dcbacb411c34613277d8b7bc0f2e26832
> ># User Landry Breuil <[e-mail]>
> >Define stat64 to stat on Open/FreeBSD, since they don't have stat64()
> >but stat handles >2Gb files fine.
> >
> >lifted from freebsd's www/firefox/files/patch-ipc-chromium-src-base-file_util_posix.cc
> >and https://bugzilla.mozilla.org/attachment.cgi?id=527621
> >
> >diff --git a/ipc/chromium/src/base/file_util_posix.cc b/ipc/chromium/src/base/file_util_posix.cc
> >--- a/ipc/chromium/src/base/file_util_posix.cc
> >+++ b/ipc/chromium/src/base/file_util_posix.cc
> >@@ -25,16 +25,21 @@
> > 
> > #include "base/basictypes.h"
> > #include "base/eintr_wrapper.h"
> > #include "base/file_path.h"
> > #include "base/logging.h"
> > #include "base/string_util.h"
> > #include "base/time.h"
> > 
> >+// FreeBSD/OpenBSD lacks stat64, but its stat handles files >2GB just fine
> >+#if defined(OS_FREEBSD) || defined(OS_OPENBSD)
> >+#define stat64 stat
> >+#endif
> >+
> 
> static int
> stat64(const char *path, struct stat *buf) { return stat(path, buf); }
> 
> plz.
> 
> r=me with nitfix.

If i do it that way, i might also need to typedef the struct, otherwise it fails with :

/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:80: error: aggregate 'file_util::stat64 st' has incomplete type and cannot be defined
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:81: error: invalid use of incomplete type 'struct file_util::stat64'
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:80: error: forward declaration of 'struct file_util::stat64'

Trying with typedef stat64 stat in addition to static int stat64().

Comment 49

[Landry Breuil (:gaston)]

And it seems that can't work. With that code:

// FreeBSD/OpenBSD lacks stat64, but its stat handles files >2GB just fine
#if defined(OS_FREEBSD) || defined(OS_OPENBSD)
typedef struct stat stat64;
static int
stat64(const char *path, struct stat *buf) { return stat(path, buf); }
#endif


it explodes with a clash :

/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc: In function 'int stat64(const char*, stat*)':
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:37: error: 'int stat64(const char*, stat*)' redeclared as different kind of symbol
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:35: error: previous declaration of 'typedef struct stat stat64'
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc: In function 'int file_util::CountFilesCreatedAfter(const FilePath&, const base::Time&)':
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:81: error: using typedef-name 'stat64' after 'struct'
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:35: error: 'stat64' has a previous declaration here
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:81: error: invalid type in declaration before ';' token
/home/landry/src/mozilla-central/ipc/chromium/src/base/file_util_posix.cc:82: error: no matching function for call to 'stat::stat(const char*, int*)'


So i'm afraid the #define might be better, even if it's ugly... after all, that's a construct often found when porting software to the bsds, and even opensolaris headers have it, see

http://fxr.watson.org/fxr/source/common/sys/stat.h?v=OPENSOLARIS;im=3#L199

Comment 50

[Landry Breuil (:gaston)]

Attachment: bug-648735.2-add-missing-includes-in-debug_util_posix.cpp

Address nits from comment #44, reasking review since i don't know how to carry r+ across different versions of a patch..

Comment 51

[Landry Breuil (:gaston)]

Attachment: bug-648735.7-include-header-for-IUSR

unconditionally include sys/types.h, from comment #46

Comment 52

[Landry Breuil (:gaston)]

Attachment: bug-648735.11-ipc-message-utils-templates-hack

Same patch, better comment : on OpenBSD, uint64_t provided by sys/types.h is unsigned long long, hence the need for that template, apparently unrelated to size_t.

Comment 53

[Landry Breuil (:gaston)]

I've done a build with those last patches, and the previous patches went on the try servers without issues. https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e207768551cd.

Setting checkin-needed for the r+'ed patches, only need to rework patch #1 for AtomicWord.

Comment 54

[Landry Breuil (:gaston)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #43)
> Comment on attachment 561021 [details] [diff] [review]
> bug-648735.1-typedef-AtomicWord-to-Atomic32-on-OpenBSD32bits
> 
> Why is the definition of intrptr_t not being pulled in for 32-bit OpenBSD? 
> You're better off fixing the general problem than working around every
> intptr_t use.

on OpenBSD, intptr_t is unsigned long, which is 32 bits on 32 bits archs and 64 bits on 64 bits archs. If i don't force-typedef AtomicWord to Atomic32 on 32 bits archs, it fails later on with :

../../ipc/chromium/src/base/singleton.h:171: error: invalid conversion from 'base::subtle::AtomicWord*' to 'volatile base::subtle::Atomic32*'

Since there seem to be quite a lot of casts, going the Atomic32 route looked simpler.. sorry, i don't have an explanation for that error.

Comment 55

[Ed Morley [:emorley]]

Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/f44151e039f6
Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b19bf3e0b69
Part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea33ddde6ade
Part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/f78d9f4f5234
Part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb924724a032
Part 9: https://hg.mozilla.org/integration/mozilla-inbound/rev/226136c5812e
Part 10: https://hg.mozilla.org/integration/mozilla-inbound/rev/7039bba56b10

Leaving open for parts 1, 2, 7 & 11.

Comment 56

[Landry Breuil (:gaston)]

(In reply to Landry Breuil from comment #54)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #43)
> > Comment on attachment 561021 [details] [diff] [review]
> > bug-648735.1-typedef-AtomicWord-to-Atomic32-on-OpenBSD32bits
> > 
> > Why is the definition of intrptr_t not being pulled in for 32-bit OpenBSD? 
> > You're better off fixing the general problem than working around every
> > intptr_t use.
> 
> on OpenBSD, intptr_t is unsigned long, which is 32 bits on 32 bits archs and
> 64 bits on 64 bits archs. If i don't force-typedef AtomicWord to Atomic32 on
> 32 bits archs, it fails later on with :
> 
> ../../ipc/chromium/src/base/singleton.h:171: error: invalid conversion from
> 'base::subtle::AtomicWord*' to 'volatile base::subtle::Atomic32*'
> 
> Since there seem to be quite a lot of casts, going the Atomic32 route looked
> simpler.. sorry, i don't have an explanation for that error.

Replying to myself as i fiddled with that code :

 #include <stdint.h>
 #include <stdio.h>
typedef int32_t Atomic32;
typedef intptr_t AtomicWord;
 int main()
 {
 printf("Atomic32 %X AtomicWord %X\n", sizeof(Atomic32), sizeof(AtomicWord));
 return 0;
 }

On 32 bits, this gives "Atomic32 4 AtomicWord 4", on 64 bits this gives "Atomic32 4 AtomicWord 8". So i don't see what's wrong with the cast. And yes, i used 'int32_t' since int32 is a non-std type...

Comment 57

[Landry Breuil (:gaston)]

Awesome, thanks chris. Re setting checkin-needed for 4 last chunks.

Comment 58

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f44151e039f6
https://hg.mozilla.org/mozilla-central/rev/3b19bf3e0b69
https://hg.mozilla.org/mozilla-central/rev/ea33ddde6ade
https://hg.mozilla.org/mozilla-central/rev/f78d9f4f5234
https://hg.mozilla.org/mozilla-central/rev/bb924724a032
https://hg.mozilla.org/mozilla-central/rev/226136c5812e
https://hg.mozilla.org/mozilla-central/rev/7039bba56b10

leavin open for the remaining parts.

Comment 59

[Ed Morley [:emorley]]

Parts 1, 2, 7 & 11:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67ec721fdd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/86601793e2ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/156ededf3287
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f840c23347

(All done here now, can be closed once these four merge).

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c67ec721fdd6
https://hg.mozilla.org/mozilla-central/rev/86601793e2ce
https://hg.mozilla.org/mozilla-central/rev/156ededf3287
https://hg.mozilla.org/mozilla-central/rev/f5f840c23347

Comment 61

[Landry Breuil (:gaston)]

Awesomeness, ponies and unicorns. Thanks to all involved, i'll celebrate that one :)