openbsd and freebsd fixes for base

OpenBSD and FreeBSD fixes for base.

OpenBSD and FreeBSD need -I/usr/local/include for <execinfo.h>.
Use !#/usr/bin/env bash because on !linux it's not located there.

Patch by Robert Nagy <robert@openbsd.org>;

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105257

[Mark Mentovai, 2011-10-12 14:58:02]

http://codereview.chromium.org/8228005/diff/2001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/base/base.gypi#newcode601
base/base.gypi:601: '/usr/local/include',
Really? Why? What’s in here?

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:34: int mib[6];
int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_PID, process, info_size, 400 };

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:41: mib[4] = info_size;
Curious what this is used for, since info_size is already passed to sysctl as a
parameter.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:42: mib[5] = 400; /* XXX */
You’re going to need to do a little better than XXX.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:44: if (sysctl(mib, 6, &info, &info_size, NULL, 0)
< 0)
Not 6, but arraysize(mib).

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:51: return
FilePath(std::string("/usr/local/chrome/chrome"));
I understand that you don’t know how to get this on OpenBSD, but you should at
least only hard-code it in one location. This is the second time I’ve seen this
string literal.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:55: : index_of_kinfo_proc_(),
Use proper spacing to format the initializer.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:58: int mib[6];
int mib[] = { … }; (as above).

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:65: mib[5] = 0;
What’s this?

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:73: if (sysctl(mib, 6, NULL, &len, NULL, 0) <0 ){
< 0)

not

<0 )

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:73: if (sysctl(mib, 6, NULL, &len, NULL, 0) <0 ){
arraysize(mib), not 6.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:84: if (sysctl(mib, 6, &kinfo_procs_[0], &len,
NULL, 0) <0) {
Space between < and 0.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:121: mib[0] = CTL_KERN;
And again.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:126: if (sysctl(mib, 3, NULL, &len, NULL, 0) < 0) {
And again, arraysize, not a constant.

I don’t understand why you seem to have taken the Mac implementation and made
these two unnecessary changes to it.

You also seem to have removed the comments from the implementation here. What’s
the point?

Most of this code looks like it’s pretty directly shareable, do we really need
to fork this file?

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:133: if (sysctl(mib, 3, &data[0], &len, NULL, 0) <
0) {
Again.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:134: LOG(ERROR) << "failed to fetch a commandline";
I’m stopping reading this file here because the same patterns seem to repeat
themselves.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:337: return;
No need to return from a void function.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:339: void EnableTerminationOnHeapCorruption() {
Blank line to separate functions.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:340: return;
No need to return from a void function.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:342: }
Blank line before.

This should be

}  // namespace base

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1109
build/common.gypi:1109: [ 'os_posix==1 and OS!="mac" and OS!="openbsd"', {
Why?

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1130
build/common.gypi:1130: [ 'os_posix==1 and OS!="mac" and OS!="openbsd" and
chromeos==0', {
Why?

[Robert Nagy, 2011-10-12 17:03:10]

http://codereview.chromium.org/8228005/diff/2001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/base/base.gypi#newcode601
base/base.gypi:601: '/usr/local/include',
On 2011/10/12 14:58:02, Mark Mentovai wrote:
> Really? Why? What’s in here?

It is the location of execinfo.h. On BSD systems all 3rd party software is
installed under /usr/local/include (gtk, nspr, nss, etc.). /usr is only for
stuff coming with the base system.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:34: int mib[6];
On 2011/10/12 14:58:02, Mark Mentovai wrote:
> int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_PID, process, info_size, 400 };
Fixed this and all the others.

Now instead of hardcoding 400, I actually calculate a good value.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:51: return
FilePath(std::string("/usr/local/chrome/chrome"));
On 2011/10/12 14:58:02, Mark Mentovai wrote:
> I understand that you don’t know how to get this on OpenBSD, but you should at
> least only hard-code it in one location. This is the second time I’ve seen
this
> string literal.

There is no way on OpenBSD to get this from a running executable. This and
another place is the only ones needed to hardcode the path.

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:126: if (sysctl(mib, 3, NULL, &len, NULL, 0) < 0) {
I've copied a clean Mac implementation and modified to work on *BSD. Basically
the mib is different and the structure elements too. It would be better to move
this function to a shared file and only do some ifdefs to handle the
differences.

On 2011/10/12 14:58:02, Mark Mentovai wrote:
> And again, arraysize, not a constant.
> 
> I don’t understand why you seem to have taken the Mac implementation and made
> these two unnecessary changes to it.
> 
> You also seem to have removed the comments from the implementation here.
What’s
> the point?
> 
> Most of this code looks like it’s pretty directly shareable, do we really need
> to fork this file?

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1109
build/common.gypi:1109: [ 'os_posix==1 and OS!="mac" and OS!="openbsd"', {
On 2011/10/12 14:58:02, Mark Mentovai wrote:
> Why?
The comment states exactly: We don't want to get warning from third-party code,
not to mention that on at least OpenBSD the gcc version is 4.2 which means that
some of the flags are going to be unsupported. And since everything else  above
4.2 is GPLv3, there is no chance we are getting a newer compiler in the near
future.

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1130
build/common.gypi:1130: [ 'os_posix==1 and OS!="mac" and OS!="openbsd" and
chromeos==0', {
On 2011/10/12 14:58:02, Mark Mentovai wrote:
> Why?
-Wno-unused-result is unsupported

[Mark Mentovai, 2011-10-12 18:42:56]

http://codereview.chromium.org/8228005/diff/2001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/base/base.gypi#newcode601
base/base.gypi:601: '/usr/local/include',
Robert Nagy wrote:
> On 2011/10/12 14:58:02, Mark Mentovai wrote:
> > Really? Why? What’s in here?
> 
> It is the location of execinfo.h. On BSD systems all 3rd party software is
> installed under /usr/local/include (gtk, nspr, nss, etc.). /usr is only for
> stuff coming with the base system.

So this is an additional dependency that exists solely to support the
backtrace() function?

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/2001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:51: return
FilePath(std::string("/usr/local/chrome/chrome"));
Robert Nagy wrote:
> There is no way on OpenBSD to get this from a running executable. This and
> another place is the only ones needed to hardcode the path.

My point is that two places is too many. There should only be one. This location
can get the path by asking the path provider, or both this and the path provider
can get it from some other common location. Don’t put "/usr/local/chrome/chrome"
as a constant in more than one location.

The other occurrence is in base/base_paths_linux.cc.

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1109
build/common.gypi:1109: [ 'os_posix==1 and OS!="mac" and OS!="openbsd"', {
Robert Nagy wrote:
> The comment states exactly: We don't want to get warning from third-party
code,
> not to mention that on at least OpenBSD the gcc version is 4.2 which means
that
> some of the flags are going to be unsupported. And since everything else 
above
> 4.2 is GPLv3, there is no chance we are getting a newer compiler in the near
> future.

Right…

This block is responsible for removing -Wall, -Werror, and -Wextra. You’re
removing this block from OS=="openbsd", so that -Wall -Werror, and -Wextra will
NOT be removed on OpenBSD—in other words, third_party code will still build with
-Wall, -Werror, and -Wextra on OpenBSD. I didn’t think that’s what you want, and
your response to my question seems to confirm that.

See below for an explanation of why I suspect this confused you.

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1130
build/common.gypi:1130: [ 'os_posix==1 and OS!="mac" and OS!="openbsd" and
chromeos==0', {
Robert Nagy wrote:
> On 2011/10/12 14:58:02, Mark Mentovai wrote:
> > Why?
> -Wno-unused-result is unsupported

You should add something about OpenBSD needing to use GCC 4.2 in the comment
below, then.

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1166
build/common.gypi:1166: 'GCC_TREAT_WARNINGS_AS_ERRORS': 'NO',
The reason that you saw OS="mac" above is that we don’t control warnings with
the 'cflags' GYP property, we do it this way inside 'xcode_settings'.

http://codereview.chromium.org/8228005/diff/8001/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/8001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On a new file, the copyright year should be the current year, 2011.

http://codereview.chromium.org/8228005/diff/8001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:35: int mib[] = { CTL_KERN, KERN_PROC,
KERN_PROC_PID, process, sizeof(struct kinfo_proc), 0 };
No lines longer than 80 characters, per the style guide. Also on line 56 and
elsewhere.

http://codereview.chromium.org/8228005/diff/8001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:37: if (sysctl(mib, arraysize(mib), NULL, &length,
NULL, 0) == -1)
Standardize on checking == -1 or < 0.

http://codereview.chromium.org/8228005/diff/8001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:40: mib[5] = (int)(length / sizeof(struct
kinfo_proc));
Use C++<style>(casts), per the style guide.

http://codereview.chromium.org/8228005/diff/8001/base/process_util_openbsd.cc...
base/process_util_openbsd.cc:54: filter_(filter) {
The “f” should be lined up under the “i” above it, per the style guide.

[Mark Mentovai, 2011-10-12 18:44:22]

http://codereview.chromium.org/8228005/diff/8001/base/sys_info_openbsd.cc
File base/sys_info_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/8001/base/sys_info_openbsd.cc#new...
base/sys_info_openbsd.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
2011 because you’re modifying this file.

[Robert Nagy, 2011-10-12 18:59:45]

Fixed this and all of the other comments in patchset 3.

On 2011/10/12 18:44:22, Mark Mentovai wrote:
> http://codereview.chromium.org/8228005/diff/8001/base/sys_info_openbsd.cc
> File base/sys_info_openbsd.cc (right):
> 
>
http://codereview.chromium.org/8228005/diff/8001/base/sys_info_openbsd.cc#new...
> base/sys_info_openbsd.cc:1: // Copyright (c) 2009 The Chromium Authors. All
> rights reserved.
> 2011 because you’re modifying this file.

[Mark Mentovai, 2011-10-12 19:13:48]

http://codereview.chromium.org/8228005/diff/11001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8228005/diff/11001/base/base.gypi#newcode500
base/base.gypi:500: ['include', '^base_paths_linux.cc'],
Since you’re using regular expressions here:

1. Escape the dot with a backslash
2. Anchor the end of the string (since you already anchored the beginning) with
$.

'^base_paths_linux\\.cc$'

Same on the next line.

http://codereview.chromium.org/8228005/diff/11001/base/base.gypi#newcode600
base/base.gypi:600: 'include_dirs': [
You didn’t answer my question about execinfo being an additional build
dependency, but you might want to ultimately have a mode that doesn’t depend on
execinfo.

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.c...
base/process_util_openbsd.cc:41: mib[5] = reinterpret_cast<int>(length /
sizeof(struct kinfo_proc));
All of your reinterpret_casts in this file look like they’d be fine as the
(safer) static_cast.

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.c...
base/process_util_openbsd.cc:331: } // namespace base
There should actually be at least two spaces between the last non-whitespace
thing on a line and the // beginning a comment.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

and

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

and

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://codereview.chromium.org/8228005/diff/11001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8228005/diff/11001/build/common.gypi#newcode1117
build/common.gypi:1117: [ 'os_posix==1 and OS!="mac" and OS!="openbsd"', {
You haven’t changed this or responded to my comment.

[Robert Nagy, 2011-10-12 19:33:39]

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1109
build/common.gypi:1109: [ 'os_posix==1 and OS!="mac" and OS!="openbsd"', {
Oh yeah, you are right, removing that.

On 2011/10/12 18:42:56, Mark Mentovai wrote:
> Robert Nagy wrote:
> > The comment states exactly: We don't want to get warning from third-party
> code,
> > not to mention that on at least OpenBSD the gcc version is 4.2 which means
> that
> > some of the flags are going to be unsupported. And since everything else 
> above
> > 4.2 is GPLv3, there is no chance we are getting a newer compiler in the near
> > future.
> 
> Right…
> 
> This block is responsible for removing -Wall, -Werror, and -Wextra. You’re
> removing this block from OS=="openbsd", so that -Wall -Werror, and -Wextra
will
> NOT be removed on OpenBSD—in other words, third_party code will still build
with
> -Wall, -Werror, and -Wextra on OpenBSD. I didn’t think that’s what you want,
and
> your response to my question seems to confirm that.
> 
> See below for an explanation of why I suspect this confused you.

http://codereview.chromium.org/8228005/diff/2001/build/common.gypi#newcode1130
build/common.gypi:1130: [ 'os_posix==1 and OS!="mac" and OS!="openbsd" and
chromeos==0', {
On 2011/10/12 18:42:56, Mark Mentovai wrote:
> Robert Nagy wrote:
> > On 2011/10/12 14:58:02, Mark Mentovai wrote:
> > > Why?
> > -Wno-unused-result is unsupported
> 
> You should add something about OpenBSD needing to use GCC 4.2 in the comment
> below, then.

Done.

http://codereview.chromium.org/8228005/diff/11001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8228005/diff/11001/base/base.gypi#newcode500
base/base.gypi:500: ['include', '^base_paths_linux.cc'],
On 2011/10/12 19:13:48, Mark Mentovai wrote:
> Since you’re using regular expressions here:
> 
> 1. Escape the dot with a backslash
> 2. Anchor the end of the string (since you already anchored the beginning)
with
> $.
> 
> '^base_paths_linux\\.cc$'
> 
> Same on the next line.

Done.

http://codereview.chromium.org/8228005/diff/11001/base/base.gypi#newcode600
base/base.gypi:600: 'include_dirs': [
On 2011/10/12 19:13:48, Mark Mentovai wrote:
> You didn’t answer my question about execinfo being an additional build
> dependency, but you might want to ultimately have a mode that doesn’t depend
on
> execinfo.
Oh sorry about that, missed it. Yes libexecinfo is a clone of the backtrace
facility found in glibc.

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.c...
base/process_util_openbsd.cc:41: mib[5] = reinterpret_cast<int>(length /
sizeof(struct kinfo_proc));
On 2011/10/12 19:13:48, Mark Mentovai wrote:
> All of your reinterpret_casts in this file look like they’d be fine as the
> (safer) static_cast.
They are actually not needed, so I removed them.

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.c...
base/process_util_openbsd.cc:331: } // namespace base
On 2011/10/12 19:13:48, Mark Mentovai wrote:
> There should actually be at least two spaces between the last non-whitespace
> thing on a line and the // beginning a comment.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
> 
> and
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> 
> and
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.c...
base/process_util_openbsd.cc:331: } // namespace base
On 2011/10/12 19:13:48, Mark Mentovai wrote:
> There should actually be at least two spaces between the last non-whitespace
> thing on a line and the // beginning a comment.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
> 
> and
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> 
> and
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

http://codereview.chromium.org/8228005/diff/11001/base/process_util_openbsd.c...
base/process_util_openbsd.cc:331: } // namespace base
On 2011/10/12 19:13:48, Mark Mentovai wrote:
> There should actually be at least two spaces between the last non-whitespace
> thing on a line and the // beginning a comment.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
> 
> and
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> 
> and
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

[Mark Mentovai, 2011-10-12 21:37:24]

Köszönöm. LGTM.

Have you signed the CLA? http://code.google.com/legal/individual-cla-v1.0.html
or http://code.google.com/legal/corporate-cla-v1.0.html.

Please add yourself to the AUTHORS file as part of this patch.

[Robert Nagy, 2011-10-12 21:45:13]

On 2011/10/12 21:37:24, Mark Mentovai wrote:
> Köszönöm. LGTM.
> 
> Have you signed the CLA? http://code.google.com/legal/individual-cla-v1.0.html
> or http://code.google.com/legal/corporate-cla-v1.0.html.
> 
> Please add yourself to the AUTHORS file as part of this patch.

Yes, I've signed the CLA and i've also added myself to the AUTHORS file in the
latest patchset.

Thanks!

[Mark Mentovai, 2011-10-12 21:52:10]

lgtm

Great, then let’s try the commit queue!

[commit-bot: I haz the power, 2011-10-12 21:52:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8228005/13001

[commit-bot: I haz the power, 2011-10-12 21:52:31]

Presubmit check for 8228005-13001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
robert.nagy@gmail.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 4.3s to calculate.

[commit-bot: I haz the power, 2011-10-12 21:57:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8228005/13001

[commit-bot: I haz the power, 2011-10-12 21:57:35]

Presubmit check for 8228005-13001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
robert.nagy@gmail.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 2.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Mark Mentovai, 2011-10-13 04:25:25]

Thanks for the patch! Committed at r105257.

The commit bot should work next time now that you’re in the AUTHORS file.