Joe McDonnell 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: > 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. -- 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: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Mon, 22 Sep 2025 18:45:28 +0000 Gerrit-HasComments: No
