863475 - integrate ARM EXIDX unwind parsing into Breakpad

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

I've had a patch lying around for a while where I integrated some code from libunwind to parse the ARM unwind tables (in .ARM.exidx and .ARM.extbl). I was using it for dumping symbols from Android system libraries to get better stack traces, but we could use this to profile nightly builds on Android without having to ship a mess of CFI. The unwind tables are pretty compact.

My original patch was here:
https://breakpad.appspot.com/480002/

The latest updated version is here:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/breakpad-mq/raw-file/be86fa11b334/arm-unwind

It should apply on top of the latest Breakpad code from SVN, but it's not rebased on top of our local Breakpad patches that haven't landed upstream.

Comment 1

[Julian Seward [:jseward]]

Attachment: Rebased, so it applies to m-c

WIP patch -- compiles but is untested.  Updated to work with our
local Breakpad representational changes (UniqueString and Module::Expr).
All the tricky impedance-matching changes are in arm_ex_to_module.cc.
There is some uglyness to do with getting the postfix expression
string back out of a Module::Expr for the cases ARM_EXIDX_CMD_REG_POP
and ARM_EXIDX_CMD_REG_TO_SP so it can be reassigned to |vsp|.  I
need to figure out how to make that less ugly.

Comment 2

[Julian Seward [:jseward]]

Some comments about EXIDX unwinding in SPS.  Maybe a bad place to
record them, but it's worth recording them somewhere.

The default nightly android config,
mobile/android/config/mozconfigs/android/nightly, sort-of works, but
doesn't give great resuts -- quite a lot of frames are not unwound
from.  This is due to a combination of reasons:

(1) nsprpub is not built with -funwind-tables, hence contains no EXIDX and
    so unwinding simply stops when it hits any NSPR frame.

(2) The scheme in UnwinderThread2.cpp:thread_register_for_profiling(), to
    record a max-safe-stack-address for the calling thread, is a problem
    and needs redesign.  Problem is that the thread calls this function
    with many frames on the stack, records an excessively low max-safe-addr,
    and subsequent samples of that thread with fewer frames on the stack
    often have SP values greater than the recorded address.  Hence no
    stack is copied and unwinding fails.  This also affects CFI and FP
    based unwinding.

(3) Relatedly to (2), the max size of the copied stack chunk, 32KB, is
    too small, and causes frame loss in deep nesting.  Increasing it
    to 48KB helps.

(4) The arcane logic in google-breakpad/src/common/linux/dump_symbols.cc:
    LoadSymbols() works against us.  For system libraries (eg libc.so)
    which have EXIDX on the main object but also have a debugobject in
    with CFI in .../symbols/system/lib/libc.so, the presence of EXIDX 
    inhibits the reading of the CFI.  It would be generally better if
    breakpad would aggregate information from both main and debug objects,
    rather than choose one or the other using hard-to-follow logic.

(5) It may be that gcc produces EXIDX which is less accurate than CFI,
    because EXIDX is only for unwinding from exception points, whereas
    CFI (at least with -fasynchronous-unwind-tables) is for unwinding 
    at any point.  This is evident when one looks at function preambles,
    eg:

        <_ZN7XREMain11XRE_mainRunEv>:
           stmdb   sp!, {r4, r5, r6, r7, r8, r9, sl, fp, lr}  // save regs
           sub     sp, #268        ; 0x10c                    // move SP down
           ...
           add     r7, sp, #8                        // R7 = SP_after_stmdb - 260

   The EXIDX for the function as a whole says:

           vsp = r7
           vsp = vsp + 4
           vsp = vsp + 256
           pop {r4, r5, r6, r7, r8, r9, r10, r11r14}

   that is, "take r7, add 260, then read regs from that address."  That's
   correct in the body of the function.  But it's not correct for the machine
   state after the first 2 preamble insns shown above.  Hence if our sampling
   thread's innermost frame falls in such a preamble point, EXIDX unwinding
   will at best be wrong and most likely fail completely.

Fixing (1) and (3) and kludging around (2) gives at least somewhat
useful results on nightly, with only a modest APK size increase from
28.8MB to 30.7MB.  I haven't investigated (4) or (5) yet.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Julian Seward from comment #2)
> (5) It may be that gcc produces EXIDX which is less accurate than CFI,
>     because EXIDX is only for unwinding from exception points, whereas
>     CFI (at least with -fasynchronous-unwind-tables) is for unwinding 
>     at any point.

Yeah, looking at ARM unwind directives, they don't even provide for the possibility of asynchronous unwind tables. :(

Comment 4

[Julian Seward [:jseward]]

Attachment: Patch, v2

Proposed patch, for review.  This does appear to work quite well.
There are no logic changes relative to Ted's original version, only
glue/impedance-matching stuff.

The only tricky bits are in arm_ex_to_module.cc, cases
ARM_EXIDX_CMD_REG_POP and ARM_EXIDX_CMD_REG_TO_SP, where we pull the
postfix string out of |vsp_expr| and assign it to |vsp|.  What's ugly
is that there is no compile-time way (afaics) to ensure that vsp_expr
is indeed a postfix expression, although it always should be.  Hence I
added a test that replaces vsp with a completely bogus string which
presumably the postfix evaluator will shout about, if it ever gets
evaluated.  Suggestions for a less ugly kludge welcomed.

Also I added a predicate Module::Expr::isExprPostfix and renamed
Module::Expr::invalid to Module::Expr::isExprInvalid for consistency.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 749340 [details] [diff] [review]
Patch, v2

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

Any particular reason not to make this ARM only?

I'm going to stop at ExtabEntryExtract for now, because this review is going to take much longer than I can dedicate to it right now, and there are already important things to fix here. I think commenting the code would make similar errors more apparent if there are others.

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.cc
@@ +26,5 @@
> +
> +void* Prel31ToAddr(const void* addr) {
> +  int32_t offset = *reinterpret_cast<const int32_t*>(addr);
> +  offset = ((long)offset << 1) >> 1;
> +  return ((char *)addr) + offset;

ted will know better, but i think breakpad only uses C++ type casts.

@@ +31,5 @@
> +}
> +
> +// Extract information from extab entry into buf. If impossible return -1.
> +int ExceptionTableInfo::ExtabEntryExtract(const struct exidx_entry* entry,
> +                                          uint8_t* buf) {

You'd need to give a buffer length (or use a MemoryRange) and do boundary checks.

@@ +34,5 @@
> +int ExceptionTableInfo::ExtabEntryExtract(const struct exidx_entry* entry,
> +                                          uint8_t* buf) {
> +  int nbuf = 0;
> +  uint32_t data = entry->data;
> +  if (data == ARM_EXIDX_CANT_UNWIND) nbuf = -1;

nbuf = -1; on its own line. Isn't EXIDX_CANT_UNWIND a exidx thing only? not extab?

@@ +39,5 @@
> +  else if (data & ARM_EXIDX_COMPACT) {
> +    buf[nbuf++] = data >> 16;
> +    buf[nbuf++] = data >> 8;
> +    buf[nbuf++] = data;
> +  } else {

This whole thing would really benefit from comments about the format and such.

And it looks like this ARM_EXIDX_COMPACT case above is wrong: it ignores the personality routine index, and actually prevents the code below, that does handle it, to actually be executed.

@@ +48,5 @@
> +        int pers = (data >> 24) & 0x0f;
> +        if (pers == 1 || pers == 2) {
> +          n_table_words = (data >> 16) & 0xff;
> +          extbl_data += 1;
> +        } else buf[nbuf++] = data >> 16;

} else {
...
}

@@ +50,5 @@
> +          n_table_words = (data >> 16) & 0xff;
> +          extbl_data += 1;
> +        } else buf[nbuf++] = data >> 16;
> +        buf[nbuf++] = data >> 8;
> +        buf[nbuf++] = data;

This is hard to read without comments about the format and why you get 2 bytes in one case and 3 in the other (and that it's really what you want to do and that it's correct)

@@ +57,5 @@
> +           buf[nbuf++] = extbl_data[1] >> 16;
> +           buf[nbuf++] = extbl_data[1] >> 8;
> +           buf[nbuf++] = extbl_data[1];
> +           extbl_data += 2;
> +        }

indentation

::: toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc
@@ +350,5 @@
>    return true;
>  }
>  
> +template<typename ElfClass>
> +bool LoadARMexidx(const typename ElfClass::Ehdr* elf_header,

Note this might conflict with my changes for bug 689178.

Comment 6

[Julian Seward [:jseward]]

Attachment: Patch, v3

Comment 7

[Julian Seward [:jseward]]

> Created attachment 751244 [details] [diff] [review] [diff] [review]
> Patch, v3

With comments in arm_ex_reader.cc, ExtabEntryExtract.  I completely
rewrote ExtabEntryExtract and checked it against the ARM specs.

Also with copyright notices for the new files; they were missing
before.

(In reply to Mike Hommey [:glandium] from comment #5)

> Any particular reason not to make this ARM only?

It's really a reader for a particular format (like dwarf, elf, stabs)
and the format readers are all in
toolkit/crashreporter/google-breakpad/src/common, so it seems logical
to put it in that directory too.

> > +void* Prel31ToAddr(const void* addr) {
> > +  int32_t offset = *reinterpret_cast<const int32_t*>(addr);
> > +  offset = ((long)offset << 1) >> 1;
> > +  return ((char *)addr) + offset;
> 
> ted will know better, but i think breakpad only uses C++ type casts.

fixed.

> > +// Extract information from extab entry into buf. If impossible return -1.
> > +int ExceptionTableInfo::ExtabEntryExtract(const struct exidx_entry* entry,
> > +                                          uint8_t* buf) {
> 
> You'd need to give a buffer length (or use a MemoryRange) and do boundary
> checks.

fixed.

> nbuf = -1; on its own line.

ExtabEntryExtract now returns an enum; that -1 thing has disappeared.

> Isn't EXIDX_CANT_UNWIND a exidx thing only? not extab?

I'm not sure what you mean by this.
The name EXIDX_CANT_UNWIND is mentioned in ARM IHI 0038 A, section 5.

> This whole thing would really benefit from comments about the format and
> such.

Fixed.

> And it looks like this ARM_EXIDX_COMPACT case above is wrong: it ignores the
> personality routine index, and actually prevents the code below, that does
> handle it, to actually be executed.

After looking at this for some time, I think it was actually correct,
but it assumes that invalid inputs don't occur, and doesn't check for
them.  The new version of ExtabEntryExtract does check more carefully
(and is commented).

> This is hard to read without comments about the format and why you get 2
> bytes in one case and 3 in the other

Fixed.

> indentation

Fixed.

> Note this might conflict with my changes for bug 689178.

I'm not clear what the problem might be.  If the use of elfhack is
going to change status (from disabled to enabled) then I imagine that
any problems we might have will affect CFI reading too, not just
EXIDX.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #7)
> > Created attachment 751244 [details] [diff] [review]
> > Patch, v3
> 
> With comments in arm_ex_reader.cc, ExtabEntryExtract.  I completely
> rewrote ExtabEntryExtract and checked it against the ARM specs.
> 
> Also with copyright notices for the new files; they were missing
> before.
> 
> (In reply to Mike Hommey [:glandium] from comment #5)
> 
> > Any particular reason not to make this ARM only?
> 
> It's really a reader for a particular format (like dwarf, elf, stabs)
> and the format readers are all in
> toolkit/crashreporter/google-breakpad/src/common, so it seems logical
> to put it in that directory too.

The question was not about the location, but about the lack of ifdefs.

> > Note this might conflict with my changes for bug 689178.
> 
> I'm not clear what the problem might be.  If the use of elfhack is
> going to change status (from disabled to enabled) then I imagine that
> any problems we might have will affect CFI reading too, not just
> EXIDX.

Bug 689178 changes a lot of breakpad code. See the breakpad patches linked there.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 751244 [details] [diff] [review]
Patch, v3

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

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.cc
@@ +116,5 @@
> +
> +static void* Prel31ToAddr(const void* addr) {
> +  void* addrNonConst = const_cast<void*>(addr);
> +  int32_t offset = *static_cast<int32_t*>(addrNonConst);
> +  offset = (static_cast<long int>(offset) << 1) >> 1;

<< has an undefined behavior for negative values, which you can have here. And >> may re-put a 1 at bit 31 or not, depending on the implementation. IOW, this is very fragile.

@@ +127,5 @@
> +
> +ExceptionTableInfo::ExExtractResult
> +ExceptionTableInfo::ExtabEntryExtract(const struct exidx_entry* entry,
> +                                      uint8_t* buf, unsigned int buf_size,
> +                                      /*OUT*/unsigned int* buf_used)

Now I understand what got me confused the first time: this is treating exidx entries, and the case where the extab data is inlined in the exidx entry duplicates the logic of reading extab data. It would make sense to create a separate function dealing with extab data, which would be used for both cases.

@@ +137,5 @@
> +  // at the bottom of the stack.
> +  if (data == ARM_EXIDX_CANT_UNWIND)
> +    return ExCantUnwind;
> +
> +# define ADD_TO_BUF(_byte) \

This and READ_OP below are not very C++y.

@@ +150,5 @@
> +    // extra data words.  Hence the allowable personalities here are:
> +    //   personality 0
> +    //   personality 1, with zero extra words
> +    //   personality 2, with zero extra words
> +    uint32_t pers = (data >> 24) & 0x0F;

I wonder if it wouldn't be better and clearer to just read data byte by byte.

@@ +159,5 @@
> +      ADD_TO_BUF(data >> 16);
> +      ADD_TO_BUF(data >> 8);
> +      ADD_TO_BUF(data);
> +    }
> +    else if ((pers == 1 || pers == 2) && xtra == 0) {

pers <= 2 would match too.

@@ +174,5 @@
> +  else {
> +    // The index table entry is a pointer to the handler entry.
> +    uint32_t* extbl_data
> +      = reinterpret_cast<uint32_t*>(Prel31ToAddr(&entry->data));
> +    data = extbl_data[0];

You're trusting that this actually points in the extab section. You should probably check this is actually true.

@@ +235,5 @@
> +  assert(buf != NULL);
> +  assert(data_size > 0);
> +
> +  while (buf < end) {
> +    uint8_t op = READ_OP();

Short of inline comments about the format, please add a comment pointing to table 4 in section 9.3 of the ARM document.

@@ +262,5 @@
> +      }
> +    }
> +    else if ((op & 0xf0) == 0xa0) {
> +      unsigned end = (op & 0x07);
> +      edata.data = (1 << (end + 1)) - 1;

A comment about the fact that this is effectively setting "end" number of bits from 4 upwards might be helpful.

@@ +286,5 @@
> +      do {
> +        byte = READ_OP();
> +        offset |= (byte & 0x7f) << shift;
> +        shift += 7;
> +      } while (byte & 0x80);

There should be an overflow check here.

@@ +294,5 @@
> +    else if (op == 0xb3 || op == 0xc8 || op == 0xc9) {
> +      edata.cmd = ARM_EXIDX_CMD_VFP_POP;
> +      edata.data = READ_OP();
> +      if (op == 0xc8) edata.data |= ARM_EXIDX_VFP_SHIFT_16;
> +      if (op != 0xb3) edata.data |= ARM_EXIDX_VFP_DOUBLE;

ARM_EXIDX_VFP_DOUBLE is kind of ambiguous, since in all cases, this is about popping double-precision registers.

@@ +302,5 @@
> +      edata.data = 0x80 | (op & 0x07);
> +      if ((op & 0xf8) == 0xd0) edata.data |= ARM_EXIDX_VFP_DOUBLE;
> +    }
> +    else if (op >= 0xc0 && op <= 0xc5) {
> +      edata.cmd = ARM_EXIDX_CMD_WREG_POP;

Are these actually used?

@@ +342,5 @@
> +    if (entry < end - 1)
> +      next_addr = (reinterpret_cast<char*>(Prel31ToAddr(&((entry + 1)->addr)))
> +                   - mapping_addr_ + loading_addr_) & 0x7fffffff;
> +    else {
> +      //XXX: how to calculate this? just "size of text section" - addr?

the exidx section has sh_link set to the section index of the corresponding text section. That can give you the address of the end of the text section.

@@ +375,5 @@
> +    if (ret < 0) {
> +      handler_->DeleteStackFrame();
> +      continue;
> +    }
> +    handler_->SubmitStackFrame();

It would probably feel cleaner if you'd have a CreateStackFrame function that returns a stack frame entry, which you'd pass to ExtabEntryDecode. Then it becomes obvious you don't need to create the stack frame before ExtabEntryExtract and free it if that fails.

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.h
@@ +92,5 @@
> +    ExSuccess,       // success
> +    ExBufOverflow,   // output buffer is too small
> +    ExCantUnwind,    // this function is marked CANT_UNWIND
> +    ExCantRepresent, // entry valid, but we can't represent it
> +    ExInvalid        // entry is invalid 

trailing whitespace

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_to_module.cc
@@ +60,5 @@
> +
> +#include <stdio.h>
> +#include <assert.h>
> +
> +// For big-picture comments on how the EXIDX reader works, 

trailing whitespace.

@@ +78,5 @@
> +namespace arm_ex_to_module {
> +
> +// Translate command from extab_data to command for Module.
> +int ARMExToModule::TranslateCmd(const struct extab_data *edata,
> +                                Module::StackFrameEntry *entry, string& vsp) {

I'll leave this function to Ted, as I don't know that CFI format you're constructing for breakpad to parse.

Comment 10

[Julian Seward [:jseward]]

Attachment: Patch, v4

Main changes:

* addresses all review comments

* range checking throughout (in reading .exidx and .extab sections,
  and in reading and writing of the intermediate buffer)

* a lot more comments

-------------------

> > > Any particular reason not to make this ARM only?
> > It's really a reader for a particular format (like dwarf, elf, stabs)
> > and the format readers are all in
> > toolkit/crashreporter/google-breakpad/src/common, so it seems logical
> > to put it in that directory too.
> The question was not about the location, but about the lack of ifdefs.

This was discussed on irc.  Putting it in ifdefs would make the standalone
unwinders not work (IIUC).

> > > Note this might conflict with my changes for bug 689178.
> Bug 689178 changes a lot of breakpad code. See the breakpad patches linked there.

This patch primarily adds 4 files.  The changes to existing files are
mostly dump_symbols.cc, LoadARMexidx(), and adds a fragment in LoadSymbols()
to call it.  These are very self contained.  Overall I don't think there
is much chance of serious conflicts.

> > +static void* Prel31ToAddr(const void* addr) {
> << has an undefined behavior for negative values, [...]

fixed.

> Now I understand what got me confused the first time: this is treating
> exidx entries, and the case where the extab data is inlined in the exidx 
> entry duplicates the logic of reading extab data. It would make sense to
> create a separate function dealing with extab data, which would be used 
> for both cases.

In the end I managed to cause control to flow to a single piece of code
that handles both cases (so, no duplication).  Actually calling a helper is
complex when taking into account bounds checking.

> This and READ_OP below are not very C++y.

It's hard to avoid the macros, but I moved to using MemoryRange at least.

> I wonder if it wouldn't be better and clearer to just read data byte by byte.

The ARM spec is clearly phrased in terms of 32 bit words, and I'd prefer to
stay with that, rather than read byte by byte and forcing maintainers to 
re-infer where the word boundaries are.

> > +    else if ((pers == 1 || pers == 2) && xtra == 0) {
> 
> pers <= 2 would match too.

It makes the clauses non-independent; the original version makes
them independent (of reordering).

> > +    uint32_t* extbl_data
> > +      = reinterpret_cast<uint32_t*>(Prel31ToAddr(&entry->data));
> > +    data = extbl_data[0];
> 
> You're trusting that this actually points in the extab section. 
> You should probably check this is actually true.

Fixed (everything is range-checked now)

> Short of inline comments about the format, please add a comment 
> pointing to table 4 in section 9.3 of the ARM document.

Fixed.  I added both in-line comments and a reference to the ARM doc.

> @@ +286,5 @@
> > +      do {
> > +        byte = READ_OP();
> > +        offset |= (byte & 0x7f) << shift;
> > +        shift += 7;
> > +      } while (byte & 0x80);
> 
> There should be an overflow check here.

Fixed by the addition of range checking for the input buffer.

> @@ +294,5 @@
> > +    else if (op == 0xb3 || op == 0xc8 || op == 0xc9) {
> > +      edata.cmd = ARM_EXIDX_CMD_VFP_POP;
> > +      edata.data = READ_OP();
> > +      if (op == 0xc8) edata.data |= ARM_EXIDX_VFP_SHIFT_16;
> > +      if (op != 0xb3) edata.data |= ARM_EXIDX_VFP_DOUBLE;
> 
> ARM_EXIDX_VFP_DOUBLE is kind of ambiguous, since in all cases, 
> this is about popping double-precision registers.

This is quite subtle, but in fact it is necessary.  The reason is
that there are two different double-precision pop cases, depending
on whether the registers were pushed with a FSTMxxD or a FSTMxxX
instruction.  In the latter case the SP needs to be moved an extra
4 bytes, and that is what the (non-)presence of the ARM_EXIDX_VFP_DOUBLE
flag signifies.

> @@ +302,5 @@
> > +      edata.data = 0x80 | (op & 0x07);
> > +      if ((op & 0xf8) == 0xd0) edata.data |= ARM_EXIDX_VFP_DOUBLE;
> > +    }
> > +    else if (op >= 0xc0 && op <= 0xc5) {
> > +      edata.cmd = ARM_EXIDX_CMD_WREG_POP;
> 
> Are these actually used?

They are not used.  This code is derived from Linaro code, and it appears
to be a pretty complete implementation of the EXIDX spec.  That includes
Intel Wireless MMX extensions, even though that is long since redundant
(I think).  I seriously doubt if we will ever come across any CPUs with
that feature.  I have no idea if the Linaro people managed to test it
with those.

> > +    else {
> > +      //XXX: how to calculate this? just "size of text section" - addr?
> 
> the exidx section has sh_link set to the section index of the corresponding
> text section. That can give you the address of the end of the text section.

Fixed.  This was quite fiddly though, so I put in a sanity check that
ignores the "size of text section" computation if it implies a stupid
size of the last symbol.

> @@ +375,5 @@
> > +    if (ret < 0) {
> > +      handler_->DeleteStackFrame();
> > +      continue;
> > +    }
> > +    handler_->SubmitStackFrame();
> 
> It would probably feel cleaner if you'd have a CreateStackFrame function
> that returns a stack frame entry, which you'd pass to ExtabEntryDecode.
> Then it becomes obvious you don't need to create the stack frame before
> ExtabEntryExtract and free it if that fails.

I looked into this, but it isn't so simple.  These calls don't just update
the stack_frame_ in handler_, they also update vsp_.  So to pull both out
and pass them around would create more plumbing.  I did however make the
AddStackFrame() call happen after ExtabEntryDecode, and added a bunch of
comments, so it's clearer and more self-contained.

> trailing whitespace

I think I managed to get all of them.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #10)
> > ARM_EXIDX_VFP_DOUBLE is kind of ambiguous, since in all cases, 
> > this is about popping double-precision registers.
> 
> This is quite subtle, but in fact it is necessary.  The reason is
> that there are two different double-precision pop cases (...)

I know. That's why I'm saying the macro name is ambiguous.

Comment 12

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #11)
> > This is quite subtle, but in fact it is necessary.  The reason is
> > that there are two different double-precision pop cases (...)
> 
> I know. That's why I'm saying the macro name is ambiguous.

Ah.  I misunderstood your comment, then.  Yes -- the name could
be clearer.  I can easily change it to something better.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 752933 [details] [diff] [review]
Patch, v4

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

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.cc
@@ +112,5 @@
> +using arm_ex_to_module::ARM_EXIDX_CMD_RESERVED;
> +using arm_ex_to_module::ARM_EXIDX_CMD_REFUSED;
> +using arm_ex_to_module::exidx_entry;
> +using arm_ex_to_module::ARM_EXIDX_VFP_SHIFT_16;
> +using arm_ex_to_module::ARM_EXIDX_VFP_DOUBLE;

Don't forget to change this name :)

@@ +116,5 @@
> +using arm_ex_to_module::ARM_EXIDX_VFP_DOUBLE;
> +using google_breakpad::MemoryRange;
> +
> +
> +static void* Prel31ToAddr(const void* addr) 

trailing whitespace
You should probably add this to your .hgrc:
[extensions]
color =
[color]
diff.trailingwhitespace = bold red_background

@@ +125,5 @@
> +  uint64_t offset64 = offset32;
> +  if (offset64 & (1ULL << 30))
> +    offset64 |= 0xFFFFFFFF80000000ULL;
> +  else
> +    offset64 &= 0x000000007FFFFFFFULL;

Why not simply do offset32 &= 0x7fffffff ?

@@ +141,5 @@
> +                                      /*OUT*/size_t* buf_used)
> +{
> +  MemoryRange mr_exidx(exidx_, exidx_size_);
> +  MemoryRange mr_extab(extab_, extab_size_);
> +  MemoryRange mr_out(buf, buf_size);

You could pass a MemoryRange instead of creating one from two arguments.

@@ +150,5 @@
> +  do { if (!mr_out.Covers(*buf_used, 1)) return ExOutBufOverflow; \
> +       buf[(*buf_used)++] = (_byte); } while (0)
> +
> +# define GET_EX_U32(_lval, _addr, _sec_mr, _sec_start) \
> +  do { if (!(_sec_mr).Covers(reinterpret_cast<const char*>(_addr) \

instead of this, you could also add an alternative MemoryRange::Covers member that takes a pointer instead of an offset.

@@ +184,5 @@
> +    //   personality 1, with zero extra words
> +    //   personality 2, with zero extra words
> +    extbl_data = NULL;
> +    pers  = (data >> 24) & 0x0F;
> +    extra = (data >> 16) & 0xFF;

You can move these two lines outside the if, since you're doing the same in both branches.

@@ +221,5 @@
> +    PUT_BUF_U8(data);
> +  }
> +  else if ((pers == 1 || pers == 2) && extra <= extra_allowed) {
> +    // "Lu16" or "Lu32" respectively -- 2 unwinding insn bytes,
> +    // and up to 255 extra words.

up to extra_allowed extra words.

@@ +265,5 @@
> +{
> +  if (buf == NULL || buf_size == 0)
> +    return -1;
> +
> +  MemoryRange mr_in(buf, buf_size);

Same as above, you could pass the MemoryRange directly.

@@ +293,5 @@
> +    }
> +    else if ((op & 0xf0) == 0x80) {
> +      uint8_t op2;
> +      GET_BUF_U8(op2);
> +      if (op == 0x80 && op2 == 0x00)

{

@@ +295,5 @@
> +      uint8_t op2;
> +      GET_BUF_U8(op2);
> +      if (op == 0x80 && op2 == 0x00)
> +        // Refuse to unwind
> +        edata.cmd = ARM_EXIDX_CMD_REFUSED;

}

@@ +331,5 @@
> +    }
> +    else if (op == 0xb1) {
> +      uint8_t op2;
> +      GET_BUF_U8(op2);
> +      if (op2 == 0 || (op2 & 0xf0))

{

@@ +333,5 @@
> +      uint8_t op2;
> +      GET_BUF_U8(op2);
> +      if (op2 == 0 || (op2 & 0xf0))
> +        // Spare
> +        edata.cmd = ARM_EXIDX_CMD_RESERVED;

}

@@ +381,5 @@
> +    }
> +    else if (op == 0xc7) {
> +      uint8_t op2;
> +      GET_BUF_U8(op2);
> +      if (op2 == 0 || (op2 & 0xf0))

{

@@ +383,5 @@
> +      uint8_t op2;
> +      GET_BUF_U8(op2);
> +      if (op2 == 0 || (op2 & 0xf0))
> +        // Spare
> +        edata.cmd = ARM_EXIDX_CMD_RESERVED;

}

@@ +426,5 @@
> +    else {
> +      // This is the last EXIDX entry in the sequence, so we don't
> +      // have an address for the start of the next function, to limit
> +      // this one.  Instead use the address of the last byte of the
> +      // text section of associated with this .exidx section, that we

"text section of associated"?

@@ +447,5 @@
> +          plausible = true;
> +        }
> +      }
> +      if (!plausible)
> +        BPLOG(INFO) << "ExceptionTableInfo: implausible EXIDX last entry size " 

trailing whitespace

@@ +456,5 @@
> +    // Extract the unwind info into |buf|.  This might fail for
> +    // various reasons.  It involves reading both the .exidx and
> +    // .extab sections.  All accesses to those sections are
> +    // bounds-checked.
> +    uint8_t buf[ARM_EXIDX_TABLE_LIMIT];

ExtabEntryExtract may extract up to 1020 bytes (255 * 4), but ARM_EXIDX_TABLE_LIMIT is 32.

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.h
@@ +88,5 @@
> + private:
> +  const char* exidx_;
> +  size_t exidx_size_;
> +  const char* extab_;
> +  size_t extab_size_;

You could keep those as MemoryRanges directly.

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_to_module.cc
@@ +78,5 @@
> +namespace arm_ex_to_module {
> +
> +// Translate command from extab_data to command for Module.
> +int ARMExToModule::TranslateCmd(const struct extab_data *edata,
> +                                Module::StackFrameEntry *entry, string& vsp) {

I'm still leaving this one to Ted.

::: toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc
@@ +688,5 @@
> +  // ARM has special unwind tables that can be used.
> +  const Shdr* arm_exidx_section =
> +      FindElfSectionByName<ElfClass>(".ARM.exidx", SHT_ARM_EXIDX,
> +                                     sections, names, names_end,
> +                                     elf_header->e_shnum);

Note that you can get the exidx location from program headers (but not extab ; ld.so relies on exidx being right).

Comment 14

[Julian Seward [:jseward]]

Comment on attachment 752933 [details] [diff] [review]
Patch, v4

Requesting review of ARMExToModule::TranslateCmd as per last few 
lines of comment 9.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 752933 [details] [diff] [review]
Patch, v4

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

Lots of nits and a few things that could be followup work. Sorry for the delay! Once you get your final patch up here I'll put it up for review upstream, mostly because I want someone to sign off on importing the libunwind code. I think the actual code review here should be sufficient for upstream landing (although this depends on all those other patches that haven't landed yet).

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_to_module.cc
@@ +78,5 @@
> +namespace arm_ex_to_module {
> +
> +// Translate command from extab_data to command for Module.
> +int ARMExToModule::TranslateCmd(const struct extab_data *edata,
> +                                Module::StackFrameEntry *entry, string& vsp) {

nit: consistently snuggle * and & to the type name.

@@ +80,5 @@
> +// Translate command from extab_data to command for Module.
> +int ARMExToModule::TranslateCmd(const struct extab_data *edata,
> +                                Module::StackFrameEntry *entry, string& vsp) {
> +  int ret = 0;
> +  unsigned i;

This is a port from the original C, I think. You should just move the variable definition inside each of the for loop initializers.

@@ +88,5 @@
> +      if (entry->initial_rules.find(ustr__pc()) == entry->initial_rules.end()) {
> +        if (entry->initial_rules.find(ustr__lr()) == entry->initial_rules.end())
> +          entry->initial_rules[ustr__pc()] = Module::Expr("lr");
> +        else
> +          entry->initial_rules[ustr__pc()] = entry->initial_rules[ustr__lr()];

nit: brace all conditionals

@@ +103,5 @@
> +      {
> +        char c[16];
> +        sprintf(c, " %d +", edata->data);
> +        vsp += c;
> +      }

These were always my least favorite part of the translator. We should probably fix this to store vsp as a Module::Expr, since 99% of the time vsp is just sp + an offset. (We can do that as a followup though.) The CFI rules this generates are awful, like ".ra: sp 4 + 4 + 4 + ^". If we dealt with it as an Expr it ought to be a lot more efficient.

@@ +120,5 @@
> +        // else here), so add something entirely bogus if it isn't, in
> +        // the hope that the postfix evaluator will shout loudly.
> +        assert(vsp_expr.isExprPostfix());
> +        vsp = vsp_expr.isExprPostfix() ? vsp_expr.getExprPostfix()
> +                                       : "ARM_EXIDX_CMD_REG_POP:INVALID";

I think you should just return -1 if this isn't a postfix expression.

@@ +130,5 @@
> +      const UniqueString* regname_us = ToUniqueString(regname);
> +      if (entry->initial_rules.find(regname_us) == entry->initial_rules.end())
> +        entry->initial_rules[ustr__sp()] = Module::Expr(regname);
> +      else
> +        entry->initial_rules[ustr__sp()] = entry->initial_rules[regname_us];

nit: brace all conditionals

@@ +134,5 @@
> +        entry->initial_rules[ustr__sp()] = entry->initial_rules[regname_us];
> +      Module::Expr& vsp_expr = entry->initial_rules[ustr__sp()];
> +      assert(vsp_expr.isExprPostfix());
> +      vsp = vsp_expr.isExprPostfix() ? vsp_expr.getExprPostfix()
> +                                     : "ARM_EXIDX_CMD_REG_TO_SP:INVALID";

Same here, just return -1 to fail out.

@@ +138,5 @@
> +                                     : "ARM_EXIDX_CMD_REG_TO_SP:INVALID";
> +      break;
> +    }
> +    case ARM_EXIDX_CMD_VFP_POP:
> +      /* Skip VFP registers, but be sure to adjust stack */

I might rephrase this slightly: "Don't recover VFP registers, but be sure to adjust the stack pointer."

@@ +140,5 @@
> +    }
> +    case ARM_EXIDX_CMD_VFP_POP:
> +      /* Skip VFP registers, but be sure to adjust stack */
> +      for (i = ARM_EXBUF_START (edata->data);
> +           i <= ARM_EXBUF_END (edata->data); i++)

nit: get rid of the spaces before the open parens.

@@ +141,5 @@
> +    case ARM_EXIDX_CMD_VFP_POP:
> +      /* Skip VFP registers, but be sure to adjust stack */
> +      for (i = ARM_EXBUF_START (edata->data);
> +           i <= ARM_EXBUF_END (edata->data); i++)
> +        vsp += " 8 +";

nit: brace loop body.

@@ +142,5 @@
> +      /* Skip VFP registers, but be sure to adjust stack */
> +      for (i = ARM_EXBUF_START (edata->data);
> +           i <= ARM_EXBUF_END (edata->data); i++)
> +        vsp += " 8 +";
> +      if (!(edata->data & ARM_EXIDX_VFP_DOUBLE)) vsp += " 4 +";

nit: brace the conditional, put the body on its own line.

@@ +147,5 @@
> +      break;
> +    case ARM_EXIDX_CMD_WREG_POP:
> +      for (i = ARM_EXBUF_START (edata->data);
> +           i <= ARM_EXBUF_END (edata->data); i++)
> +        vsp += " 8 +";

nits: ditch spaces before open parens, brace loop body.

@@ +150,5 @@
> +           i <= ARM_EXBUF_END (edata->data); i++)
> +        vsp += " 8 +";
> +      break;
> +    case ARM_EXIDX_CMD_WCGR_POP:
> +      for (i = 0; i < 4; i++)

This 4 probably wants to be a named constant. (I forget what it actually stands for.)

@@ +151,5 @@
> +        vsp += " 8 +";
> +      break;
> +    case ARM_EXIDX_CMD_WCGR_POP:
> +      for (i = 0; i < 4; i++)
> +        if (edata->data & (1 << i)) vsp += " 4 +";

nits: braces, body on new line.

::: toolkit/crashreporter/google-breakpad/src/common/arm_ex_to_module.h
@@ +101,5 @@
> + "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
> + "r8", "r9", "r10", "r11", "r12", "sp", "lr", "pc",
> + "f0", "f1", "f2", "f3", "f4", "f5", "f6", "f7",
> + "fps", "cpsr"
> +};

It would be nice to be able to merge this with the table that the ARM stackwalker uses:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/processor/stackwalker_arm.cc#84

but it's not a deal-breaker.

Comment 16

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8277fd16758c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8277fd16758c