initial v8 openbsd support

initial v8 openbsd support

[pvalchev, 2009-11-25 02:55:05]

seems to work from a few basic tests

[Mikhail Naganov, 2009-11-25 09:26:42]

On 2009/11/25 02:55:05, pvalchev wrote:
> seems to work from a few basic tests

Out of curiosity, I've compared 'platform-openbsd.cc' with 'platform-freebsd.cc'
and found only minor differences. I think, a better approach would be to reuse
the common code for these two platforms as much as possible.

So, my proposal is to rename 'platform-freebsd' to something like
'platform-bsd', and throw in #ifdefs where needed.

[Søren Thygesen Gjesse, 2009-11-25 16:31:25]

On 2009/11/25 09:26:42, Mikhail Naganov wrote:
> On 2009/11/25 02:55:05, pvalchev wrote:
> > seems to work from a few basic tests
> 
> Out of curiosity, I've compared 'platform-openbsd.cc' with
'platform-freebsd.cc'
> and found only minor differences. I think, a better approach would be to reuse
> the common code for these two platforms as much as possible.
> 
> So, my proposal is to rename 'platform-freebsd' to something like
> 'platform-bsd', and throw in #ifdefs where needed.

Maybe it is also be possible to move more common code into platform-posix.

[pvalchev, 2009-11-25 17:48:37]

On 2009/11/25 09:26:42, Mikhail Naganov wrote:
> On 2009/11/25 02:55:05, pvalchev wrote:
> > seems to work from a few basic tests
> 
> Out of curiosity, I've compared 'platform-openbsd.cc' with
'platform-freebsd.cc'
> and found only minor differences. I think, a better approach would be to reuse
> the common code for these two platforms as much as possible.
> 
> So, my proposal is to rename 'platform-freebsd' to something like
> 'platform-bsd', and throw in #ifdefs where needed.

I can see where you are coming from, but the freebsd one is basically a copy of
the linux one with OS-specific modifications too. So you can make the argument
that there should be only one, with lots of ifdef's - but that's very ugly and I
like chrome's approach of having different _os.c files.

There are some significant differences between freebsd and openbsd in this file,
for example the lack of ucontext. Also a lot of the mmap/mprotect magic will be
different, because OpenBSD has W^X (where no page can be writable and executable
at the same time, as a security measure). The difference between the two files
may seem minor right now, but that's because several of the openbsd functions
are UNIMPLEMENTED();, I just wanted to get further before spending significant
time on flushing out this file, but there is more work to be done.

Did I mention I hate ifdef's? :) But it's up to you guys to decide in the end.

[pvalchev, 2009-11-25 17:49:26]

On 2009/11/25 16:31:25, Søren Gjesse wrote:
> On 2009/11/25 09:26:42, Mikhail Naganov wrote:
> > On 2009/11/25 02:55:05, pvalchev wrote:
> > > seems to work from a few basic tests
> > 
> > Out of curiosity, I've compared 'platform-openbsd.cc' with
> 'platform-freebsd.cc'
> > and found only minor differences. I think, a better approach would be to
reuse
> > the common code for these two platforms as much as possible.
> > 
> > So, my proposal is to rename 'platform-freebsd' to something like
> > 'platform-bsd', and throw in #ifdefs where needed.
> 
> Maybe it is also be possible to move more common code into platform-posix.

The common code (or at least the majority of it) is already in platform-posix,
there are just a few functions missing that are left up to each OS to implement
(as they differ).

Peter

[Erik Corry, 2009-11-25 21:06:54]

http://codereview.chromium.org/431047/diff/1/4
File src/platform-openbsd.cc (right):

http://codereview.chromium.org/431047/diff/1/4#newcode269
src/platform-openbsd.cc:269: /*
Please don't leave commented code in the source.

http://codereview.chromium.org/431047/diff/1/4#newcode557
src/platform-openbsd.cc:557: if (active_sampler_->IsProfiling()) {
And here

[Mikhail Naganov, 2009-11-25 22:21:23]

On 2009/11/25 17:48:37, pvalchev wrote:
> On 2009/11/25 09:26:42, Mikhail Naganov wrote:
> > On 2009/11/25 02:55:05, pvalchev wrote:
> > > seems to work from a few basic tests
> > 
> > Out of curiosity, I've compared 'platform-openbsd.cc' with
> 'platform-freebsd.cc'
> > and found only minor differences. I think, a better approach would be to
reuse
> > the common code for these two platforms as much as possible.
> > 
> > So, my proposal is to rename 'platform-freebsd' to something like
> > 'platform-bsd', and throw in #ifdefs where needed.
> 
> I can see where you are coming from, but the freebsd one is basically a copy
of
> the linux one with OS-specific modifications too. So you can make the argument
> that there should be only one, with lots of ifdef's - but that's very ugly and
I
> like chrome's approach of having different _os.c files.
>

Can you please point out more concretely what '_os.c' files do you mean? In
Chromium, I can only find such files in sqlite and native_client. But in
Chromium's own code I see lots of "#if defined(OS_LINUX) ||
defined(OS_FREEBSD)"-like branches.

I agree that having such ifdefs is ugly is confusing, it also doesn't scale
well: e.g. what will happen if someone will be adding NetBSD or Solaris support?
But I also don't have any good example of how to get portability right from the
start. Is there any good example of a serious cross-platform project which
doesn't use #ifdef's?
 
> There are some significant differences between freebsd and openbsd in this
file,
> for example the lack of ucontext. Also a lot of the mmap/mprotect magic will
be
> different, because OpenBSD has W^X (where no page can be writable and
executable
> at the same time, as a security measure). The difference between the two files
> may seem minor right now, but that's because several of the openbsd functions
> are UNIMPLEMENTED();, I just wanted to get further before spending significant
> time on flushing out this file, but there is more work to be done.
> 
> Did I mention I hate ifdef's? :) But it's up to you guys to decide in the end.

I wouldn't like to be a blocker of adding OpenBSD support. I guess this is for
Chromium, right? But if you really have understanding of future changes in your
'platform-openbsd' source, please clean it up and add appropriate comments.
Right now it looks like it has been hastily copied and patched from
'platform-freebsd'.

[Evan Martin, 2009-11-25 22:24:20]

On 2009/11/25 22:21:23, Mikhail Naganov wrote:
> Can you please point out more concretely what '_os.c' files do you mean? In
> Chromium, I can only find such files in sqlite and native_client. But in
> Chromium's own code I see lots of "#if defined(OS_LINUX) ||
> defined(OS_FREEBSD)"-like branches.

I am attempting to remove many those.

My general approach is:
if the function looks like
void f() {
  #if OS1
    implementation_1();
  #elif OS2
    implementation_2();
}
then it should be split into multiple files.

Otherwise, if you need the occasional ifdef within shared code, it's ok.

Take a look at some of the _freebsd files in Chrome's base/ and compare them to
the _linux or _posix ones (I've just been splitting them over the last week).

[Mikhail Naganov, 2009-11-26 11:10:38]

On 2009/11/25 22:24:20, Evan Martin wrote:
> On 2009/11/25 22:21:23, Mikhail Naganov wrote:
> > Can you please point out more concretely what '_os.c' files do you mean? In
> > Chromium, I can only find such files in sqlite and native_client. But in
> > Chromium's own code I see lots of "#if defined(OS_LINUX) ||
> > defined(OS_FREEBSD)"-like branches.
> 
> I am attempting to remove many those.
> 
> My general approach is:
> if the function looks like
> void f() {
>   #if OS1
>     implementation_1();
>   #elif OS2
>     implementation_2();
> }
> then it should be split into multiple files.
> 
> Otherwise, if you need the occasional ifdef within shared code, it's ok.
> 
> Take a look at some of the _freebsd files in Chrome's base/ and compare them
to
> the _linux or _posix ones (I've just been splitting them over the last week).

OK, so it looks like the strategy in Chromium is as follows:
- allow minimal spot-sized #ifdefs
- split files otherwise, as follows:
  - initially split into _posix and _win (and, possibly _mac)
  - split _posix into _linux, _freebsd or whatever

I like it. Evan, am I getting things right?

[Søren Thygesen Gjesse, 2009-11-26 11:37:38]

LGTM

Please change the svn:eol-style to native.

http://codereview.chromium.org/431047/diff/1/4
File src/platform-openbsd.cc (right):

http://codereview.chromium.org/431047/diff/1/4#newcode142
src/platform-openbsd.cc:142: // TODO(1240712): munmap has a return value which
is ignored here.
Please remove this TODO or create a new bug in
http://code.google.com/p/v8/issues/list and change the TODO to refer to this new
bug.

[pvalchev, 2009-12-01 22:08:22]

http://codereview.chromium.org/431047/diff/1/4
File src/platform-openbsd.cc (right):

http://codereview.chromium.org/431047/diff/1/4#newcode142
src/platform-openbsd.cc:142: // TODO(1240712): munmap has a return value which
is ignored here.
On 2009/11/26 11:37:38, Søren Gjesse wrote:
> Please remove this TODO or create a new bug in
> http://code.google.com/p/v8/issues/list and change the TODO to refer to this
new
> bug.

Fixed (there's an ASSERT that it returns 0 which is success, that should be
sufficient as there's nothing else that can be really done if it fails, it's a
void free function)

http://codereview.chromium.org/431047/diff/1/4#newcode269
src/platform-openbsd.cc:269: /*
On 2009/11/25 21:06:54, Erik Corry wrote:
> Please don't leave commented code in the source.

Done.

http://codereview.chromium.org/431047/diff/1/4#newcode557
src/platform-openbsd.cc:557: if (active_sampler_->IsProfiling()) {
On 2009/11/25 21:06:54, Erik Corry wrote:
> And here

Done.

[Søren Thygesen Gjesse, 2009-12-02 13:44:41]

On 2009/12/01 22:08:22, pvalchev wrote:
> http://codereview.chromium.org/431047/diff/1/4
> File src/platform-openbsd.cc (right):
> 
> http://codereview.chromium.org/431047/diff/1/4#newcode142
> src/platform-openbsd.cc:142: // TODO(1240712): munmap has a return value which
> is ignored here.
> On 2009/11/26 11:37:38, Søren Gjesse wrote:
> > Please remove this TODO or create a new bug in
> > http://code.google.com/p/v8/issues/list and change the TODO to refer to this
> new
> > bug.
> 
> Fixed (there's an ASSERT that it returns 0 which is success, that should be
> sufficient as there's nothing else that can be really done if it fails, it's a
> void free function)
> 
> http://codereview.chromium.org/431047/diff/1/4#newcode269
> src/platform-openbsd.cc:269: /*
> On 2009/11/25 21:06:54, Erik Corry wrote:
> > Please don't leave commented code in the source.
> 
> Done.
> 
> http://codereview.chromium.org/431047/diff/1/4#newcode557
> src/platform-openbsd.cc:557: if (active_sampler_->IsProfiling()) {
> On 2009/11/25 21:06:54, Erik Corry wrote:
> > And here
> 
> Done.

Landed as bleeding_edge@3398 through http://codereview.chromium.org/465002.