942290 - Breakpad should handle libraries with DWARF on some functions and ARM EXIDX entries on others

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The Android/ARM libc has system call stubs with annotations for exception handling info, so they appear in the exception index but don't have DWARF frame info.  Because the unstripped libc binary has DWARF info for its C code, we use that and don't consider the ARM exception info at all (or, apparently, the normal symbol table — there's no FUNC entry in the Breakpad file for the asm stubs, but they have ELF symbols with appropriate sizes and types).

This is relevant to threads blocked in a syscall at the time of crash (the stack scan might work, or might give bad information), and to bug 941375 (handling sandbox violations like any other crash, at least in testing).

So, we should merge the available information on a per-symbol basis.  gdb presumably already does something similar, because it's not confused in this case.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug942290-breakpad-ehabi-merge-hg0.diff

This seems to work — it uses the exidx entry for a function if the existing StackFrameEntry objects (from DWARF) don't fully cover it.

Example: https://pastebin.mozilla.org/4113532

We're still missing the name (and source location) of the libc function involved, because breakpad doesn't fall back to the information in the ELF symbol table in the absence of a DW_TAG_subprogram, but that's typically going to be obvious from its call site.  The important thing is to avoid spurious frames as seen in https://pastebin.mozilla.org/4113618 (the same crash without this patch).

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This bug caused some confusion in analyzing bug 986250, and it could have been worse (the stack scan seems to have actually done the right thing there, which it doesn't always), so I think we might want it for 1.4.

Comment 3

[Preeti Raghunath(:Preeti)]

Jed,

Please request approval on the same.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Clearing needinfo for now, as there's not much point to requesting approval on an unreviewed patch.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8366117 [details] [diff] [review]
bug942290-breakpad-ehabi-merge-hg0.diff

Review of attachment 8366117 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good. Note that none of this code is currently upstream, which is a bit of a mess. If you could add your patch to toolkit/crashreporter/breakpad-patches/ that would help me not forget to get it upstreamed. I think I'm going to drop a bunch of our other local patches since the profiler isn't using them anymore, but if this is important to you I'll get the exidx parser and this patch upstream.

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_to_module.cc
@@ +161,5 @@
>    }
>    return ret;
>  }
>  
> +bool ARMExToModule::AddStackFrame(uintptr_t addr, size_t size) {

It feels like you might want to put this in a different function instead, like "bool HaveStackInfoCovering(...)". The way this function is working now is a little weird.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug942290-breakpad-ehabi-merge-hg1.diff

Thanks for the review.  Updated, with stack-frame-entry coverage test factored out as suggested; carrying over r+.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(The patch is also updated to contain a copy of itself.)

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://tbpl.mozilla.org/?tree=Try&rev=686540ed8cdf

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1e2c1524892f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e2c1524892f

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #8)
> https://tbpl.mozilla.org/?tree=Try&rev=686540ed8cdf

For the record (and because I'm not sure how well the automated tests cover this), I also tested locally by backing out the workaround for bug 906996 and provoking it, with and without the final patch.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Self-needinfo to ask for 1.4 uplift once it seems stable.  I can also ask for aurora/beta, but it's more valuable on b2g because of sandboxing.  But I'll wait until Fennec/B2G nightlies happen and crash-stats can give me a before/after to point at.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #12)
> Self-needinfo to ask for 1.4 uplift once it seems stable.  I can also ask
> for aurora/beta, but it's more valuable on b2g because of sandboxing.  But
> I'll wait until Fennec/B2G nightlies happen and crash-stats can give me a
> before/after to point at.

This might be difficult, because any B2G build that doesn't have this patch and does have the same libc.so (up to the first 4096 bytes of .text, because we don't have real build IDs, apparently?) as one that does will overwrite the symbols if it was more recently uploaded when the crash is submitted.  Empirically, I haven't found any crashes on crash-stats where it actually worked.

See also: https://crash-stats.mozilla.com/search/?product=B2G&build_id=%3E20140521000000&platform=Android&version=32.0a1

Also, Fennec need not apply, because we don't (I think?) have any breakpad symbols for libc there.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'd like to get this uplifted to 1.4 if possible.  The patch applies as-is, and I've tested it locally to verify that it works.  Also, I've started a try run: https://tbpl.mozilla.org/?tree=Try&rev=c306c9d90da5

This patch doesn't affect the code that's actually run on devices, only the tool that's used during the build to create the files used to analyze crashes.  Thus the risk of uplift is relatively small.

For what we gain from this: see comment #0; we'll get more reliable information about sandbox violations, and possibly about other crashes if there's something important about a thread that was blocked during the crash.

It's been on m-c for 10 days at the time of this writing, and no problems have been ascribed to it.

My understanding is that this only really relevant on b2g, because that's the only platform where we use our dump_syms to gather information from system libraries (and it's the assembly routines for syscalls in libc.so that are at issue here).  Also, this patch only affects ARM platforms in any case.  Therefore, I haven't also requested uplift to aurora or beta.

Comment 15

[Preeti Raghunath(:Preeti)]

Triage will resolve via email and post results here

Comment 16

[(not currently active) Ted Mielczarek]

This should be totally safe to branch-land. As Jed said, this doesn't affect shipping code, just the tools we use to dump symbols.

Comment 17

[Preeti Raghunath(:Preeti)]

Can we wait and take this in 2.0 instead? I am concerned about having too much already in 1.4.

Comment 18

[(not currently active) Ted Mielczarek]

That question is for Jed.

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The response to comment #17 already exists as comment #16.  But if Release Management doesn't want the patch, I'm not going to fight over it.