Non-SFI NaCl: Fix browser_tests based on libc_free.c

Non-SFI NaCl: Fix browser_tests based on libc_free.c

There were two issues with clang:

- Clang emits .data.rel.ro.local for local struct values with
  an initializer, which lets the linker to emit a few
  relocation info.
- In debug build, clang uses memcpy to copy a structure with
  five members. Neither -fno-builtin nor -ffreestanding did
  not prevent this issue.
- In release build, clang translates for-loop based zero copy
  to memset.

This patch initializes all structures without initializers
or copy. This patch works with GYP_DEFINES=clang=0.

To make sure we will not add memcpy or something in future,
we will build libc_free.nexe with -Wl,--no-undefined.

This also reverts

https://codereview.chromium.org/386543002

to enable the disabled tests.

BUG=392768
TEST=./out/Debug/browser_tests --gtest_filter='*NonSfi*Messaging*'

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282697

[hamaji, 2014-07-10 11:32:24]

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/no...
File chrome/test/data/nacl/nonsfi/libc_free.c (right):

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/no...
chrome/test/data/nacl/nonsfi/libc_free.c:224: struct PP_StartFunctions
start_funcs;
The initialization of this structure is OK with -Wl,-z,norelro, but I felt it
would be slightly safer to stick with the simplest code.

[Mark Seaborn, 2014-07-10 17:17:45]

LGTM, but I think you should remove the "norelro" bit (and update the commit
message), because I don't think that part of it is the effective part.

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/na...
chrome/test/data/nacl/nacl_test_data.gyp:1162: '-Wl,-z,norelro',
I don't think adding "norelro" works.

For example:

struct S {
  int *x[6];
};

struct S global1;
struct S global2;

static int val1;
static int val2;

void foo1() {
  struct S local = {
    &val1, &val1, &val1, &val1, &val1, &val1
  };
  global1 = local;
}

void foo2() {
  int i = 0;
  global2.x[i++] = &val2;
  global2.x[i++] = &val2;
  global2.x[i++] = &val2;
  global2.x[i++] = &val2;
  global2.x[i++] = &val2;
  global2.x[i++] = &val2;
}

If you run:

clang libc_free_reloc.c -shared -fPIC -Wl,-z,norelro && objdump -R a.out

then you'll see there are still R_X86_64_RELATIVE relocs on 'local' that refer
to val1.  The linker can't make those go away when producing a
position-independent binary.

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/no...
File chrome/test/data/nacl/nonsfi/libc_free.c (right):

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/no...
chrome/test/data/nacl/nonsfi/libc_free.c:224: struct PP_StartFunctions
start_funcs;
On 2014/07/10 11:32:24, hamaji wrote:
> The initialization of this structure is OK with -Wl,-z,norelro, but I felt it
> would be slightly safer to stick with the simplest code.

I don't think that's right.  I don't think "norelro" can remove the relocations
that the compiler produces.  I think the change to libc_free.c is the part of
the workaround that's effective.

FYI, LLVM's optimiser might still turn this back into the original code.  To be
safer against future compiler changes, you'd need to either:
 * disable optimisations -- less robust
 * make these assignments 'volatile' -- more robust, but not fully
compiler-proof, because the compiler could still feel free to introduce globals
containing relocs.

That's just FYI, so no change to libc_free.c is required.

[Nico, 2014-07-10 22:15:47]

(You will also want to revert https://codereview.chromium.org/382013002/ when
you land this)

[hamaji, 2014-07-11 05:16:48]

thakis: IrtManifest seems to have a different issue. In short, I think the test
code is broken and clang found the issue. I'll write a fix in a separate change.

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/na...
chrome/test/data/nacl/nacl_test_data.gyp:1162: '-Wl,-z,norelro',
On 2014/07/10 17:17:45, Mark Seaborn wrote:
> I don't think adding "norelro" works.
> 
> For example:
> 
> struct S {
>   int *x[6];
> };
> 
> struct S global1;
> struct S global2;
> 
> static int val1;
> static int val2;
> 
> void foo1() {
>   struct S local = {
>     &val1, &val1, &val1, &val1, &val1, &val1
>   };
>   global1 = local;
> }
> 
> void foo2() {
>   int i = 0;
>   global2.x[i++] = &val2;
>   global2.x[i++] = &val2;
>   global2.x[i++] = &val2;
>   global2.x[i++] = &val2;
>   global2.x[i++] = &val2;
>   global2.x[i++] = &val2;
> }
> 
> If you run:
> 
> clang libc_free_reloc.c -shared -fPIC -Wl,-z,norelro && objdump -R a.out
> 
> then you'll see there are still R_X86_64_RELATIVE relocs on 'local' that refer
> to val1.  The linker can't make those go away when producing a
> position-independent binary.

True. I might have been testing this in a wrong way. Removed.

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/no...
File chrome/test/data/nacl/nonsfi/libc_free.c (right):

https://codereview.chromium.org/381883002/diff/20001/chrome/test/data/nacl/no...
chrome/test/data/nacl/nonsfi/libc_free.c:224: struct PP_StartFunctions
start_funcs;
On 2014/07/10 17:17:45, Mark Seaborn wrote:
> On 2014/07/10 11:32:24, hamaji wrote:
> > The initialization of this structure is OK with -Wl,-z,norelro, but I felt
it
> > would be slightly safer to stick with the simplest code.
> 
> I don't think that's right.  I don't think "norelro" can remove the
relocations
> that the compiler produces.  I think the change to libc_free.c is the part of
> the workaround that's effective.
> 
> FYI, LLVM's optimiser might still turn this back into the original code.  To
be
> safer against future compiler changes, you'd need to either:
>  * disable optimisations -- less robust
>  * make these assignments 'volatile' -- more robust, but not fully
> compiler-proof, because the compiler could still feel free to introduce
globals
> containing relocs.
> 
> That's just FYI, so no change to libc_free.c is required.

Acknowledged.

[hamaji, 2014-07-11 05:16:56]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-07-11 05:18:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/381883002/40001

[commit-bot: I haz the power, 2014-07-11 07:14:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-07-11 07:23:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-11 07:23:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[hamaji, 2014-07-11 07:46:16]

LLVM optimizer is too smart. I guess it would be OK to land this without the
last check from Mark, but I'd wait for a day just in case. We have no reason to
land this in hurry.

https://codereview.chromium.org/381883002/diff/60001/chrome/test/data/nacl/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/381883002/diff/60001/chrome/test/data/nacl/na...
chrome/test/data/nacl/nacl_test_data.gyp:1142: # Optimizers may translate the
original code to code which
Sorry, the previous patch was not sufficient for release build. I didn't notice
this yesterday because trybots were not working.

[Mark Seaborn, 2014-07-11 16:27:24]

LGTM

https://codereview.chromium.org/381883002/diff/60001/chrome/test/data/nacl/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/381883002/diff/60001/chrome/test/data/nacl/na...
chrome/test/data/nacl/nacl_test_data.gyp:1143: # requires builtin functions
and/or relocations. Actually,
Nit: 'Actually' -> 'Specifically', perhaps?

[hamaji, 2014-07-11 18:34:36]

https://codereview.chromium.org/381883002/diff/60001/chrome/test/data/nacl/na...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/381883002/diff/60001/chrome/test/data/nacl/na...
chrome/test/data/nacl/nacl_test_data.gyp:1143: # requires builtin functions
and/or relocations. Actually,
On 2014/07/11 16:27:24, Mark Seaborn wrote:
> Nit: 'Actually' -> 'Specifically', perhaps?

Done.

[hamaji, 2014-07-11 18:34:49]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-07-11 18:36:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/381883002/80001

[commit-bot: I haz the power, 2014-07-11 21:06:20]

Change committed as 282697