Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23925 )
Change subject: KUDU-3736 fix SIGSEGV in codegen with libgcc-11.5.0-10+ ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc File src/kudu/codegen/jit_frame_manager.cc: http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@138 PS2, Line 138: JITFrameManager::~JITFrameManager() { > This is intentional, and it's necessary since not all the pre-allocated spa As an alternative, we might override releaseMappedMemory() to be a no-op and only release the pre-allocated block here. munmap() should unmap all the internal sub-ranges. However, with such an approach it would be quite natural to go even further and fully mock all the underlying memory management within the pre-allocated block, doing all the internal accounting and call mprotect() instead of mmap() for the requested ranges just to provide the requested protection of the memory areas. I guess this a good next step if we want to reduce the number of mmap()/unmap() calls, but it should be properly implemented and tested. Since we want to fix this SIGSEGV crash issue ASAP, I'm not sure it makes sense to do it right now, but we can invest some time here, if necessary. As for the bigger picture, the code generation isn't in a hot path: all the compilations are done in the background and the original requestor goes without JIT-optimized projection implementation if the code isn't in the codegen cache already, so I don't see high ROI on this topic, frankly. One good trait of this current approach is that it is simple enough and adds just an overhead of one extra mmap()/munmap() syscall pair, plus a few trivial checks per allocation of a data/code section. http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@161 PS2, Line 161: const size_t code_required_size_bytes = : code_align * ((code_size + code_align - 1) / code_align); : const size_t ro_data_required_size_bytes = : ro_data_align * ((ro_data_size + ro_data_align - 1) / ro_data_align); : const size_t rw_data_required_size_bytes = : rw_data_align * ((rw_data_size + rw_data_align - 1) / rw_data_align); : : // Extra safety margin: pre-allocate 2 times more, aligning up the the memory : // page size for each section type. : const size_t required_size_bytes = 2 * ( : page_size * ((code_required_size_bytes + page_size - 1) / page_size) + : page_size * ((ro_data_required_size_bytes + page_size - 1) / page_size) + : page_size * ((rw_data_required_size_bytes + page_size - 1) / page_size)); > [nit] I think making a labda auto align_up = [](int x, int alignment) { ret That's a good point! Done. -- To view, visit http://gerrit.cloudera.org:8080/23925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I691d2f442c3148f235847c4c8e56767577804b1a Gerrit-Change-Number: 23925 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 03 Feb 2026 08:15:00 +0000 Gerrit-HasComments: Yes
