Non-SFI Mode: Build libevent with PNaCl toolchain for nacl_helper_nonsfi.

Non-SFI Mode: Build libevent with PNaCl toolchain for nacl_helper_nonsfi.

This CL is to build libevent for nacl_helper_nonsfi, which
will be depended on by base/ library (in more precise, nacl_helper_nonsfi
will use MessagePumpLibevent for MessageLoopForIO).
This CL includes:
- Add .gyp file.
- Add config.h and event-config.h customized for PNaCL toolchain.
- Add random.c to support random() function, which is not provided by the newlib-based PNaCl toolchain.
Note that this library is not actually used for now. It is just
for making sure that the building it is successfully done.

BUG=358465
TEST=Ran trybot.
TBR=darin@chromium.org

Committed: https://crrev.com/b919fef97f4868147f14cbfd6561b16b21552257
Cr-Commit-Position: refs/heads/master@{#299502}

[hidehiko, 2014-10-09 17:17:53]

hidehiko@chromium.org changed reviewers:
+ hamaji@chromium.org, mazda@chromium.org, mseaborn@chromium.org,
uekawa@chromium.org

[hidehiko, 2014-10-09 17:17:54]

This CL is depending on crrev.com/623123002, which is now in CQ.
I'll run trybot when the CL is landed.
Please let me send this out to the code review for now, to move the project
forward.

Thank you for your review in advance,
- hidehiko

[hidehiko, 2014-10-09 19:52:04]

On 2014/10/09 17:17:54, hidehiko wrote:
> This CL is depending on crrev.com/623123002, which is now in CQ.
> I'll run trybot when the CL is landed.
> Please let me send this out to the code review for now, to move the project
> forward.
> 
> Thank you for your review in advance,
> - hidehiko

[hidehiko, 2014-10-09 19:52:26]

On 2014/10/09 19:52:04, hidehiko wrote:
> On 2014/10/09 17:17:54, hidehiko wrote:
> > This CL is depending on crrev.com/623123002, which is now in CQ.
> > I'll run trybot when the CL is landed.
> > Please let me send this out to the code review for now, to move the project
> > forward.
> > 
> > Thank you for your review in advance,
> > - hidehiko

FYI, triggered the trybots.

[Junichi Uekawa, 2014-10-10 02:47:52]

https://codereview.chromium.org/646643002/diff/100001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/646643002/diff/100001/components/nacl_nonsfi....
components/nacl_nonsfi.gyp:42: 'extra_deps_newlib32_nonsfi': [
dumb question, where does this symbol get used ?

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/RE...
File third_party/libevent/README.chromium (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/RE...
third_party/libevent/README.chromium:23: linux/ and manually modified to adapt
PNaCl toolchain.
try:
Add libevent_nacl.gyp, config.h, and event-config.h to nacl/ for building the
library
with PNaCl toolchain. config.h and event-config.h are modified copies of those
found under 
   linux/ adapted for PNaCl toolchain.

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/ev...
File third_party/libevent/event-config.h (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/ev...
third_party/libevent/event-config.h:10: #elif defined(__native_client_nonsfi__)
is there a reason why this is after __APPLE__ ?
Non-SFI NaCl may build for Mac OS X, but that doesn't define __APPLE__ ?

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
File third_party/libevent/nacl/config.h (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
third_party/libevent/nacl/config.h:1: /* Define if clock_gettime is available in
libc */
maybe add a header that this is manually generated from ../linux/config.h ?

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
File third_party/libevent/nacl/event-config.h (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:1: #ifndef _EVENT_CONFIG_H_
ditto

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:273: static inline int random() {
return rand(); }
wow.

[hidehiko, 2014-10-10 06:27:41]

Thank you for review. PTAL.

https://codereview.chromium.org/646643002/diff/100001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/646643002/diff/100001/components/nacl_nonsfi....
components/nacl_nonsfi.gyp:42: 'extra_deps_newlib32_nonsfi': [
On 2014/10/10 02:47:52, Junichi Uekawa wrote:
> dumb question, where does this symbol get used ? 

In native_client/build/untrusted.gypi's dependency rule.

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/RE...
File third_party/libevent/README.chromium (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/RE...
third_party/libevent/README.chromium:23: linux/ and manually modified to adapt
PNaCl toolchain.
On 2014/10/10 02:47:52, Junichi Uekawa wrote:
> try:
> Add libevent_nacl.gyp, config.h, and event-config.h to nacl/ for building the
> library
> with PNaCl toolchain. config.h and event-config.h are modified copies of those
> found under 
>    linux/ adapted for PNaCl toolchain.

Done, according to offline talk. Thank you for help!

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/ev...
File third_party/libevent/event-config.h (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/ev...
third_party/libevent/event-config.h:10: #elif defined(__native_client_nonsfi__)
On 2014/10/10 02:47:52, Junichi Uekawa wrote:
> is there a reason why this is after __APPLE__ ?
> Non-SFI NaCl may build for Mac OS X, but that doesn't define __APPLE__ ?

Swapped. No reasons are there, since we're not planning to build
nacl_helper_nonsfi on Mac OS, so the conditions should be mutually exclusive.

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
File third_party/libevent/nacl/config.h (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
third_party/libevent/nacl/config.h:1: /* Define if clock_gettime is available in
libc */
On 2014/10/10 02:47:52, Junichi Uekawa wrote:
> maybe add a header that this is manually generated from ../linux/config.h ?

Done.

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
File third_party/libevent/nacl/event-config.h (right):

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:1: #ifndef _EVENT_CONFIG_H_
On 2014/10/10 02:47:52, Junichi Uekawa wrote:
> ditto

Done.

https://codereview.chromium.org/646643002/diff/100001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:273: static inline int random() {
return rand(); }
On 2014/10/10 02:47:52, Junichi Uekawa wrote:
> wow.

Acknowledged.

[Mark Seaborn, 2014-10-13 16:02:01]

https://codereview.chromium.org/646643002/diff/300001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/646643002/diff/300001/components/nacl_nonsfi....
components/nacl_nonsfi.gyp:54: # This dependency is introduced to make sure the
library is actualy
"actually"

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/RE...
File third_party/libevent/README.chromium (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/RE...
third_party/libevent/README.chromium:21: 7) Add files to support building with
PNaCl toolchain. Added libevent_nacl.gyp
Nit: "the PNaCl toolchain"

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
File third_party/libevent/libevent_nacl.gyp (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:4: 
Can you name this file "libevent_nacl_nonsfi.gyp"?  Otherwise readers might
think we're building this to run as user code under SFI NaCl, when we're really
building this to run directly under Linux.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:13: 'target_name': 'event_nonsfi_nacl',
Nit: "nonsfi" is used more often as a suffix than a prefix in the code, so could
you name this "event_nacl_nonsfi"?

$ git grep -i 'nacl.*nonsfi' components/nacl ppapi/ | wc -l
230
$ git grep -i 'nonsfi.*nacl' components/nacl ppapi/ | wc -l
68

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:32: 'nacl',
Also "nacl_nonsfi" for this directory?

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:33:
'<(DEPTH)/native_client/src/public/linux_syscalls/',
Nit: Trailing "/" not needed

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
File third_party/libevent/nacl/config.h (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/config.h:1: /* Copied from Linux version and changed
the features according NaCl, which
Please update the comment to reflect that this build *will* be running under
Linux, not just something 'close to Linux'. :-)

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/config.h:101: /* #undef HAVE_SIGACTION */
We do have sigaction(), so presumably you are disabling this usage intentionally
(and manually) to prevent libevent interfering with signal handlers?  That might
be worth commenting.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
File third_party/libevent/nacl/event-config.h (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:272: * by the toolchain, so here we
define the static inline function, to redirect
Nit: "the toolchain" -> "newlib" or "the newlib-based PNaCl toolchain"

"define the..." -> "define a..."

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:276: static inline int random() {
return rand(); }
Nit: Might as well follow the coding style and put the function body on a
separate line.

[hidehiko, 2014-10-14 08:00:56]

Patchset #4 (id:500001) has been deleted

[hidehiko, 2014-10-14 08:04:34]

Patchset #4 (id:520001) has been deleted

[hidehiko, 2014-10-14 09:07:13]

Thank you for review. PTAL.

https://codereview.chromium.org/646643002/diff/300001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/646643002/diff/300001/components/nacl_nonsfi....
components/nacl_nonsfi.gyp:54: # This dependency is introduced to make sure the
library is actualy
On 2014/10/13 16:02:00, Mark Seaborn wrote:
> "actually"

Done.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/RE...
File third_party/libevent/README.chromium (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/RE...
third_party/libevent/README.chromium:21: 7) Add files to support building with
PNaCl toolchain. Added libevent_nacl.gyp
On 2014/10/13 16:02:00, Mark Seaborn wrote:
> Nit: "the PNaCl toolchain"

Done.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
File third_party/libevent/libevent_nacl.gyp (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:4: 
On 2014/10/13 16:02:00, Mark Seaborn wrote:
> Can you name this file "libevent_nacl_nonsfi.gyp"?  Otherwise readers might
> think we're building this to run as user code under SFI NaCl, when we're
really
> building this to run directly under Linux.

Done.
FYI: Though, I'm planning to add nonsfi build rule to exiting _nacl.gyp files
for other libraries, such as base_nacl.gyp or ipc_nacl.gyp etc, as the diff is
very small. If you have any concerns about it, please let me know in advance.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:13: 'target_name': 'event_nonsfi_nacl',
On 2014/10/13 16:02:01, Mark Seaborn wrote:
> Nit: "nonsfi" is used more often as a suffix than a prefix in the code, so
could
> you name this "event_nacl_nonsfi"?
> 
> $ git grep -i 'nacl.*nonsfi' components/nacl ppapi/ | wc -l
> 230
> $ git grep -i 'nonsfi.*nacl' components/nacl ppapi/ | wc -l
> 68

Hmm, the former pattern matches to namespace or include directive, such as
nacl::nonsfi::SomeFunction or #include "components/nacl/loader/nonsfi/...",
while the latter matches some constants, like nacl::nonsfi::kNaClSomething.
And, as far as I looked at, there are few good referecnes in both patterns?

Anyway, I have no strong opinion here, so I changed to nonsfi suffix name.
I'll use the same name convention in following CLs.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:32: 'nacl',
On 2014/10/13 16:02:01, Mark Seaborn wrote:
> Also "nacl_nonsfi" for this directory?

Done.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:33:
'<(DEPTH)/native_client/src/public/linux_syscalls/',
On 2014/10/13 16:02:00, Mark Seaborn wrote:
> Nit: Trailing "/" not needed

Done.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
File third_party/libevent/nacl/config.h (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/config.h:1: /* Copied from Linux version and changed
the features according NaCl, which
On 2014/10/13 16:02:01, Mark Seaborn wrote:
> Please update the comment to reflect that this build *will* be running under
> Linux, not just something 'close to Linux'. :-)

Done.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/config.h:101: /* #undef HAVE_SIGACTION */
On 2014/10/13 16:02:01, Mark Seaborn wrote:
> We do have sigaction(), so presumably you are disabling this usage
intentionally
> (and manually) to prevent libevent interfering with signal handlers?  That
might
> be worth commenting.

newlib ABI sigaction is not provided (i.e. we only have linux_sigaction() for
nonsfi), right?
Added comments.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
File third_party/libevent/nacl/event-config.h (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:272: * by the toolchain, so here we
define the static inline function, to redirect
On 2014/10/13 16:02:01, Mark Seaborn wrote:
> Nit: "the toolchain" -> "newlib" or "the newlib-based PNaCl toolchain"
> 
> "define the..." -> "define a..."

Done.

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/na...
third_party/libevent/nacl/event-config.h:276: static inline int random() {
return rand(); }
On 2014/10/13 16:02:01, Mark Seaborn wrote:
> Nit: Might as well follow the coding style and put the function body on a
> separate line.

Done.

[Mark Seaborn, 2014-10-14 15:48:00]

Commit message nits:
"by PNaCl toolchain" -> "with PNaCl toolchain"
"depended by base" -> "depended on by base"

LGTM

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
File third_party/libevent/libevent_nacl.gyp (right):

https://codereview.chromium.org/646643002/diff/300001/third_party/libevent/li...
third_party/libevent/libevent_nacl.gyp:4: 
On 2014/10/14 09:07:12, hidehiko wrote:
> On 2014/10/13 16:02:00, Mark Seaborn wrote:
> > Can you name this file "libevent_nacl_nonsfi.gyp"?  Otherwise readers might
> > think we're building this to run as user code under SFI NaCl, when we're
> > really
> > building this to run directly under Linux.
> 
> Done.
> FYI: Though, I'm planning to add nonsfi build rule to exiting _nacl.gyp files
> for other libraries, such as base_nacl.gyp or ipc_nacl.gyp etc, as the diff is
> very small. If you have any concerns about it, please let me know in advance.

That's fine.  I just don't want readers to misunderstand what target we're
building libevent for.

https://codereview.chromium.org/646643002/diff/540001/third_party/libevent/na...
File third_party/libevent/nacl_nonsfi/event-config.h (right):

https://codereview.chromium.org/646643002/diff/540001/third_party/libevent/na...
third_party/libevent/nacl_nonsfi/event-config.h:278: int random();
The man page says the return type is "long int".

[hidehiko, 2014-10-14 16:48:44]

hidehiko@chromium.org changed reviewers:
+ darin@chromium.org

[hidehiko, 2014-10-14 16:53:25]

Thank you for review, Mark.

+darin to R and TBR.
Following the instruction in third_party/OWNERS, I added to darin@ to R and TBR,
and am submitting this CL...

https://codereview.chromium.org/646643002/diff/540001/third_party/libevent/na...
File third_party/libevent/nacl_nonsfi/event-config.h (right):

https://codereview.chromium.org/646643002/diff/540001/third_party/libevent/na...
third_party/libevent/nacl_nonsfi/event-config.h:278: int random();
On 2014/10/14 15:48:00, Mark Seaborn wrote:
> The man page says the return type is "long int".

Good catch. Fixed.

[hidehiko, 2014-10-14 16:53:42]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-14 16:54:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646643002/560001

[commit-bot: I haz the power, 2014-10-14 17:31:19]

Committed patchset #5 (id:560001)

[commit-bot: I haz the power, 2014-10-14 17:34:03]

Patchset 5 (id:??) landed as
https://crrev.com/b919fef97f4868147f14cbfd6561b16b21552257
Cr-Commit-Position: refs/heads/master@{#299502}