Solaris: various edits towards compiling Chromium on Solaris. Changed __Solar...

Solaris: various edits towards compiling Chromium on Solaris. Changed __Solaris__ to __sun. Defined NAME_MAX as MAXNAMLEN for systems where it is undefined.

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43297

[jchoi42, 2010-02-23 22:55:10]

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?

[jchoi42, 2010-02-28 20:03:31]

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

Anything, guys? :)

[brettw at google, 2010-03-01 05:28:37]

On Sun, Feb 28, 2010 at 12:03 PM,  <electricmonopole@gmail.com> wrote:
> Blast from the past- I didn't realize this was still open!
>
> Anything, guys? :)
>
> http://codereview.chromium.org/652166

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

[Evan Martin, 2010-03-01 07:16:56]

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/bugs@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

[jchoi42, 2010-03-01 17:43:43]

> 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??? !!

[Evan Martin, 2010-03-01 17:48:59]

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.

[Evan Martin, 2010-03-01 17:54:35]

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

[jchoi42, 2010-03-03 17:29:52]

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


On 2010/03/01 17:54:35, Evan Martin wrote:
> 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

[jchoi42, 2010-03-24 13:59:53]

Anyone?

On 2010/03/03 17:29:52, jchoi42 wrote:
> Done. Though the changes to build/common.gypi made no difference :(
> 
> 
> On 2010/03/01 17:54:35, Evan Martin wrote:
> > 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

[Paweł Hajdan Jr., 2010-03-24 14:06:27]

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.

[jchoi42, 2010-03-24 15:01:05]

> 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? :)

[Paweł Hajdan Jr., 2010-03-24 16:41:39]

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).

[jchoi42, 2010-03-24 17:30:37]

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

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

[Mark Mentovai, 2010-03-24 17:34:09]

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, 2010-03-24 17:35:06]

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.

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

[Evan Martin, 2010-03-24 21:06:41]

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?

[Mark Mentovai, 2010-03-24 21:16:08]

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.

[Evan Martin, 2010-03-24 21:21:44]

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.

[jchoi42, 2010-03-25 15:58:33]

> 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/...

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 :)

[jchoi42, 2010-03-25 16:03:45]

> 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

[Mark Mentovai, 2010-03-26 16:40:08]

electricmonopole@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, 2010-03-26 16:43:36]

On Fri, Mar 26, 2010 at 9:39 AM, Mark Mentovai <mark@chromium.org> wrote:
> electricmonopole@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.

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, 2010-03-26 16:47:02]

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.

> struct test {
>  int32_t x;
>  int32_t y;
> };

[Evan Martin, 2010-03-26 16:52:22]

On Fri, Mar 26, 2010 at 9:46 AM, Mark Mentovai <mark@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, 2010-03-26 16:54:33]

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

[jchoi42, 2010-03-31 15:04:02]

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

[Evan Martin, 2010-03-31 15:56:34]

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.

[jchoi42, 2010-03-31 16:09:37]

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?

On 2010/03/31 15:56:34, Evan Martin wrote:
> 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.

[Evan Martin, 2010-03-31 16:16:46]

+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?

[Evan Martin, 2010-03-31 16:45:53]

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.

[jchoi42, 2010-03-31 16:52:02]

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

[jchoi42, 2010-03-31 20:21:56]

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

[Evan Martin, 2010-03-31 20:36:58]

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

[jchoi42, 2010-03-31 20:59:17]

> 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"', {

?

[Evan Martin, 2010-03-31 21:02:51]

On 2010/03/31 20:59:17, jchoi42 wrote:
> > 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"', {
> 
> ?

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

[jchoi42, 2010-03-31 21:24:24]

done!