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.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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 | ||
|---|---|---|
| 149–150 | instead of useing m_arch as a cache I'd explicitly initialize it in MinidumpParser::Initialize() | |
| 212–216 | why is this needed when we have MinidumpOSPlatform::Linux ? | |
| source/Plugins/Process/minidump/ProcessMinidump.cpp | ||
| 179 | 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 | ||
| 192 | constexpr? | |
| 195 | use std::array for these kind of static arrays? (debug bounds checks, easy access to the static size, ...) | |
| 225 | symbolic constants or use the array size? | |
| 231 | lldb_assert ? | |
| 242 | not a big deal, but this kind of accessors for constants can be constexpr themselves | |
| 248 | failfast if out of bounds? who'd pass an invalid index and expect a meaninful result? | |
| 281 | remove unused parameter name or [[maybe_unused]] | |
| source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h | ||
| 82 | = nullptr; | |
| 83 | 0; | |
| source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h | ||
| 37 | default instead of {} ? | |
Comment Actions
I will make update the patch with many of the suggested inline comments.
| source/Plugins/Process/minidump/MinidumpParser.cpp | ||
|---|---|---|
| 150 | Can do | |
| 216 | 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 | ||
| 179 | 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. | |
| source/Plugins/Process/minidump/MinidumpParser.cpp | ||
|---|---|---|
| 216 |
| |
| source/Plugins/Process/minidump/ProcessMinidump.cpp | ||
| 179 | 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? | |
| source/Plugins/Process/minidump/ProcessMinidump.cpp | ||
|---|---|---|
| 179 | 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 | ||
| 43–56 | 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). | |
| source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp | ||
|---|---|---|
| 51 | Pavel's comment reminded me: what about the S registers (32bit fp) and Q registers (128bit Neon)? | |
Comment Actions
FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM
minidumps soon.
Comment Actions
- 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
| source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp | ||
|---|---|---|
| 192 | will do | |
| 195 | Tried it but it introduces a global constructor. We try to avoid those. | |
| source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp | ||
| 56 | 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. | |
Comment Actions
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 | ||
| 195 | 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) | |
Comment Actions
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 ↗ | (On Diff #158371) | mismatch in the parameter name in the comment and signature |
| source/Plugins/Process/minidump/MinidumpParser.cpp | ||
| 569 | should error be initialized here? | |
| source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp | ||
| 56 | 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? | |
| 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. | |
Comment Actions
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.
Comment Actions
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
Comment Actions
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).
Comment Actions
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)).
instead of useing m_arch as a cache I'd explicitly initialize it in MinidumpParser::Initialize()