Same as  issue 760248 ?  Adding Bernie to help us track down an owner.

Brad, is this in your jurisdiction (you seem to have several other NaCl bugs)?

 Issue 757379  has been merged into this issue.

Derek, Bill, do either of you have a working arm device to try this on?
Let's sync tomorrow.

FWIW, I loaded 63.0.3206.0 (9916.0.0) - latest-dev - onto a Minnie and reproduced the NaCl issue reported here by trying to load the bullet demo in the initial report.

[5642:5770:0906/094733.640265:VERBOSE2:ppapi_plugin_process_host.cc(460)] ppapi plugin process launched.
[5642:5642:0906/094733.950483:VERBOSE1:instance_holder.h(67)] Instance arc::mojom::IntentHelperInstance not available.
[5642:5642:0906/094734.642723:VERBOSE1:arc_session.cc(591)] Notified that ARC instance is stopped cleanly
[5642:5642:0906/094734.642844:INFO:arc_session_runner.cc(198)] ARC stopped: SHUTDOWN
[5642:5770:0906/094734.742732:ERROR:service_manager.cc(156)] Connection InterfaceProviderSpec prevented service: content_browser from binding interface: content::mojom::Child exposed by: nacl_loader
[5642:5770:0906/094734.933024:ERROR:nacl_process_host.cc(256)] NaCl process exited with status 64000 (0xfa00)
[5642:5770:0906/094813.659611:VERBOSE1:ppapi_plugin_process_host.cc(496)] PpapiPluginProcessHost[broker]::OnChannelError()

I also launched a nonsfi process (two ARC Chrome apps, OverDrive and Starry Night) and it launched without issue.

Bisecting Chrome OS, I found 9817.0.0 (Chrome 62.0.3176.0) loads the bullet demo but 9818.0.0 (Chrome 62.0.3176.0 - same Chrome version) fails. This makes me suspect something on the Chrome OS side has changed here.

i don't see anything in that OS delta that looks relevant from devs:
  https://crosland.corp.google.com/log/9817.0.0..9818.0.0

if we look at bot commits, there is a SDK toolchain roll.  backing up a bit, i see this toolchain commit to update LLVM which i think is new to the 9818.0.0 builds:
  https://chromium-review.googlesource.com/602683

I am trying to verify if LLVM roll is the root cause.

I tried with Elm and can't reproduce it. Trying to set up a Minnie now.

I can repro the problem with Minnie.  Any pointers about how do I debug this issue?

Ping!! How can I trace or debug what is going wrong with NaCl?
I have put NACLVERBOSITY=4 in /etc/chrome_dev.conf but I don't see any NaCl logs in /var/log/messages or /var/log/chrome/chrome.

I've gotten some output in /var/log/ui/ui.LATEST (attached).

I'll see if I can build/deploy a debug version of chrome for added info.

Deleted: ui.LATEST 67.0 KB

ui.LATEST 67.0 KB Download

when you were able to repro it w/the new llvm, was it due to the roll, or was it the switch to compiler-rt ?

if you deploy a working 9817.0.0 image, then build just Chrome using the new toolchain and deploy it, does it now fail ?  would be nice to discount system level changes (other than the toolchain).

also, i believe the nacl runtime is entirely isolated.  so if you took a 9817.0.0 working image and copied over just the nacl runtime from a failing 9818.0.0, does it start failing ?  and vice versa (failing 9818.0.0 + 9817.0.0 nacl == working ?).

i'm not super familiar with the on-disk layout, but i think just the /opt/google/chrome/nacl_* files.

Disabling compiler-rt fixes the issue. I am trying to find out which builtin is causing the problem.

But the fail is only on ARMv7 boards. Armv8 (32-bit) and amd64 boards are fine.

NaCl failing run has this extra information. The rest of the log before it is mostly the same.

** Signal 6 from trusted code: pc=b28ebb86


> if you took a 9817.0.0 working image and copied over just the nacl runtime from a failing 9818.0.0, does it start failing ? and vice versa (failing 9818.0.0 + 9817.0.0 nacl == working ?)

Copying nacl files from 9817.0.0 to 9818.0.0 => NaCl works
Copying nacl files from 9818.0.0 to 9817.0.0 => NaCl fails.

So does this imply that the problem is in building NaCl, not the chrome binary? How is NaCl built and is there a way to pass an extra Link flag to it?

the nacl binaries are built as part of Chrome, so you should be able to do a simple chrome build using the two toolchains to narrow things down faster.  the nacl binaries are just sep targets to ninja so you don't need to even build Chrome.  although i don't know off hand what those targets are exactly ... would i be naive to assume `ninja nacl_irt_x86_64.nexe` would work ?

yes, take a look at the ninja command.. there are 4 or 5 targets there. finding the right target would simplify things for you...

Can confirm that building only nacl_helper with libgcc (Clang link option "-rtlib=libgcc") fixes the issue.

Interestingly, all other nacl binaries are built with nacl toolchain except nacl_helper.

Is there an easy way to enable this link option just for nacl_helper binary linking?

nacl_helper is a host binary, not a nacl binary, so you wouldn't want to build it with the nacl toolchains.

If the problem is as you say, adding "-rtlib=libgcc" to 

https://cs.chromium.org/chromium/src/components/nacl/loader/BUILD.gn?l=135

should be all you need.

I defer to bradnelson/dschuff/llozano/vapier as to whether that's the right thing to do or not, though.

As you say, the other binaries are built with the nacl toolchain, which only includes compiler_rt.
nacl_helper is a mostly-ordinary host binary, and it should be fine to link it with libgcc instead of compiler_rt if you need to as a stopgap. However if changing the rtlib is the only difference in reproducing the bug then it seems likely to be a bug in the compiler_rt library or possibly the toolchain, which is something we'd want to fix.

There could be a bug in one of the builtins, or possibly in the way the library is built (we do link nacl_helper as a PIE, so if compiler_rt is a static non-PIC library that might cause a problem). Do we build chrome with the same toolchain on CrOS that we do on desktop Linux?

dschuff@ Linking with libgcc is only intended as a temporary stop gap measure since this is a P0 bug.
All of ChromeOS has been built using compiler-rt in R62.  So far, this is the only issue with compiler-rt that we know of. 
But, as this is the first time compiler-rt has been used, I can't rule out any bugs in the builtins. But the fact that this issue is observed only on ARMv7 board but not 32-bit ARMv8 boards e.g. ELM is strange since the builtins are identical for both.
I do not think this is a PIC issue since most of ChromeOS uses pic/pie. But I'll check the compiler-rt build logs to verify that.

CroS toolchain is different from Linux desktop toolchain. I think that the desktop toolchain doesn't use compiler-rt yet.

i'd suggest landing a workaround now (i.e. -rtlib=libgcc) since this has been holding up the dev channel for a while, and users are reporting problems with nacl (like unable to run Secure Shell).  it doesn't prevent further analysis and is easy to back out once the real fix is found.

Can we land the workaround today? We typically do a dev on Tuesdays which would be building this evening for tomorrow. 

If we are just changing a flag, consider it merge approved for 62.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7772f5b0029ee8f4592b437ef5554914548d0d3c

commit 7772f5b0029ee8f4592b437ef5554914548d0d3c
Author: Manoj Gupta <manojgupta@google.com>
Date: Mon Sep 11 19:58:54 2017

nacl_helper: Use libgcc for linking in ChromeOS.

NaCl isn't working in ChromeOS when compiler-rt is used. Force
libgcc to unblock NaCl.

BUG= chromium:761103 
TEST=https://gonativeclient.appspot.com/demo/bullet works.

Change-Id: I6ca5a7734c088c986caf5bdc45da1715d100d36e
Reviewed-on: https://chromium-review.googlesource.com/658957
Commit-Queue: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501004}
[modify] https://crrev.com/7772f5b0029ee8f4592b437ef5554914548d0d3c/components/nacl/loader/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24aa1b63806e82818ab2a8822e5caef65a03b4d3

commit 24aa1b63806e82818ab2a8822e5caef65a03b4d3
Author: Manoj Gupta <manojgupta@google.com>
Date: Mon Sep 11 20:01:17 2017

nacl_helper: Use libgcc for linking in ChromeOS.

NaCl isn't working in ChromeOS when compiler-rt is used. Force
libgcc to unblock NaCl.

BUG= chromium:761103 
TEST=https://gonativeclient.appspot.com/demo/bullet works.

(cherry picked from commit cfef30353e49ca6e864fbc52a6cb13ba7843e63f)

TBR=manojgupta@google.com

Change-Id: I875c76c45ed25425df8ad8f29432aba1f7f22a7b
Reviewed-on: https://chromium-review.googlesource.com/661219
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#144}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/24aa1b63806e82818ab2a8822e5caef65a03b4d3/components/nacl/loader/BUILD.gn

This should be in 62.0.3202.17+ Chrome.

We will try to have a fixed Chrome version for tomorrow's Dev.

Debugging further shows something strange in the function  <__aeabi_read_tp>: 

Disassembly shows illegal instructions in this function when linked with compiler-rt.

000d6f50 <__aeabi_read_tp>:
mrc     15, 0, r0, cr13, cr0, {3}
bx      lr
nop
stmia   r0!, {r2}
b.n     d6a9c <_Unwind_RaiseException+0x8>
stmia   r0!, {r2, r3}
b.n     d7080 <__aeabi_read_tp+0x130>
vrhadd.u16      d14, d12, d31
strb    r1, [r0, #27]
vqshl.u32       q10, q12, #25
nop                     ; (mov r8, r8)
stmia   r0!, {}
b.n     d6ab0 <_Unwind_RaiseException+0x1c>
blx     4e608c <_end+0x2e98f4>
lsls    r0, r1, #8
vqshl.u32       q10, q12, #20
nop                     ; (mov r8, r8)
stmia   r0!, {}
b.n     d6ac0 <_Unwind_Resume+0x8>
blx     4e609c <_end+0x2e9904>
vminnm.f<illegal width 64>      <illegal reg q7.5>, q10, <illegal reg q9.5>
bx      pc
nop                     ; (mov r8, r8)
stmia   r0!, {}
b.n     d6ad0 <_Unwind_Resume+0x18>
blx     4e60ac <_end+0x2e9914>
vmaxnm.f<illegal width 64>      <illegal reg q15.5>, q10, <illegal reg q9.5>
bx      pc

With the work around, this no longer blocks release. 

Changing the summary to reflect the current status.

The illegal instructions went away after linking compiler-rt + gcc_s (for exceptions handling). NaCL still fails so still checking.

Manual disabling of compiler-rt functions found that the root cause is clear_cache function.

https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/clear_cache.c

Disabling this function made NaCl work with compiler-rt.
Now need to understand what is going wrong with clear_cache function.

This is the definition in the post processed file.

void __clear_cache(void *start, void *end) {
# 116 "/var/tmp/portage/cross-armv7a-cros-linux-gnueabi/compiler-rt-5.0_pre305632-r3/work/compiler-rt-5.0_pre305632/lib/builtins/clear_cache.c"
         register int start_reg __asm("r0") = (int) (intptr_t) start;
         const register int end_reg __asm("r1") = (int) (intptr_t) end;
         const register int flags __asm("r2") = 0;
         const register int syscall_nr __asm("r7") = 0x0f0002;
         __asm __volatile("svc 0x0"
                          : "=r"(start_reg)
                          : "r"(syscall_nr), "r"(start_reg), "r"(end_reg),
                            "r"(flags));
         if (start_reg != 0) {
             compilerrt_abort_impl("/var/tmp/portage/cross-armv7a-cros-linux-gnueabi/compiler-rt-5.0_pre305632-r3/work/compiler-rt-5.0_pre305632/lib/builtins/clear_cache.c", 125, __func__);
         }
# 176 "/var/tmp/portage/cross-armv7a-cros-linux-gnueabi/compiler-rt-5.0_pre305632-r3/work/compiler-rt-5.0_pre305632/lib/builtins/clear_cache.c"
}

The assembly code shows a call to abort if popeq at offset 10 is not executed:

00000000 <__clear_cache>:
   0:   b580            push    {r7, lr}
   2:   2702            movs    r7, #2
   4:   2200            movs    r2, #0
   6:   f2c0 070f       movt    r7, #15
   a:   df00            svc     0
   c:   2800            cmp     r0, #0
   e:   bf08            it      eq
  10:   bd80            popeq   {r7, pc}
  12:   4803            ldr     r0, [pc, #12]   ; (20 <__clear_cache+0x20>)
  14:   4a03            ldr     r2, [pc, #12]   ; (24 <__clear_cache+0x24>)
  16:   217d            movs    r1, #125        ; 0x7d
  18:   4478            add     r0, pc
  1a:   447a            add     r2, pc
  1c:   f7ff fffe       bl      0 <compilerrt_abort_impl>
  20:   00000004        .word   0x00000004
  24:   00000006        .word   0x00000006

I am a bit confused here. Is it understood why the first argument is not just a nullptr for these cases? Without having a better understanding of ARM's implementation of this system call, I think that removing the abort() might hide a different kind of problem (like a hardware inability to actually just flush some subset of the cache). We'll talk more to ARM tomorrow about this.

the code can be a little confusing, but note that start_reg is both an input and an output to the inline asm.  so the abort is happening because the syscall(SYS_ARM_cacheflush,...) call is returning an error.

why is that error happening ?  i don't think it's a seccomp violation because we would have gotten a different error (pretty sure).

as for why clear_cache is being called in this way, no idea.  i don't think NaCl is explicitly making this call (at least codesearch isn't finding any hits) which means it's a call generated by llvm itself.  which presumably means that it not being a NULL pointer is what we want.

Actually NaCl is calling clear_cache through the __builtin___clear_cache

https://cs.chromium.org/chromium/src/native_client/src/include/concurrency_ops.h?type=cs&q=__builtin___clear_cache&sq=package:chromium&l=99

More clarfication:  Seems like the code is checking the value in r0 (not the address) after the svc call and possibly kernel returns an error code.

This is the implementation of cache op in kernel but I am not sure how to get the exact error code out from NaCl.
http://elixir.free-electrons.com/linux/latest/source/arch/arm/kernel/traps.c#L579

might be easier to build a custom kernel on your system with debug printfs added to the clear cache code paths ;)

 Issue 760248  has been merged into this issue.

At c28, this *was* supposed to block dev, right? Getting user reports of breakage. Considering this bug was reported on August 31 and that m62 branched on Sep 1, dev should have never been cut without this bug being fixed.

i'm pretty sure we shipped a dev image before the bug was found, and then we through a blocker label to help up the priority/visibility.  but it's been cherry picked back now, so new builds should be fixed.

Issue 763567 has been merged into this issue.

Kernel logs show -EFAULT is returned when clear_cache is called.

http://elixir.free-electrons.com/linux/latest/source/include/uapi/asm-generic/errno-base.h
#define	EFAULT		14	/* Bad address */

Can NaCl team check the sanity of the addresses being passed to __builtin___clear_cache here:
https://cs.chromium.org/chromium/src/native_client/src/include/concurrency_ops.h?type=cs&q=__builtin___clear_cache&sq=package:chromium&l=58

There are two calls to the __builtin___clear_cache. The first one is fine but the second one results in -EFAULT being returned internally in kernel.

  __builtin___clear_cache((char *) writable_addr,
                          (char *) writable_addr + size); // This is fine
  __builtin___clear_cache((char *) executable_addr,
                          (char *) executable_addr + size); // This one triggers the error

Most likely this is because the virtual addresses passed in the second call are't mapped (v7_coherent_user_range function in http://elixir.free-electrons.com/linux/v3.14.79/source/arch/arm/mm/cache-v7.S.)

I'm pretty sure that the mappings at executable_addr are OK.  They start off as mmap()'d from a file as PROT_NONE, but they are mprotect()'d to PROT_READ|PROT_EXEC some time before we do the cache flush on them.

Maybe the cache_flush syscall doesn't like operating on read-only pages.  Or maybe the mprotect() call operates lazily and doesn't actually change the page table entries yet.  (The PTEs might only get updated on a page fault, and maybe the cache flush operation doesn't trigger that updating.)

A possible fix is to change compiler-rt's cache_flush wrapper not to abort if the syscall returns an error.

my personal opinion is that compiler-rt's abort usage is pointlessly hostile.  but if llvm isn't interested in changing their behavior, nacl itself can always workaround this by making the syscall directly.

I have sent a CL to llvm upstream to remove abort() in clear_cache. But upstream reviewers want more analysis of the underlying issue (https://reviews.llvm.org/D37788).
 
mseaborn@ If you can provide a more precise analysis, It will be easier to convince upstream.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/1697df2534816342993337cb64b8c2ed1691d44f

commit 1697df2534816342993337cb64b8c2ed1691d44f
Author: Manoj Gupta <manojgupta@google.com>
Date: Fri Sep 15 23:54:09 2017

compiler-rt: Patches for ARM softfp and clear_cache bugs.

clear_cache: Disable abort() if ARM syscall may error.

Also Cherry pick upstream CL 4854a215fc3c0b10ab57b4b9b5e4cbeb5bf0624a
Author:     Oleg Ranevskyy <oranevskyy@accesssoftek.com>
CommitDate: Wed Aug 23 14:26:31 2017 +0000

    [ARM][Compiler-rt] Fix AEABI builtins to correctly pass arguments to non-AEABI functions on HF targets

    Summary:
    This is a patch for PR34167.

    On HF targets functions like `__{eq,lt,le,ge,gt}df2` and `__{eq,lt,le,ge,gt}sf2` expect their arguments to be passed in d/s
registers, while some of the AEABI builtins pass them in r registers.

    Reviewers: compnerd, peter.smith, asl

    Reviewed By: peter.smith, asl

    Subscribers: peter.smith, aemerson, dberris, javed.absar, llvm-commits, asl, kristof.beyls

    Differential Revision: https://reviews.llvm.org/D36675

BUG= chromium:761103 
BUG= chromium:705071 

TEST=NaCL on ARM works.
TEST=ARM softfp code with mfpu=none and mfloat-abi=softfp is correct for cmp builtins.

Change-Id: I399ebae1e69dbb6727b89dffa062de72dbf39a62
Reviewed-on: https://chromium-review.googlesource.com/669661
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Commit-Queue: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
Trybot-Ready: Manoj Gupta <manojgupta@chromium.org>

[add] https://crrev.com/1697df2534816342993337cb64b8c2ed1691d44f/sys-libs/compiler-rt/files/compiler-rt-disable-abort-cacheflush.patch
[rename] https://crrev.com/1697df2534816342993337cb64b8c2ed1691d44f/sys-libs/compiler-rt/compiler-rt-5.0_pre305632-r4.ebuild
[add] https://crrev.com/1697df2534816342993337cb64b8c2ed1691d44f/sys-libs/compiler-rt/files/cherry/4854a215fc3c0b10ab57b4b9b5e4cbeb5bf0624a.patch

I have patched compiler-rt on ChromeOS to remove the abort() but NaCL analysis is still needed for upstream review.

a)Tested URL https://gonativeclient.appspot.com/demo/bullet mentioned in Comment #0 and working fine in 62.0.3202.28/9901.21.0 dev-channel Daisy.

b)Installed 'PNaclExamples' App ,tested Audio API,Multi-Threaded Earth Demo,Flocking Geese Demo on 62.0.3202.28/9901.21.0.

c)Checked Duped  Issue 757379  (In comment#4) also working fine on latest M-62 Dev i.e. on 62.0.3202.28/9901.21.0 Daisy device.

Ping NaCl team. Is there an analysis I can send to upstream llvm?

This was reproducible on earlier M62 builds using Daisy device [62.0.3201.0 & 62.0.3202.8] & Blaze [62.0.3202.7]

This is working fine on M61 build # 61.0.3163.0 [Daisy device], hence adding the respective labels.

@manojgupta: I haven't got an ARM Chromebook to test this on, so it's hard to tell what causes the syscall to return an error.  Could you try running the attached C program on an ARM Chromebook for me?  Is it practical to run it through strace to see what the cache_flush() syscall returns?  If not, we can modify the program to call the syscall directly and print the return value.

Deleted: cache_flush_readonly.c 354 bytes

cache_flush_readonly.c 354 bytes View Download

Thanks Mark. I tested this on the custom kernel I had that logs all calls to do_cache_op.
I see the syscall return value -EINVAL

@manojgupta: It's odd that it gave EINVAL for that case when it gave EFAULT earlier (in comment 44).  Can you try the attached program?  It tries multiple cache_flush() calls to test different page permissions.

Deleted: cache_flush_prot.c 438 bytes

cache_flush_prot.c 438 bytes View Download

Same error with new program.

Btw, the reason for different error is below. Basically the failure location is different:

do_cache_op(unsigned long start, unsigned long end, int flags)
 {
       if (end < start || flags) {
                return -EINVAL; // Failing here with your code
       }
       if (!access_ok(VERIFY_READ, start, end - start)) {
                return -EFAULT; // Used to fail here in NaCl
       }

@manojgupta: Ah, sorry, there was a mistake in my test: __builtin___cache_flush() takes an end address rather than a size.  Here is a fixed version.  Can you try that?

Deleted: cache_flush_prot.c 471 bytes

cache_flush_prot.c 471 bytes View Download

It returns -EFAULT now.

Does it return EFAULT on all of the iterations?  That program does 8 cache_flush() syscalls; it would be useful to know which give errors and which don't.

Iterations 1/2/5/6 result in error. Remaining iterations 3/4/7/8 are fine.

1. [434359.451949] Cache operation addresses 3002617856 3002621952 0
Failed with result -14

2. [434359.452565] Cache operation addresses 3002613760 3002617856 0
Failed with result -14

3. [434359.453227] Cache operation addresses 3002609664 3002613760 0
Fine

4, [434359.454315] Cache operation addresses 3002605568 3002609664 0
Fine

5. [434359.457563] Cache operation addresses 3002601472 3002605568 0
Failed with result -14

6. [434359.458442] Cache operation addresses 3002597376 3002601472 0
Failed with result -14

7. [434359.459167] Cache operation addresses 3002593280 3002597376 0
Fine

8. [434359.459879] Cache operation addresses 3002589184 3002593280 0
Fine.

Thanks!  OK, so it seems clear that Linux is rejecting cache_flush() on mappings without PROT_WRITE.

Can you tell me what Git revision of the Linux kernel you are using, and which Git repo I can find that in?  I'd like to check how this code path works now.

It used to be that flushing the read-only mappings was necessary.  (See comment 9 of https://bugs.chromium.org/p/nativeclient/issues/detail?id=2443 for background, if you're interested.)

I think this this the git repo for kernel https://chromium.googlesource.com/chromiumos/third_party/kernel . And I think the branch is chromeos-3.14 for the ARM32 kernels.

I believe, some arm chromebooks are on 3.10 branch as well.

Mark, can you also give me a quick summary of why the call is failing. This will help the upstream review that is currently stuck.

> And I think the branch is chromeos-3.14 for the ARM32 kernels.

we have arm devices spread across many versions.  you can see the full list here:
  http://dev.chromium.org/chromium-os/developer-information-for-chrome-os-devices
3.8, 3.10, 3.14, 3.18, 4.4 are all there.

but it would be good to focus on just one for the purposes of initial triaging.  let's say 3.14 since that's what Manoj appears to be using in his tests.

mseaborn@ Anything I can share with upstream llvm? 

Thanks

Is this really blocking 62 beta?

Do we need to pick in https://chromium-review.googlesource.com/#/c/chromiumos/overlays/chromiumos-overlay/+/669661/ ?

No, it is not blocking any release now since the NaCl workaround flag is still in place.

Thanks for confirmation.

I have landed the fix in compiler-rt upstream. Closing this for now.