Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/23443 )
Change subject: IMPALA-13472: Patch breakpad to fix minidump stacks for 64KB pages ...................................................................... Patch Set 1: Code-Review+1 > > To my best understanding, 2 things happen here: > > + We round the pointer to whole pages, so we look up the starting > > address in FindMapping (which should not make a difference), and > we > > also copy whole memory pages. > > + We maximize the copy to 32k to avoid overly large stack traces. > > > > I do not understand from the code or the git log why we need > whole > > pages, but wouldn't setting the upper limit to kStackToCapture = > > max(32 * 1024, page_size), to ensure that we still copy whole > > pages, make more sense? (Because that is what the code appears to > > be doing, although there is no reasoning given for it). > > I tried that approach and it functions properly, but it more than > doubles the size of the minidump in most cases. That's what > motivated me to pursue a more complicated approach. > > Breakpad does have logic to limit the size of a minidump, but it > doesn't kick in until there are a lot of threads. Here is how it > works: > 1. It calculates the expected size of the stacks based on how many > threads there are. It does this by multiplying the number of > threads by 8KB (a hard coded constant). > 2. If that expected size exceeds the limit, it collects the stack > regularly for 20 threads (and always the crashing thread). Then, it > limits the stack to 2KB for the remaining threads. > 3. The logic that limits the stack size to 2KB does understand that > the leading memory before the stack pointer is useless and skips it > (i.e. it advances the stack pointer by 2KB chunks until it is at > the 2KB boundary before the stack pointer). > > For Impala and Kudu, the limit is 20480KB, so the logic kicks in at > 20480 / 8 = 2560 threads. I think a lot of minidumps have fewer > than 2560 threads. > > It's not a big deal if we want a simpler approach, but I don't > think any of that extra memory is useful (other than the red zone). > >From what I can tell, Crashpad, the successor to Breakpad, doesn't > include extra stack memory other than the red zone. Thank you -- To view, visit http://gerrit.cloudera.org:8080/23443 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf19523e69628e78071636292b3cf464a7b626b8 Gerrit-Change-Number: 23443 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Tue, 23 Sep 2025 11:12:19 +0000 Gerrit-HasComments: No
