Marton Greber 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: (4 comments) Thanks for working on this, and for the detailed commit message Alexey! Couple questions from my end while still trying to understand every bit. Do we want any targeted tests for the new JIT memory manager behavior (e.g., accounting vs page‑rounded mappings and teardown order)? 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@95 PS2, Line 95: const size_t num_pages = (num_bytes + page_size - 1) / page_size; Are section allocation requests to the memory mapper guaranteed to be page-aligned/page-sized by LLVM? If not, the mapper’s internal page rounding + MAP_FIXED could potentially overshoot the reserved range unless we validate against the rounded size. http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@115 PS2, Line 115: ec = Memory::protectMappedMemory(result, protection_flags); Is it guaranteed that protectMappedMemory() always performs icache invalidation on this platform, and only when MF_EXEC is set? If not, do we need an explicit InvalidateInstructionCache call? http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@138 PS2, Line 138: JITFrameManager::~JITFrameManager() { SectionMemoryManager’s destructor releases all AllocatedMem blocks. Here we also release preallocated_block_ in the derived destructor. Are we intentionally double releasing sub ranges of that prealloc region, or should we avoid releasing preallocated_block_ here and let the base class handle teardown? 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); Do we rely on LLVM always passing non?zero alignment values here? -- 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: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Mon, 02 Feb 2026 15:30:23 +0000 Gerrit-HasComments: Yes
