Page MenuHomePhabricator

Add support for ARM and ARM64 breakpad generated minidump files.
ClosedPublic

Authored by clayborg on Jul 24 2018, 12:24 PM.

Details

Summary

In this patch I add support for ARM and ARM64 break pad files. There are two flavors of ARM: Apple where FP is R7, and non Apple where FP is R11. Added minimal tests that load up ARM64 and the two flavors or ARM core files with a single thread and known register values in each register. Each register is checked for the exact value.

Diff Detail

Event Timeline

clayborg created this revision.Jul 24 2018, 12:24 PM
lemo added a subscriber: lemo.Jul 24 2018, 3:17 PM

Looks really good, a few comments inline.

This may not big a big deal, but the part about FP (and Apple vs. non-Apple) is confusing: the FP is a pretty weak convention, and in some ABIs is not actually "fixed" (ex. FP can be either R7 or R11, which in turn can be used as GPRs). If Apple sticks to a strict usage it makes sense to name it but then maybe we should just not name any FP for non-Apple?

source/Plugins/Process/minidump/MinidumpParser.cpp
147–148

instead of useing m_arch as a cache I'd explicitly initialize it in MinidumpParser::Initialize()

151–155

why is this needed when we have MinidumpOSPlatform::Linux ?

source/Plugins/Process/minidump/ProcessMinidump.cpp
178

shouldn't this be a check resulting in an error? why do we need to make this implicit adjustment here?

source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
193

constexpr?

196

use std::array for these kind of static arrays? (debug bounds checks, easy access to the static size, ...)

226

symbolic constants or use the array size?

232

lldb_assert ?

243

not a big deal, but this kind of accessors for constants can be constexpr themselves

249

failfast if out of bounds? who'd pass an invalid index and expect a meaninful result?
(btw, std::array would provide the debug checks if that's all that we want)

282

remove unused parameter name or [[maybe_unused]]

source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h
83

= nullptr;

84

0;

source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
38

default instead of {} ?

I will make update the patch with many of the suggested inline comments.

source/Plugins/Process/minidump/MinidumpParser.cpp
148

Can do

155

I have run into some minidump files that don't have linux set corrreclty as the OS, but they do have "linux" in the description. So this is to handle those cases. I have told the people that are generating these about the issue and they will fix it, but we have minidump files out in the wild that don't set linux as the OS correctly.

source/Plugins/Process/minidump/ProcessMinidump.cpp
178

By default the "host" platform is selected and it was incorrectly being used along with _any_ triple. So we need to adjust the platform if the host platform isn't compatible. The platform being set correctly helps with many things during a debug session like finding symbols and much more.

lemo added inline comments.Jul 24 2018, 4:06 PM
source/Plugins/Process/minidump/MinidumpParser.cpp
155
  1. any idea where this minidumps originate from?
  2. should we raise some kind of signal when we go down this path? print an warning or at least verbose logging?
source/Plugins/Process/minidump/ProcessMinidump.cpp
178

Nice catch/fix in that case!

Just curious: It still seems a bit surprising to me to have the target mutated by the process object - is that how it's normally done in other cases?

labath added inline comments.Jul 25 2018, 1:40 AM
source/Plugins/Process/minidump/ProcessMinidump.cpp
178

I agree that this looks out of place in the minidump plugin. I don't see any other process/core file plugin doing that, but there should be nothing special about minidump files as far as platform selection goes, right? (In fact I wouldn't be surprised if this is causing problems for us already, as some of our elf core tests are disabled on windows).

Maybe this could be moved at least to a binary-format-agnostic place, so that all core file plugins benefit from this?

source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
44–57

It looks like these don't define any of the sub-registers (W, H, S and D sets)?

Btw, for x86, the way we avoided the need to define all of these registers all over again is that we took the register descriptions from the minidump file, and rearranged them in memory to match what the existing register contexts (for elf core files) expect. Maybe you could do that here too? Since you're already storing a copy of the register data in the m_regs field, it shouldn't even impact the memory footprint (but it will make all of this goo go away).

lemo added inline comments.Jul 25 2018, 9:44 AM
source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
52

Pavel's comment reminded me: what about the S registers (32bit fp) and Q registers (128bit Neon)?

FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM
minidumps soon.

clayborg updated this revision to Diff 158321.Jul 31 2018, 10:46 AM
  • Fixed inline comments
  • Moved platform setting into Target::SetArchitecture(...) instead of doing this manually in the core file code.
  • Added ARM64 w0-w31, d0-d31, s0-s31 and h0-h31 registers
  • Added ARM s0-s31 and q0-q15 registers
  • Improved values in registers in arm and arm64 minidump files to ensure unique values in each register and updated tests to test for it
clayborg added inline comments.Jul 31 2018, 10:47 AM
source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
193

will do

196

Tried it but it introduces a global constructor. We try to avoid those.

source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
57

I would rather define a new context and avoid mutating one register context into another. I didn't really like the other register contexts for minidumps. I like to show the actual data that is available, not a translation of one set of data to another.

lemo added a comment.Jul 31 2018, 11:29 AM

Thanks Greg, looks good to me (a couple of inline comments left at your discretion)

source/Plugins/Process/minidump/ProcessMinidump.cpp
15

it this set for removal?

source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
196

We shouldn't have a dynamic initializer: that's strange, if that's the case we have a compiler bug on our hands. A quick experiment indicates that even with -O0 recent clang/llvm do the right thing: https://godbolt.org/g/NMUFLP

Is the problem only with arrays of RegisterInfo structs? If that's the case the cause is RegisterInfo itself and std::array should not make a difference (ie. we'd see the dynamic initializer even with plain C arrays)

clayborg updated this revision to Diff 158371.Jul 31 2018, 1:17 PM

Removed unnecessary Xcode project changes and removed #include that wasn't needed.

clayborg added inline comments.Jul 31 2018, 1:29 PM
source/Plugins/Process/minidump/ProcessMinidump.cpp
15

Ah yes!

source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
197

Not sure why. I was using the latest Xcode and got the warning. RegisterInfo is just a plain struct so it shouldn't cause any issues.

labath accepted this revision.Aug 1 2018, 2:02 AM

Looks great. I only noticed some typos when looking this over again. We can continue the register shuffling discussion offline.

include/lldb/Target/Target.h
929–939

mismatch in the parameter name in the comment and signature

source/Plugins/Process/minidump/MinidumpParser.cpp
495

should error be initialized here?

source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
57

Since, you're the one doing the work, I am fine leaving this up to you, but I'd like to understand what is it that you didn't like about the x86 register contexts.

In what way do they misrepresent the data available? As far as I can tell, the difference here is just in the internal representation of the data. It should not change the actual values of the registers displayed to the user (if it does, I'd like to fix it). In other words, I should be able to rewrite the implementation here to reshuffle the register order in the same way as x86, and the tests should still pass.

If this is true, then this is only a question of optimizing the internal implementation for some criteria. In this case, I would choose to optimize for code size/readability, and try to avoid defining 200 lines of constants, which largely duplicate what is already given in the other register contexts.

222

llvm style would put spaces between the operators here. Could you run clang-format over the diff?

This revision is now accepted and ready to land.Aug 1 2018, 2:02 AM
clayborg updated this revision to Diff 158631.Aug 1 2018, 2:06 PM
  • run clang-format
  • fix doxygen parameter names
clayborg marked 2 inline comments as done.Aug 1 2018, 2:07 PM
clayborg added inline comments.
source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
58

No need to change the x86. Just seems like more work to map one set of registers to another when/if the actual context is in another format. The ARM registers differ from any other ARM register context as they have 8 "extra" registers, so it didn't make sense to try and map it to another register context. So the "don't like the x86" part is just because it was remapping from one context to another and these register contexts are simple.

This revision was automatically updated to reflect the committed changes.

Please remember to test with the cmake build when you add or remove files,
as that is the build that all of the buildbots use. I almost reverted this
since it broke every LLDB buildbot, but I noticed that it's just forgetting
to remove the files from the CMakeLists.txt so I'll fix it.

There are a number of minidump tests that started failing for us on both Linux and Windows and I suspect it's due to this change. Did the unit tests pass for you with the changes on either Linux or Windows?

Failing Tests (6):

lldb-Unit :: Process/minidump/./LLDBMinidumpTests/MinidumpParserTest.GetMemoryListNotPadded
lldb-Unit :: Process/minidump/./LLDBMinidumpTests/MinidumpParserTest.GetMemoryListPadded
lldb-Unit :: Process/minidump/./LLDBMinidumpTests/MinidumpParserTest.GetModuleListNotPadded
lldb-Unit :: Process/minidump/./LLDBMinidumpTests/MinidumpParserTest.GetModuleListPadded
lldb-Unit :: Process/minidump/./LLDBMinidumpTests/MinidumpParserTest.GetThreadListNotPadded
lldb-Unit :: Process/minidump/./LLDBMinidumpTests/MinidumpParserTest.GetThreadListPadded

I don't see this mentioned here yet, so: This patch also seems to introduce a few hundred warnings with -Wextended-offsetof (which is enabled by default on the macOS builds):

[...]llvm/tools/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:510:5: warning: using extended field designator is an extension [-Wextended-offsetof]                                                                                                                                                                         
    DEF_S(20),                                                                                                                                                                          
    ^~~~~~~~~                                                                                                                                                                           
[...]llvm/tools/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:67:25: note: expanded from macro 'DEF_S'                             
    "s" #i, nullptr, 4, OFFSET(v[i * 16]), eEncodingVector,                    \                                                                                                        
                        ^~~~~~~~~~~~~~~~~                                                                                                                                               
[...]llvm/tools/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:29:20: note: expanded from macro 'OFFSET'                            
#define OFFSET(r) (offsetof(RegisterContextMinidump_ARM64::Context, r))                                                                                                                 
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                                  
[...]stddef.h:120:24: note: expanded from macro 'offsetof'                        
#define offsetof(t, d) __builtin_offsetof(t, d)                                                                                                                                         
                       ^                     ~

(And the tests also fail on macOS, but they are probably fixed when the Linux/Windows tests are fixed).

labath added a subscriber: rovka.Aug 3 2018, 1:54 AM

I've reverted this to keep the bots green until the issues pointed out
by Stella and Raphael are resolved.

Based on a quick inspection, it seems that the test issue is that
GetSystemInfo call that has been added to MinidumpParser::Initialize
is failing. I guess that's because the hand-crafted(?) minidumps for
these tests don't contain the necessary data.

For the -Wextended-offsetof issue, the way that other register
contexts avoid those is by factoring this out into two offsetof
expressions (offsetof(big_struct, small_struct_field) +
offsetof(small_struct, actual_field)).