New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide useful defaults for Android logcat output #21812

Merged
merged 4 commits into from Sep 26, 2018

Conversation

Projects
None yet
6 participants
@jdm
Member

jdm commented Sep 25, 2018

These changes integrate the stdout/stderr redirection from https://github.com/tomaka/android-rs-glue/blob/0e80cfbcee362f207389dc2660525c3d74987f70/cargo-apk/injected-glue/lib.rs#L240-L303, and set up the default RUST_LOG filters so that useful JS errors and GL errors appear in the logcat output. I have verified that thread panics appear in output as well (without backtraces, sadly) by visiting https://acid3.acidtests.org.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21637 and fix #21783
  • These changes do not require tests because no tests for logcat.

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Sep 25, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@jdm jdm force-pushed the jdm:useful-output branch from ae8e9e5 to b1e2bb3 Sep 25, 2018

@jdm

This comment has been minimized.

Member

jdm commented Sep 26, 2018

@highfive highfive assigned paulrouget and unassigned nox Sep 26, 2018

@SimonSapin

This comment has been minimized.

Member

SimonSapin commented Sep 26, 2018

Sorry I lagged a bit, I’m about to submit a review with some code changes.

@SimonSapin

For backtraces, maybe std::env::set_var("RUST_BACKTRACE", "1") would help?

#[allow(non_camel_case_types)]
type pthread_mutexattr_t = c_long;
#[allow(non_camel_case_types)]
type pthread_attr_t = c_void; // FIXME: wrong

This comment has been minimized.

@SimonSapin

SimonSapin Sep 26, 2018

Member

These types exist in the libc crate. android-rs-glue chooses not to depend on libc for some reason, but we already do. Let’s not redefine them?

fn read(fd: c_int, buf: *mut c_void, count: usize) -> isize;
fn pthread_create(_: *mut pthread_t, _: *const pthread_attr_t,
_: extern fn(*mut c_void) -> *mut c_void, _: *mut c_void) -> c_int;
fn pthread_detach(thread: pthread_t) -> c_int;

This comment has been minimized.

@SimonSapin

SimonSapin Sep 26, 2018

Member

Same for these functions.

}
let mut thread = mem::uninitialized();
let result = pthread_create(&mut thread, ptr::null(), logging_thread,

This comment has been minimized.

@SimonSapin

SimonSapin Sep 26, 2018

Member

I was wondering why this uses pthread rather than std::thread. tomaka/android-rs-glue#75 (comment) says:

At that time, trying to use Rust threads would lead to a crash because the Rust standard library needs to be initialized before being usable.

This initialization is generally automatically injected in _start by the compiler when you compile a regular binary. But here we are bypassing this mechanism.

@jdm, @paulrouget, do you know if this is also relevant to us?

This comment has been minimized.

@jdm

jdm Sep 26, 2018

Member

I tested using the stdlib threads and it works fine on my Pixel 2.

loop {
let result = read(descriptor, buf.as_mut_ptr().offset(cursor as isize) as *mut _,
buf.capacity() - 1 - cursor);

This comment has been minimized.

@SimonSapin

SimonSapin Sep 26, 2018

Member

I’ve convinced myself that this offset doesn’t overflow the buffer and that the substractions don’t underflow beyond zero, but it’s rather subtle.

cursor is always set to one of:

  • Zero

  • buf.capacity() - 1, which is 511

  • buf.len() which is set through set_len to result + cursor, where result is number of bytes returned by a previous read call, which is at most equal to the third argument given to read, buf.capacity() - 1 - cursor

  • buf.len() - last_newline_pos, which doesn’t underflow because last_newline_pos is returned from buf.iter().rposition(…)

I’d be much more comfortable if this code used much less unsafe pointer manipulation and more checked slicing, something like SimonSapin@a7dab4e

cursor = buf.len();
}
if cursor == buf.capacity() - 1 {
__android_log_write(3, tag, buf.as_ptr());

This comment has been minimized.

@SimonSapin

SimonSapin Sep 26, 2018

Member

Unless I missed something I think this is missing a nul terminator. Something like *buf.get_unchecked_mut(cursor) = b'\0' as c_char; before this call should fix that.

This may be worth reporting upstream.

This comment has been minimized.

@jdm
@jdm

This comment has been minimized.

Member

jdm commented Sep 26, 2018

Setting RUST_BACKTRACE=1 tends to hit segfaults in libunwind when using NDK12. I'll give it a try as part of the NDK15 upgrade.

@jdm jdm force-pushed the jdm:useful-output branch from b1e2bb3 to a07f1b4 Sep 26, 2018

@jdm

This comment has been minimized.

Member

jdm commented Sep 26, 2018

I used your code changes unmodified except for fitting them in with the previous commit.

@jdm jdm force-pushed the jdm:useful-output branch from a07f1b4 to b11f071 Sep 26, 2018

@SimonSapin

This comment has been minimized.

Member

SimonSapin commented Sep 26, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 26, 2018

📌 Commit b11f071 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 26, 2018

⌛️ Testing commit b11f071 with merge 515afac...

bors-servo added a commit that referenced this pull request Sep 26, 2018

Auto merge of #21812 - jdm:useful-output, r=SimonSapin
Provide useful defaults for Android logcat output

These changes integrate the stdout/stderr redirection from https://github.com/tomaka/android-rs-glue/blob/0e80cfbcee362f207389dc2660525c3d74987f70/cargo-apk/injected-glue/lib.rs#L240-L303, and set up the default RUST_LOG filters so that useful JS errors and GL errors appear in the logcat output. I have verified that thread panics appear in output as well (without backtraces, sadly) by visiting https://acid3.acidtests.org.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21637 and fix #21783
- [x] These changes do not require tests because no tests for logcat.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21812)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 26, 2018

@bors-servo bors-servo merged commit b11f071 into servo:master Sep 26, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment