The default bug view has changed. See this FAQ.

Fix ipc/chromium build on OpenBSD

RESOLVED FIXED in mozilla9

Status

()

Core
IPC
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

Trunk
mozilla9
All
OpenBSD
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 17 obsolete attachments)

1.11 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
1.67 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
1.02 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
13.75 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
1.08 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
920 bytes, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
878 bytes, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
1.22 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
2.70 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
973 bytes, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
2.09 KB, patch
cjones
: review+
emorley
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 524828 [details] [diff] [review]
backtrace/backtrace_symbols_fd is glibc-only

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

Comment 2

6 years ago
Created attachment 524829 [details] [diff] [review]
fts.h needs sys/types.h

Fails to build with missing typedefs otherwise
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

6 years ago
Created attachment 524830 [details] [diff] [review]
stat64 doesn't exist on openbsd

Patch is horrible.
(Assignee)

Comment 6

6 years ago
Created attachment 524831 [details] [diff] [review]
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
(Assignee)

Comment 7

6 years ago
Created attachment 524832 [details] [diff] [review]
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.
(Assignee)

Comment 8

6 years ago
Created attachment 524833 [details] [diff] [review]
OpenBSD doesn't have vswprintf() (yet)

use vsnprintf() instead
(Assignee)

Comment 9

6 years ago
Created attachment 524834 [details]
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 ?
(Assignee)

Comment 10

6 years ago
Created attachment 524835 [details] [diff] [review]
cheat and include prcpucfg_linux.h

mh, forgot to set file type.
Attachment #524834 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 524836 [details] [diff] [review]
fix #error No usable tick clock function

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

Comment 12

6 years ago
Created attachment 524837 [details] [diff] [review]
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 ?
(Assignee)

Comment 13

6 years ago
Created attachment 524838 [details] [diff] [review]
include sys/uio.h

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

Comment 14

6 years ago
Created attachment 524839 [details] [diff] [review]
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.
(Assignee)

Comment 15

6 years ago
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.
CCing the three IPC folk in case they don't get all IPC mails.
(Assignee)

Comment 17

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

Comment 18

6 years ago
Created attachment 524927 [details] [diff] [review]
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*'
(Assignee)

Updated

6 years ago
Blocks: 650665
(Assignee)

Comment 19

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

Comment 20

6 years ago
Created attachment 529048 [details] [diff] [review]
typedef Atomic32 AtomicWord on OpenBSD/32bits

OpenBSD/macppc needs that fix too, so make that #if defined(OS_OPENBSD) && !defined(ARCH_CPU_64_BITS)
Attachment #524927 - Attachment is obsolete: true
(Assignee)

Comment 21

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

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

Comment 24

6 years ago
(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.
Try changing
#if !(defined(CHROMIUM_MOZILLA_BUILD) && defined(OS_LINUX) && defined(ARCH_CPU_64_BITS))
around ParamTraits<int64> and ParamTraits<uint64>
(Assignee)

Comment 26

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

Comment 27

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

Comment 28

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

Not needed anymore, OpenBSD has vswprintf now.
Attachment #524833 - Attachment is obsolete: true
(Assignee)

Comment 29

6 years ago
Created attachment 561021 [details] [diff] [review]
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.
Attachment #529048 - Attachment is obsolete: true
Attachment #561021 - Flags: review?(jones.chris.g)
(Assignee)

Comment 30

6 years ago
Created attachment 561022 [details] [diff] [review]
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.
Assignee: nobody → landry
Attachment #524828 - Attachment is obsolete: true
Attachment #561022 - Flags: review?(jones.chris.g)
(Assignee)

Comment 31

6 years ago
Created attachment 561024 [details] [diff] [review]
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.
Attachment #561024 - Flags: review?(jones.chris.g)
(Assignee)

Comment 32

6 years ago
Created attachment 561026 [details] [diff] [review]
bug-648735.4-add-missing-include-sys-types-in-file_util.h

Needed, otherwise lacks a bunch of typedefs. ready to commit.
Attachment #524829 - Attachment is obsolete: true
Attachment #561026 - Flags: review?(jones.chris.g)
(Assignee)

Comment 33

6 years ago
Created attachment 561028 [details] [diff] [review]
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.
Attachment #524835 - Attachment is obsolete: true
Attachment #561028 - Flags: review?(jones.chris.g)
(Assignee)

Comment 34

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

Comment 35

6 years ago
Created attachment 561029 [details] [diff] [review]
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.
Attachment #524830 - Attachment is obsolete: true
Attachment #561029 - Flags: review?(jones.chris.g)
(Assignee)

Comment 36

6 years ago
Created attachment 561030 [details] [diff] [review]
bug-648735.7-include-header-for-IUSR

Add missing sys/stat.h inclusion on OpenBSD to define S_IRUSR and S_IWUSR
Attachment #524831 - Attachment is obsolete: true
Attachment #561030 - Flags: review?(jones.chris.g)
(Assignee)

Comment 37

6 years ago
Created attachment 561034 [details] [diff] [review]
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...
Attachment #524832 - Attachment is obsolete: true
Attachment #561034 - Flags: review?(jones.chris.g)
(Assignee)

Comment 38

6 years ago
Created attachment 561035 [details] [diff] [review]
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
Attachment #524838 - Attachment is obsolete: true
Attachment #561035 - Flags: review?(jones.chris.g)
(Assignee)

Comment 39

6 years ago
Created attachment 561036 [details] [diff] [review]
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.
Attachment #524836 - Attachment is obsolete: true
Attachment #561036 - Flags: review?(jones.chris.g)
(Assignee)

Comment 40

6 years ago
Created attachment 561037 [details] [diff] [review]
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
Attachment #524839 - Attachment is obsolete: true
Attachment #561037 - Flags: review?(jones.chris.g)
(Assignee)

Comment 41

6 years ago
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.
Target Milestone: --- → mozilla9
(Assignee)

Comment 42

6 years ago
Oh, and i've sent all of this to try : https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e207768551cd
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 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 <landry@openbsd.org>
>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.
Attachment #561022 - Flags: review?(jones.chris.g) → review+
Attachment #561024 - Flags: review?(jones.chris.g) → review+
Attachment #561026 - Flags: review?(jones.chris.g) → review+
Attachment #561028 - Flags: review?(jones.chris.g) → review+
Comment on attachment 561029 [details] [diff] [review]
bug-648735.6-define-stat64-to-stat-on-bsd

># HG changeset patch
># Parent 630da21dcbacb411c34613277d8b7bc0f2e26832
># User Landry Breuil <landry@openbsd.org>
>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.
Attachment #561029 - Flags: review?(jones.chris.g) → review+
Comment on attachment 561030 [details] [diff] [review]
bug-648735.7-include-header-for-IUSR

># HG changeset patch
># Parent d3d2315348af61d0121aae80865513fa08bfff6c
># User Landry Breuil <landry@openbsd.org>
>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.
Attachment #561030 - Flags: review?(jones.chris.g) → review+
Attachment #561034 - Flags: review?(jones.chris.g) → review+
Attachment #561035 - Flags: review?(jones.chris.g) → review+
Attachment #561036 - Flags: review?(jones.chris.g) → review+
Comment on attachment 561037 [details] [diff] [review]
bug-648735.11-ipc-message-utils-templates-hack

># HG changeset patch
># Parent f0b51e7e47ba956b90b5be99cf002cbda40d924c
># User Landry Breuil <landry@openbsd.org>
>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.
Attachment #561037 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 48

6 years ago
(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 <landry@openbsd.org>
> >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().
(Assignee)

Comment 49

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

Comment 50

6 years ago
Created attachment 561149 [details] [diff] [review]
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..
Attachment #561022 - Attachment is obsolete: true
Attachment #561149 - Flags: review?(jones.chris.g)
(Assignee)

Comment 51

6 years ago
Created attachment 561150 [details] [diff] [review]
bug-648735.7-include-header-for-IUSR

unconditionally include sys/types.h, from comment #46
Attachment #561030 - Attachment is obsolete: true
Attachment #561150 - Flags: review?(jones.chris.g)
(Assignee)

Comment 52

6 years ago
Created attachment 561152 [details] [diff] [review]
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.
Attachment #561037 - Attachment is obsolete: true
Attachment #561152 - Flags: review?(jones.chris.g)
(Assignee)

Comment 53

6 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 54

6 years ago
(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.
Attachment #561024 - Flags: checkin+
Attachment #561026 - Flags: checkin+
Attachment #561028 - Flags: checkin+
Attachment #561029 - Flags: checkin+
Attachment #561034 - Flags: checkin+
Attachment #561035 - Flags: checkin+
Attachment #561036 - Flags: checkin+
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.
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [on inbound merge, leave open for remaining patches]
Version: unspecified → Trunk
(Assignee)

Comment 56

6 years ago
(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...
Status: ASSIGNED → NEW
Version: Trunk → unspecified
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Attachment #561021 - Flags: review?(jones.chris.g) → review+
Attachment #561149 - Flags: review?(jones.chris.g) → review+
Attachment #561150 - Flags: review?(jones.chris.g) → review+
Attachment #561152 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 57

6 years ago
Awesome, thanks chris. Re setting checkin-needed for 4 last chunks.
Keywords: checkin-needed
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.
Attachment #561021 - Flags: checkin+
Attachment #561149 - Flags: checkin+
Attachment #561150 - Flags: checkin+
Attachment #561152 - Flags: checkin+
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).
Keywords: checkin-needed
Whiteboard: [on inbound merge, leave open for remaining patches]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 61

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

Updated

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