Closed
Bug 648735
Opened 13 years ago
Closed 12 years ago
Fix ipc/chromium build on OpenBSD
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: gaston, Assigned: gaston)
References
Details
Attachments
(11 files, 17 obsolete files)
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 |
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•13 years ago
|
||
Not sure the fix is correct, but bsd's libc dont have backtrace()/backtrace_symbols_fd().
Assignee | ||
Comment 2•13 years ago
|
||
Fails to build with missing typedefs otherwise
Assignee | ||
Comment 3•13 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•13 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•13 years ago
|
||
Patch is horrible.
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
use vsnprintf() instead
Assignee | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
mh, forgot to set file type.
Attachment #524834 -
Attachment is obsolete: true
Assignee | ||
Comment 11•13 years ago
|
||
Strange, OS_POSIX was defined but that file doesnt seem to obey it
Assignee | ||
Comment 12•13 years ago
|
||
maybe sparc should be added too, and other oses ? Or it's better done in chromium-config.mk ?
Assignee | ||
Comment 13•13 years ago
|
||
build fails with error: variable 'iovec iov' has initializer but incomplete type
Assignee | ||
Comment 14•13 years ago
|
||
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•13 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.
Comment 16•13 years ago
|
||
CCing the three IPC folk in case they don't get all IPC mails.
Assignee | ||
Comment 17•13 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•13 years ago
|
||
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•13 years ago
|
Blocks: openbsdmeta
Assignee | ||
Comment 19•13 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•13 years ago
|
||
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•13 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•13 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 23•13 years ago
|
||
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•13 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.
Comment 25•13 years ago
|
||
Try changing #if !(defined(CHROMIUM_MOZILLA_BUILD) && defined(OS_LINUX) && defined(ARCH_CPU_64_BITS)) around ParamTraits<int64> and ParamTraits<uint64>
Assignee | ||
Comment 26•13 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•13 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•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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+
Updated•12 years ago
|
Attachment #561024 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #561026 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
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+
Updated•12 years ago
|
Attachment #561034 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #561035 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
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•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
unconditionally include sys/types.h, from comment #46
Attachment #561030 -
Attachment is obsolete: true
Attachment #561150 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 52•12 years ago
|
||
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•12 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•12 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.
Updated•12 years ago
|
Attachment #561024 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561026 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561028 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561029 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561034 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561035 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561036 -
Flags: checkin+
Comment 55•12 years ago
|
||
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•12 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•12 years ago
|
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Updated•12 years ago
|
Attachment #561021 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #561149 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #561150 -
Flags: review?(jones.chris.g) → review+
Updated•12 years ago
|
Attachment #561152 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 57•12 years ago
|
||
Awesome, thanks chris. Re setting checkin-needed for 4 last chunks.
Keywords: checkin-needed
Comment 58•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #561021 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561149 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561150 -
Flags: checkin+
Updated•12 years ago
|
Attachment #561152 -
Flags: checkin+
Comment 59•12 years ago
|
||
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]
Comment 60•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 61•12 years ago
|
||
Awesomeness, ponies and unicorns. Thanks to all involved, i'll celebrate that one :)
You need to log in
before you can comment on or make changes to this bug.
Description
•