Solaris: various edits towards compiling Chromium on Solaris. Changed __Solar... (issue652166)

[electric...@gmail.com]

Reviewers: ,

Message:
gcl upload prints this-

** Presubmit Messages **
Found a bad license header in these files:
tools/gyp/pylib/gyp/__init__.py

Should I just go ahead and change Copyright (c) Google 2009 to 2010?

Description:
Solaris: various edits towards compiling Chromium on Solaris. Changed
__Solaris__ to __sun. Replaced NAME_MAX with more OS-universal MAXNAMLEN.

BUG=30101
TEST=compiles
Patch by James Choi <jchoi42 at pha.jhu.edu>

Please review this at http://codereview.chromium.org/652166

SVN Base: http://src.chromium.org/svn/trunk/src/

Affected files:
M base/base.gypi
M base/data_pack.cc
M base/float_util.h
M base/process_util.h
M build/common.gypi
M build/linux/dump_app_syms
M tools/gyp/pylib/gyp/__init__.py

[electric...@gmail.com]

Blast from the past- I didn't realize this was still open!

Anything, guys? :)

http://codereview.chromium.org/652166

[Brett Wilson]

Code review is broken for me now.

My comment is to remove the indenting for the define in float_util.h,
we don't indent ifdefs like that (follow how the rest of the patch
is).

Brett

[ev...@chromium.org]

http://codereview.chromium.org/652166/diff/2001/3009
File base/float_util.h (right):

http://codereview.chromium.org/652166/diff/2001/3009#newcode15
base/float_util.h:15: # define finite(val) (val <=
std::numeric_limits<double>::max())
Better than an ifdef, why not just extend the #if chain within the
function body?

http://codereview.chromium.org/652166/diff/2001/3007
File base/process_util.h (right):

http://codereview.chromium.org/652166/diff/2001/3007#newcode43
base/process_util.h:43: char szExeFile[MAXNAMLEN + 1];
% cat test.c ; ./test
#include <stdio.h>
#include <limits.h>
#include <dirent.h>

int main(int argc, char* argv[]) {
printf("NAME_MAX %d\nMAXNAMLEN %d\n", NAME_MAX, MAXNAMLEN);
}
NAME_MAX 255
MAXNAMLEN 255

However, this thread argues that MAXNAMLEN is obsolete:
http://www.mail-archive.com/bu...@crater.dragonflybsd.org/msg03065.html

Is NAME_MAX really not available?

http://codereview.chromium.org/652166/diff/2001/3001
File base/third_party/nspr/prcpucfg.h (right):

http://codereview.chromium.org/652166/diff/2001/3001#newcode43
base/third_party/nspr/prcpucfg.h:43: #elif defined(__sun)
How come?

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

http://codereview.chromium.org/652166/diff/2001/3004#newcode710
build/common.gypi:710: ['OS!="solaris"', {
Can you rather leave the above lines in, and then do

OS=="solaris", {
'cflags!': : ['-fvisibility=hidden'],

The "!" means "remove this from the cflags". I just worry about
scattering the normal set of CFLAGS out.

http://codereview.chromium.org/652166/diff/2001/3005
File build/linux/dump_app_syms (right):

http://codereview.chromium.org/652166/diff/2001/3005#newcode51
build/linux/dump_app_syms:51: /usr/gnu/bin/sed -i "1s/ [0-9A-F]* /
$NEWSIG /" "$OUTFILE"
Can you rather compute a $GNU_SED variable? I always hate to see
duplicated code like this, even when it's one line

http://codereview.chromium.org/652166

[electric...@gmail.com]

> However, this thread argues that MAXNAMLEN is obsolete:

> http://www.mail-archive.com/bugs%40crater.dragonflybsd.org/msg03065.html

> Is NAME_MAX really not available?

No, it's not. :(

I was told Solaris kept MAXNAMLEN around for compatibility, since not all
platforms have NAME_MAX, and relying on it would be unpredictable. (eg
http://opensolaris.org/jive/message.jspa?messageID=138689).

OTOH, I could add an "#if !defined(NAME_MAX) #define NAME_MAX MAXNAMELEN" or
"... #define MAXNAMLEN NAME_MAX". There'd be no difference on my computer
anyhow, but those vendor corner cases make the latter seem more reasonable
imho...

> http://codereview.chromium.org/652166/diff/2001/3001
> File base/third_party/nspr/prcpucfg.h (right):

> http://codereview.chromium.org/652166/diff/2001/3001#newcode43
> base/third_party/nspr/prcpucfg.h:43: #elif defined(__sun)
> How come?

gcc has predefined __sun and __sun__ ... but not __Solaris__ :P

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

> http://codereview.chromium.org/652166/diff/2001/3004#newcode710
> build/common.gypi:710: ['OS!="solaris"', {
> Can you rather leave the above lines in, and then do

> OS=="solaris", {
> 'cflags!': : ['-fvisibility=hidden'],

> The "!" means "remove this from the cflags". I just worry about
> scattering
the
> normal set of CFLAGS out.

Absolutely. It's much cleaner your way. However, with this new code, my
compile
crashburns about visibility attributes. It does not appear to be
acknowledging
this conditional anymore??? !!

http://codereview.chromium.org/652166

[ev...@chromium.org]

http://codereview.chromium.org/652166/diff/2010/2014
File build/common.gypi (right):

http://codereview.chromium.org/652166/diff/2010/2014#newcode719
build/common.gypi:719: 'conditions': [
There looks to be a second 'conditions' block lower down at the same
scope -- I bet it conflicts (perhaps overwrites) with this one.

http://codereview.chromium.org/652166

[ev...@chromium.org]

On 2010/03/01 17:43:43, jchoi42 wrote:
> > Is NAME_MAX really not available?

> No, it's not. :(

SQLite does this in a shared Unix file:

#ifndef NAME_MAX
# ifdef MAXPATHLEN
# define NAME_MAX MAXPATHLEN
# else
# ifdef FILENAME_MAX
# define NAME_MAX FILENAME_MAX
# else
# define NAME_MAX 256 /* nobody has fewer than this (since the PDP-8 ;)
*/
# endif
# endif
#endif

So I guess the first part would be ok

#ifndef NAME_MAX
#ifdef MAXPATHLEN
#define NAME_MAX MAXPATHLEN
#endif
#endif

http://codereview.chromium.org/652166

[electric...@gmail.com]

Done. Though the changes to build/common.gypi made no difference :(

[electric...@gmail.com]

Anyone?

[phajd...@chromium.org]

http://codereview.chromium.org/652166/diff/2026/2032
File base/data_pack.cc (right):

http://codereview.chromium.org/652166/diff/2026/2032#newcode26
base/data_pack.cc:26: #pragma pack(1)
I think this logic should be extracted to some macro in
base/compiler_specific.h

http://codereview.chromium.org/652166/diff/2026/2035
File base/float_util.h (right):

http://codereview.chromium.org/652166/diff/2026/2035#newcode11
base/float_util.h:11: #include <limits> // required on Solaris for
finite()
nit: This is a C++ header, so should go after the c-style headers, after
a blank line. Also, no comment is required, and actually it should be
removed.

http://codereview.chromium.org/652166/diff/2026/2029
File tools/gyp/pylib/gyp/__init__.py (right):

http://codereview.chromium.org/652166/diff/2026/2029#newcode314
tools/gyp/pylib/gyp/__init__.py:314: 'sunos5': 'make',
I think this change should go to the upstream gyp repo.

http://codereview.chromium.org/652166

[electric...@gmail.com]

> http://codereview.chromium.org/652166/diff/2026/2032#newcode26
> base/data_pack.cc:26: #pragma pack(1)
> I think this logic should be extracted to some macro in
base/compiler_specific.h

Sure. Specifically, what did you have in mind?

> http://codereview.chromium.org/652166/diff/2026/2029#newcode314
> tools/gyp/pylib/gyp/__init__.py:314: 'sunos5': 'make',
> I think this change should go to the upstream gyp repo.

Agreed. How does this work, or alternately, does anyone reading this have
commit
access there? :)

http://codereview.chromium.org/652166

[phajd...@chromium.org]

On 2010/03/24 15:01:05, jchoi42 wrote:
> > http://codereview.chromium.org/652166/diff/2026/2032#newcode26
> > base/data_pack.cc:26: #pragma pack(1)
> > I think this logic should be extracted to some macro in
> base/compiler_specific.h

> Sure. Specifically, what did you have in mind?

Add a new macro that would expand to one thing on Solaris and to the other
thing
everywhere else. Find a name that sounds reasonable, we can correct that in
the
next round of review anyway.

> > http://codereview.chromium.org/652166/diff/2026/2029#newcode314
> > tools/gyp/pylib/gyp/__init__.py:314: 'sunos5': 'make',
> > I think this change should go to the upstream gyp repo.

> Agreed. How does this work, or alternately, does anyone reading this have
commit
> access there? :)

Remove it from this CL, and ask gyp people how to contribute to gyp (Mark
Mentovai aka mark, markm, mmentovai is a good person to ask about that).

http://codereview.chromium.org/652166

[electric...@gmail.com]

Ok, adding Mark here for the __init__.py issue.

Moved the pragma bit out to base/compiler_specific.h for review.

http://codereview.chromium.org/652166

[Mark Mentovai]

Check out the GYP trunk according to these instructions:

http://code.google.com/p/gyp/source/browse/

You can modify __init__.py there, and use gcl for your code review.
Send it to me.

[Mark Mentovai]

I wrote:
> Check out the GYP trunk according to these instructions:
>
> http://code.google.com/p/gyp/source/browse/

http://code.google.com/p/gyp/source/checkout, actually.

[ev...@chromium.org]

http://codereview.chromium.org/652166/diff/22001/23007
File base/base.gypi (right):

http://codereview.chromium.org/652166/diff/22001/23007#newcode599
base/base.gypi:599: # 'cflags': [ '-oohfhjdhfksfkjk'],
probably didn't intend to include this? :)

http://codereview.chromium.org/652166/diff/22001/23009
File base/compiler_specific.h (right):

http://codereview.chromium.org/652166/diff/22001/23009#newcode98
base/compiler_specific.h:98: #endif // BASE_COMPILER_SPECIFIC_H_
This line should be the last in the file, I think.

http://codereview.chromium.org/652166/diff/22001/23009#newcode101
base/compiler_specific.h:101: #define PRAGMA_PUSH _Pragma ( "pack(1)" )
This think this should be something more like
PRAGMA_PACK_STRUCT_PUSH

That is, it's not just pushing a generic pragma, it's pushing a specific
pragma (to pack structs)

http://codereview.chromium.org/652166/diff/22001/23008
File base/float_util.h (right):

http://codereview.chromium.org/652166/diff/22001/23008#newcode20
base/float_util.h:20: # define finite(val) (val <=
std::numeric_limits<double>::max())
why the #define, why not just return (val <= ...)

http://codereview.chromium.org/652166/diff/22001/23006
File base/process_util.h (right):

http://codereview.chromium.org/652166/diff/22001/23006#newcode39
base/process_util.h:39: #define NAME_MAX 256
Can you note where this comes from? Or should we have it at all?

http://codereview.chromium.org/652166/diff/22001/23003
File build/common.gypi (right):

http://codereview.chromium.org/652166/diff/22001/23003#newcode721
build/common.gypi:721: #'-fvisibility=hidden',
Probably unintentional.

http://codereview.chromium.org/652166/diff/22001/23004
File build/linux/dump_app_syms (right):

http://codereview.chromium.org/652166/diff/22001/23004#newcode55
build/linux/dump_app_syms:55: `$GNU_SED -i "1s/ [0-9A-F]* / $NEWSIG /"
"$OUTFILE"`
Why the backticks here?

http://codereview.chromium.org/652166

[ma...@chromium.org]

I’d look some of these up myself, but I don’t have access to a Solaris
system.
Maybe I should put one in a virtual machine.

http://codereview.chromium.org/652166/diff/22001/23009
File base/compiler_specific.h (right):

http://codereview.chromium.org/652166/diff/22001/23009#newcode101
base/compiler_specific.h:101: #define PRAGMA_PUSH _Pragma ( "pack(1)" )

Are you using gcc or something else?

http://codereview.chromium.org/652166/diff/22001/23008
File base/float_util.h (right):

http://codereview.chromium.org/652166/diff/22001/23008#newcode20
base/float_util.h:20: # define finite(val) (val <=
std::numeric_limits<double>::max())

http://docs.sun.com/app/docs/doc/801-6680-03/6i11qck75?l=en&a=view&q=finite
says <ieeefp.h>?

http://codereview.chromium.org/652166/diff/22001/23001
File base/third_party/nspr/prcpucfg.h (right):

http://codereview.chromium.org/652166/diff/22001/23001#newcode43
base/third_party/nspr/prcpucfg.h:43: #elif defined(__sun)
Doesn’t this identify a vendor and not an OS?

http://codereview.chromium.org/652166/diff/22001/23004
File build/linux/dump_app_syms (right):

http://codereview.chromium.org/652166/diff/22001/23004#newcode51
build/linux/dump_app_syms:51: GNU_SED="/usr/gnu/bin/sed"
We should just avoid using sed extensions like -i.

http://codereview.chromium.org/652166

[ev...@chromium.org]

http://codereview.chromium.org/652166/diff/22001/23004
File build/linux/dump_app_syms (right):

http://codereview.chromium.org/652166/diff/22001/23004#newcode51
build/linux/dump_app_syms:51: GNU_SED="/usr/gnu/bin/sed"

On 2010/03/24 21:16:08, Mark Mentovai wrote:
> We should just avoid using sed extensions like -i.

It occurs to me now that we don't care about getting symbols on
non-Linux systems anyway, so maybe you can just leave this file alone.

http://codereview.chromium.org/652166

[electric...@gmail.com]

> http://codereview.chromium.org/652166/diff/22001/23007#newcode599
> base/base.gypi:599: # 'cflags': [ '-oohfhjdhfksfkjk'],
> probably didn't intend to include this? :)

It's actually um... a secret compiler option that whitens your teeth and
fixes
global warming, but it's not c99 so IF YOU INSIST I'll take it out. Gosh!

> http://codereview.chromium.org/652166/diff/22001/23006#newcode39
> base/process_util.h:39: #define NAME_MAX 256
> Can you note where this comes from? Or should we have it at all?

http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/thread/8f14ade244e9e74d/68dc9205dba9da87?lnk=raot

I've also seen code use 255 instead of 256.

I can't find the original post talking about why Solaris has no NAME_MAX,
but
this is relevant: http://opensolaris.org/jive/message.jspa?messageID=138689

> http://codereview.chromium.org/652166/diff/22001/23004#newcode55
> build/linux/dump_app_syms:55: `$GNU_SED -i "1s/ [0-9A-F]* / $NEWSIG /"
> "$OUTFILE"`
> Why the backticks here?

It's evaluating it; I'm not happy with it either, but I'm open to any
alternatives :)

http://codereview.chromium.org/652166

[electric...@gmail.com]

> http://codereview.chromium.org/652166/diff/22001/23009#newcode101
> base/compiler_specific.h:101: #define PRAGMA_PUSH _Pragma ( "pack(1)" )
> Are you using gcc or something else?

Yup, I'm using gcc 4.3.3. It fails at
http://forums.sun.com/thread.jspa?threadID=5072550

> http://codereview.chromium.org/652166/diff/22001/23001#newcode43
> base/third_party/nspr/prcpucfg.h:43: #elif defined(__sun)
> Doesn’t this identify a vendor and not an OS?

Hmm, that's true. I think it's the same thing here, though. Think I should
change it to #elif defined OS_SOLARIS?

> http://codereview.chromium.org/652166/diff/22001/23004#newcode51
> build/linux/dump_app_syms:51: GNU_SED="/usr/gnu/bin/sed"

> We should just avoid using sed extensions like -i.

:D

http://codereview.chromium.org/652166

[Mark Mentovai]

electric...@gmail.com wrote:
>> http://codereview.chromium.org/652166/diff/22001/23009#newcode101
>> base/compiler_specific.h:101: #define PRAGMA_PUSH _Pragma ( "pack(1)" )
>> Are you using gcc or something else?
>
> Yup, I'm using gcc 4.3.3. It fails at
> http://forums.sun.com/thread.jspa?threadID=5072550

If gcc on Solaris doesn't support pushing and popping the pack
setting, then we shouldn't rely on pushing and popping on any target,
and our macros shouldn't encourage pushing and popping or make it seem
like we can push or pop.

Mark

[Evan Martin]

On my 64-bit system, it seems we don't need struct packing anyway.

$ cat test.c
#include <stdint.h>

struct test {
int32_t x;
int32_t y;
};

int main() {
return sizeof(struct test);
}
evmar:~$ ./test ; echo $?
8

[Mark Mentovai]

Evan Martin wrote:
> On my 64-bit system, it seems we don't need struct packing anyway.

Sure, not if your struct doesn’t have any 64-bit fields. Turn |y| in
your example into an int64_t. Then you’ll get different results
between -m32 and -m64.

[Evan Martin]

On Fri, Mar 26, 2010 at 9:46 AM, Mark Mentovai <ma...@chromium.org> wrote:
> Evan Martin wrote:
>> On my 64-bit system, it seems we don't need struct packing anyway.
>
> Sure, not if your struct doesn’t have any 64-bit fields.  Turn |y| in
> your example into an int64_t.  Then you’ll get different results
> between -m32 and -m64.

For the one struct that uses this, it's all 32-bit fields. I think we
can remove all the pack business and replace it with a
COMPILE_ASSERT(sizeof struct foo == 12);

[Mark Mentovai]

Evan Martin wrote:
> For the one struct that uses this, it's all 32-bit fields.  I think we
> can remove all the pack business and replace it with a
> COMPILE_ASSERT(sizeof struct foo == 12);

That’d be fine.

Mark

[electric...@gmail.com]

Did one of you guys want to edit the file, or should I? In the latter case,
what
specifically would you like changed?

http://codereview.chromium.org/652166

[ev...@chromium.org]

On 2010/03/31 15:04:02, jchoi42 wrote:
> Did one of you guys want to edit the file, or should I? In the latter
> case,
what
> specifically would you like changed?

See the COMPILE_ASSERT macro in base/basictypes.h.
Use it to assert that sizeof(that struct) == 12.
Then we can remove the pack lines.

http://codereview.chromium.org/652166

[electric...@gmail.com]

There is already a line
COMPILE_ASSERT(sizeof(DataPackEntry) == 12, size_of_header_must_be_twelve);
in base/data_pack.cc. Is this what you meant? Or something different?

[ev...@chromium.org]

+erg, who wrote the code

Ah, we do already check! Elliot, did you add the #pragma pack bit to fix
anything or was it just for clarity?

http://codereview.chromium.org/652166

[e...@chromium.org]

On 2010/03/31 16:16:46, Evan Martin wrote:
> +erg, who wrote the code

> Ah, we do already check! Elliot, did you add the #pragma pack bit to fix
> anything or was it just for clarity?

Actually, you wrote this code, but with the non-portable
"__attribute((packed));" in a232515a. I just changed it to #pragma pack in
cc2fd214 for compatibility with Visual Studio since all platforms need
DataPack
for the theme cache.

I do think it adds to clarity since this is an on disk/mmapped format...

http://codereview.chromium.org/652166

[ev...@chromium.org]

Ok, so it then sounds like we can remove the pack and let the compile_assert
take care of it. We can readd packing stuff if it ever becomes necessary.

http://codereview.chromium.org/652166

[electric...@gmail.com]

Removed build/linux/dump_app_syms as it has been reworked by evmar to not
use
sed -i :)

http://codereview.chromium.org/652166

[electric...@gmail.com]

Removed base/compiler_specific.h and modified base/data_pack.cc to not use
the
push/pop pragma packing, as described above.

http://codereview.chromium.org/652166

[ev...@chromium.org]

LGTM, a few last fixes

http://codereview.chromium.org/652166/diff/59001/60003
File build/common.gypi (right):

http://codereview.chromium.org/652166/diff/59001/60003#newcode730
build/common.gypi:730: #'-fvisibility-inlines-hidden',
This can't be intentional, right? :)

http://codereview.chromium.org/652166/diff/59001/60003#newcode1177
build/common.gypi:1177: # Disable native client on FreeBSD/OpenBSD for
now
this comment is now wrong

http://codereview.chromium.org/652166

[electric...@gmail.com]

> http://codereview.chromium.org/652166/diff/59001/60003#newcode1177
> build/common.gypi:1177: # Disable native client on FreeBSD/OpenBSD for now
> this comment is now wrong

Ok, in that case, should I also take out the OS==freebsd,openbsd bit like...
- ['disable_nacl==1 or OS=="freebsd" or OS=="openbsd"', {
+ ['disable_nacl==1 or OS=="solaris"', {

?

http://codereview.chromium.org/652166

[ev...@chromium.org]

> ?

Nah, I was just saying that the comment should just reflect the code.

http://codereview.chromium.org/652166

[electric...@gmail.com]

done!

http://codereview.chromium.org/652166