Update base to extend NaCl support (needed for skia support)

Update base to extend NaCl support (needed for skia support)

TEST="Let //base/process include memory.h, but do not make any changes to
memory.cc. Run `./mojo/tools/mojob.py build`. Observe a link error. Add
the UncheckedMalloc definition to memory.cc. Observe the link error is gone"
BUG=#431
R=phosek@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/c3b05c507e712f4ee65522e0a00e38c735c85244

[Sean Klein, 2015-12-15 18:33:38]

smklein@chromium.org changed reviewers:
+ phosek@chromium.org

[Sean Klein, 2015-12-15 18:34:28]

smklein@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2015-12-15 18:51:35]

https://codereview.chromium.org/1531443002/diff/1/base/memory/BUILD.gn
File base/memory/BUILD.gn (right):

https://codereview.chromium.org/1531443002/diff/1/base/memory/BUILD.gn#newcode49
base/memory/BUILD.gn:49: "discardable_memory.cc",
You might want to enable discardable_memory.{cc,h} too -- their code compiles
trivially and this reduces the amount of conditionalisation.

https://codereview.chromium.org/1531443002/diff/1/base/process/memory.h
File base/process/memory.h (right):

https://codereview.chromium.org/1531443002/diff/1/base/process/memory.h#newco...
base/process/memory.h:52: #if defined(OS_LINUX) || defined(OS_ANDROID) ||
defined(OS_NACL)
Hmm, AdjustOOMScore() is a Linuxism.  We don't have /proc in a NaCl sandbox, so
this function will fail there.

When is AdjustOOMScore() defined?  base/process/memory_stubs.cc defines a no-op
implementation, but it's not referenced by any build files. :-(

Can you add a TEST= line to say how you tested this change, please?

[Sean Klein, 2015-12-15 19:27:08]

Description was changed from

==========
Update base to extend NaCl support (needed for skia support)

BUG=#431
==========

to

==========
Update base to extend NaCl support (needed for skia support)

TEST=`./mojo/tools/mojob.py build`
BUG=#431
==========

[Sean Klein, 2015-12-15 19:29:05]

On 2015/12/15 18:51:35, Mark Seaborn wrote:
> https://codereview.chromium.org/1531443002/diff/1/base/memory/BUILD.gn
> File base/memory/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1531443002/diff/1/base/memory/BUILD.gn#newcode49
> base/memory/BUILD.gn:49: "discardable_memory.cc",
> You might want to enable discardable_memory.{cc,h} too -- their code compiles
> trivially and this reduces the amount of conditionalisation.
> 
> https://codereview.chromium.org/1531443002/diff/1/base/process/memory.h
> File base/process/memory.h (right):
> 
>
https://codereview.chromium.org/1531443002/diff/1/base/process/memory.h#newco...
> base/process/memory.h:52: #if defined(OS_LINUX) || defined(OS_ANDROID) ||
> defined(OS_NACL)
> Hmm, AdjustOOMScore() is a Linuxism.  We don't have /proc in a NaCl sandbox,
so
> this function will fail there.
> 
> When is AdjustOOMScore() defined?  base/process/memory_stubs.cc defines a
no-op
> implementation, but it's not referenced by any build files. :-(
> 
> Can you add a TEST= line to say how you tested this change, please?

The only Test I am doing for this CL is building Mojo (the examples rely on
base, and they are build with the PNaCl Newlib toolchain). At the moment, Mojo
isn't building/running any base tests using NaCl or PNaCl. Do you want me to try
to filter out the "base_unittests" target into a portable version for NaCl?

[Sean Klein, 2015-12-15 19:29:26]

https://codereview.chromium.org/1531443002/diff/1/base/memory/BUILD.gn
File base/memory/BUILD.gn (right):

https://codereview.chromium.org/1531443002/diff/1/base/memory/BUILD.gn#newcode49
base/memory/BUILD.gn:49: "discardable_memory.cc",
On 2015/12/15 18:51:35, Mark Seaborn wrote:
> You might want to enable discardable_memory.{cc,h} too -- their code compiles
> trivially and this reduces the amount of conditionalisation.

Done.

https://codereview.chromium.org/1531443002/diff/1/base/process/memory.h
File base/process/memory.h (right):

https://codereview.chromium.org/1531443002/diff/1/base/process/memory.h#newco...
base/process/memory.h:52: #if defined(OS_LINUX) || defined(OS_ANDROID) ||
defined(OS_NACL)
On 2015/12/15 18:51:35, Mark Seaborn wrote:
> Hmm, AdjustOOMScore() is a Linuxism.  We don't have /proc in a NaCl sandbox,
so
> this function will fail there.
> 
> When is AdjustOOMScore() defined?  base/process/memory_stubs.cc defines a
no-op
> implementation, but it's not referenced by any build files. :-(
> 
> Can you add a TEST= line to say how you tested this change, please?

I think this was a leftover from a linking error, but with a more recent
re-build, appears to no longer be necessary.

[Petr Hosek, 2015-12-15 21:37:15]

lgtm

[Sean Klein, 2015-12-15 21:54:38]

Mark sent this email, but I don't think it was added to this issue:

What I mean is:  This change is motivated by compiling+linking a Skia-based
program with PNaCl.  In building an example Skia-based program, you found that
you needed a definition of UncheckedMalloc() for PNaCl.  However, in the current
Mojo build, no PNaCl-based programs refer to UncheckedMalloc().  So if you do,
"./mojo/tools/mojob.py build", that by itself isn't testing UncheckedMalloc() --
it is compile-testing the definition, but not testing it beyond that because the
linker never pulls that definition in.  Basically, the new UncheckedMalloc() is
currently dead code. :-)

So my question is:  What is your test case that motivates adding
UncheckedMalloc()?

To answer that, you could either:

 1) Make the change "complete" by adding a build target that builds a Skia-based
program with PNaCl.  Sometimes it's easier to review a "complete" change than a
partial one.  How many more changes are required to do this?

 2) Just describe (e.g. in TEST=) how to reproduce the link error that motivated
adding UncheckedMalloc().  e.g. What target did you try to build?

 3) Add a targeted test which exercises UncheckedMalloc() (e.g. base_unittests
as you suggested).

Sorry if this is pedantic, but I think it's good to be clear what's going on
when adding code that is not exercised yet.

 
Do you want me to try to filter out the "base_unittests" target into a portable
version for NaCl?

That could be useful to have for testing purposes, but I don't know how much
work it would be.

[Sean Klein, 2015-12-15 22:01:57]

Description was changed from

==========
Update base to extend NaCl support (needed for skia support)

TEST=`./mojo/tools/mojob.py build`
BUG=#431
==========

to

==========
Update base to extend NaCl support (needed for skia support)

TEST="Let //base/process include memory.h, but do not make any changes to
memory.cc
Run `./mojo/tools/mojob.py build`. Observe a link error. Add the UncheckedMalloc
definition
to memory.cc. Observe the link error is gone"
BUG=#431
==========

[Sean Klein, 2015-12-15 22:02:57]

Description was changed from

==========
Update base to extend NaCl support (needed for skia support)

TEST="Let //base/process include memory.h, but do not make any changes to
memory.cc
Run `./mojo/tools/mojob.py build`. Observe a link error. Add the UncheckedMalloc
definition
to memory.cc. Observe the link error is gone"
BUG=#431
==========

to

==========
Update base to extend NaCl support (needed for skia support)

TEST="Let //base/process include memory.h, but do not make any changes to
memory.cc. Run `./mojo/tools/mojob.py build`. Observe a link error. Add
the UncheckedMalloc definition to memory.cc. Observe the link error is gone"
BUG=#431
==========

[Sean Klein, 2015-12-15 22:03:09]

The need for "UncheckedMalloc" is actually motivated by this CL:

Once "memory.h" is added to the "sources" list for //base/process,
the following link error is seen: "error: undefined reference to
'base::UncheckedMalloc(unsigned int, void**)'".

This is resolved by adding "UncheckedMalloc" for OS_NACL in memory.cc.

Added this explanation to "TEST=".

[Sean Klein, 2015-12-16 00:48:41]

Description was changed from

==========
Update base to extend NaCl support (needed for skia support)

TEST="Let //base/process include memory.h, but do not make any changes to
memory.cc. Run `./mojo/tools/mojob.py build`. Observe a link error. Add
the UncheckedMalloc definition to memory.cc. Observe the link error is gone"
BUG=#431
==========

to

==========
Update base to extend NaCl support (needed for skia support)

TEST="Let //base/process include memory.h, but do not make any changes to
memory.cc. Run `./mojo/tools/mojob.py build`. Observe a link error. Add
the UncheckedMalloc definition to memory.cc. Observe the link error is gone"
BUG=#431
R=phosek@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/c3b05c507e712f4ee65522e0a00...
==========

[Sean Klein, 2015-12-16 00:48:43]

Committed patchset #2 (id:20001) manually as
c3b05c507e712f4ee65522e0a00e38c735c85244 (presubmit successful).