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

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

This was previously reviewed in
https://codereview.chromium.org/381883002/

The original patch was reverted because it broke LSAN builder.
There were some undefined LSAN related symbols and linker
started complaining as we added -Wl,--no-undefined.

Now we do not set -fsanitize=* flags so we do not use sanitizer
tools in libc_free.nexe.

The original description:

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=283380

[hamaji, 2014-07-14 08:22:52]

The patch set 1 is the original patch.

[Mark Seaborn, 2014-07-15 23:12:52]

LGTM

Are the LSAN bots new?  Presumably LSAN would have had a problem with this test
before the main Linux bots switched to using Clang.  I'm not sure whether LSAN
requires Clang.

[hamaji, 2014-07-16 05:06:22]

On 2014/07/15 23:12:52, Mark Seaborn wrote:
> LGTM
> 
> Are the LSAN bots new?  Presumably LSAN would have had a problem with this
test
> before the main Linux bots switched to using Clang.  I'm not sure whether LSAN
> requires Clang.

I think the LSAN bot isn't new. The previous patch broke it not because we
switch to clang, but because I added --no-undefined. I'm not sure why it is
working without some ASAN/LSAN specific symbols, though.

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

On 2014/07/16 05:06:22, hamaji wrote:
> On 2014/07/15 23:12:52, Mark Seaborn wrote:
> > LGTM
> > 
> > Are the LSAN bots new?  Presumably LSAN would have had a problem with this
> test
> > before the main Linux bots switched to using Clang.  I'm not sure whether
LSAN
> > requires Clang.
> 
> I think the LSAN bot isn't new. The previous patch broke it not because we
> switch to clang, but because I added --no-undefined. I'm not sure why it is
> working without some ASAN/LSAN specific symbols, though.

Ah, I think now I understood. ASAN/LSAN builders are not working for 32bit. They
are only for x64. On x64, we don't run these tests. Leaving a few ASAN/LSAN
undefined symbols in libc_free.nexe won't be a problem because we never run it.
However, we are building x64 libc_free.nexe. So, adding -Wl,--no-undefined
caused a build breakage.

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

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-07-16 05:17:54]

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

[commit-bot: I haz the power, 2014-07-16 07:11:37]

Change committed as 283380

[Mark Seaborn, 2014-07-17 18:26:09]

On 15 July 2014 22:11, <hamaji@chromium.org> wrote:

> On 2014/07/16 05:06:22, hamaji wrote:
>
>> On 2014/07/15 23:12:52, Mark Seaborn wrote:
>> > LGTM
>> >
>> > Are the LSAN bots new?  Presumably LSAN would have had a problem with
>> this test
>> > before the main Linux bots switched to using Clang.  I'm not sure
>> whether LSAN
>
>  > requires Clang.
>>
>
>  I think the LSAN bot isn't new. The previous patch broke it not because we
>> switch to clang, but because I added --no-undefined. I'm not sure why it
>> is
>> working without some ASAN/LSAN specific symbols, though.
>
>
Ah, that makes sense.  Thanks for the explanation.



> Ah, I think now I understood. ASAN/LSAN builders are not working for
> 32bit. They
> are only for x64. On x64, we don't run these tests. Leaving a few ASAN/LSAN
> undefined symbols in libc_free.nexe won't be a problem because we never
> run it.
> However, we are building x64 libc_free.nexe. So, adding -Wl,--no-undefined
> caused a build breakage.


FWIW, we *do* run the libc-free tests on x86-64 Linux.  They are the only
test coverage we have for Non-SFI NaCl on x86-64, which is important
because it's the only test coverage we have for Non-SFI NaCl on the default
trybots and CQ.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2014-07-17 18:26:10]

On 15 July 2014 22:11, <hamaji@chromium.org> wrote:

> On 2014/07/16 05:06:22, hamaji wrote:
>
>> On 2014/07/15 23:12:52, Mark Seaborn wrote:
>> > LGTM
>> >
>> > Are the LSAN bots new?  Presumably LSAN would have had a problem with
>> this test
>> > before the main Linux bots switched to using Clang.  I'm not sure
>> whether LSAN
>
>  > requires Clang.
>>
>
>  I think the LSAN bot isn't new. The previous patch broke it not because we
>> switch to clang, but because I added --no-undefined. I'm not sure why it
>> is
>> working without some ASAN/LSAN specific symbols, though.
>
>
Ah, that makes sense.  Thanks for the explanation.



> Ah, I think now I understood. ASAN/LSAN builders are not working for
> 32bit. They
> are only for x64. On x64, we don't run these tests. Leaving a few ASAN/LSAN
> undefined symbols in libc_free.nexe won't be a problem because we never
> run it.
> However, we are building x64 libc_free.nexe. So, adding -Wl,--no-undefined
> caused a build breakage.


FWIW, we *do* run the libc-free tests on x86-64 Linux.  They are the only
test coverage we have for Non-SFI NaCl on x86-64, which is important
because it's the only test coverage we have for Non-SFI NaCl on the default
trybots and CQ.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.