Closed Bug 1510574 Opened 6 years ago Closed 6 years ago

Linux32 crash symbols missing at least in mochitests

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: NarcisB, Assigned: gsvelto)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1510574 - Remove shared memory segments from generated minidumps to cut down on the number of modules
(deleted), text/x-phabricator-request
Details
Can you take a look why the symbols are missing here, Gabriele? Thank you.
Flags: needinfo?(gsvelto)
I grabbed a minidump from the failing ones and the issue there was fairly silly. Breakpad's minidump-reading logic fails if the number of modules in the modules list exceeds a certain number: https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/toolkit/crashreporter/google-breakpad/src/processor/minidump.cc#2725 This can be overridden but minidump_stackwalk sticks with the default which is 2048: https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/toolkit/crashreporter/google-breakpad/src/processor/minidump.cc#3861 The minidump I looked at had 5800+ modules (mostly because of deleted mappings). I bumped up the limit locally and the minidump was processed just fine, symbolicated stack and all. It took ~7 seconds on my machine which is a fair bit longer than usual but nothing dramatic. Ted, do you think we can raise this limit? minidump_dump already sets it to MAX_UINT32 so maybe we can go a bit higher than 2048 without suffering adverse effects.
Flags: needinfo?(gsvelto) → needinfo?(ted)
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
you'll need using google_breakpad::MinidumpModuleList; as well. btw, this fixes a long standing issue for me though I also included the change to minidump.cc as well though not sure it is required.
Flags: needinfo?(gsvelto)
May have spoken too soon. Partially fixed it appears.
Flags: needinfo?(gsvelto)
Summary: Linux debug crash symbols missing at least in mochitests → Linux32 crash symbols missing at least in mochitests
I commented in Phabricator, but we don't currently build the in-tree code for use in automation. glandium updated the (very old) binaries we had been using in bug 1512706, but punted on a more complete solution because it got complicated. It looked like he might fix bug 1391408 though.
Depends on: 1391408
Flags: needinfo?(ted)
I will, but I also won't go further to hook it up for test jobs to download it from the toolchain tasks.
Attachment #9028567 - Attachment is obsolete: true
Blocks: 1517131
I was wondering, would it make sense to build minidump_stackwalk as an host binary like we do with dump_syms instead of taking it from outside of mozilla-central?
That's essentially what I have WIP patches for (except it's not /exactly/ building them as host binaries)
Would this affect Linux64 too? I've been running into this on my Linux64 try pushes for at least a week, and it's a huge pain... If this bug doesn't cover Linux64, I'm happy to file a separate one to track that.
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #13) > Would this affect Linux64 too? I've been running into this on my Linux64 > try pushes for at least a week, and it's a huge pain... Yes, it would affect all platforms.
(In reply to Gabriele Svelto [:gsvelto] from comment #4) > The minidump I looked at had 5800+ modules (mostly because of deleted mappings). Is there a way to convince breakpad to ignore non-executable mappings? If it weren't picking up shared memory and data files, that number would probably be a lot smaller. (Shared memory segments on Linux currently show up as deleted files, because they are, but I've also seen breakpad pick up memfd mappings created by system libraries so bug 1440203 probably won't help here.)
The Linux dump writing code does all sorts of fiddling with the list of mappings, so we could absolutely filter out stuff that's not useful: https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/crashreporter/breakpad-client/linux/minidump_writer/linux_dumper.cc#575 We do stuff /proc/self/maps into the minidump verbatim (it's hard to get at without running minidump_dump, but it's there), so I don't think we'd lose much by doing that.

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #16)

The Linux dump writing code does all sorts of fiddling with the list of
mappings, so we could absolutely filter out stuff that's not useful:
https://dxr.mozilla.org/mozilla-central/rev/
c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/crashreporter/breakpad-
client/linux/minidump_writer/linux_dumper.cc#575

If I'm following that correctly, it merges adjacent mappings from the same file, and tracks whether any of those mappings was executable… but does Breakpad ever need to do anything with non-executable segments?

In any case, for code that isn't actually a library or executable, MappingInfo::exec should always be false, and vice versa.

No, Breakpad has no need for non-executable segments.

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #18)

No, Breakpad has no need for non-executable segments.

... And yet, they are included in crash reports as modules.

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

... And yet, they are included in crash reports as modules.

I'm on PTO this week but I'll resume work on this ASAP. The starting point as we had discussed is to fork breakpad for good and do all the adjustments we need to fix our short-term pain-points. I would also like to fix this issue as quickly as possible. I've got patches to build minidump_stackwalker as a host tool, Mike can you attach your changes so that I can integrate them? What I still need to do is adjust our automation script to pick up the host tool instead of using tooltool but that shouldn't be too hard.

Flags: needinfo?(mh+mozilla)

I've got patches to build minidump_stackwalker as a host tool

That's not enough. Because the host is not necessarily where it's going to run. For one, minidump_stackwalk doesn't build on Windows, so windows builds need one built with mingw. And mac tests need a native mac one, but the host for mac builds is linux. Which is why I went with separate jobs to build minidump_stackwalk. Which should all be landed by the time you're back from PTO.

In any case, minidump_stackwalk is not involved in putting modules in minidumps.

Flags: needinfo?(mh+mozilla)

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

That's not enough. Because the host is not necessarily where it's going to run. For one, minidump_stackwalk doesn't build on Windows, so windows builds need one built with mingw. And mac tests need a native mac one, but the host for mac builds is linux. Which is why I went with separate jobs to build minidump_stackwalk. Which should all be landed by the time you're back from PTO.

That's nasty.

In any case, minidump_stackwalk is not involved in putting modules in minidumps.

You're right, I was still stuck on the idea of increasing the maximum number of modules allowed by the tool but it's much easier to trim down the module list when generating the dump. I'll hack the client code ASAP to do that.

OK, I've got a patch that removes the shared memory segments from the module list which seems to be working correctly. Getting rid of all the non-executable modules doesn't work because it messes up Breakpad's stack scanning logic. That logic should never be looking at pointers within an IPC shared segment so not having those modules shouldn't be a problem.

Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30fd54bb8089 Remove shared memory segments from generated minidumps to cut down on the number of modules r=ted

https://hg.mozilla.org/mozilla-central/rev/30fd54bb8089

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

For reference, I've made some notes about future changes that will need adjustments to the path filtering (bug 1426526 comment #2, bug 1440203 comment #6).

Blocks: 1524051
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: