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 5: (3 comments) > (3 comments) > > Given the urgency of this fix, it would be great to have some unit > tests added for this that verify boundary, alignment, concurrency > (wherever applicable) conditions in a separate patch. Thank you for the review. Yep, it would be great to put together a few unit tests. As for the testing that has been already performed, I can report that this code has passed many hours (even a few days) of many (30+) independent runs of CodegenTest.CodegenRandomSchemas stress test on x86_64 and aarch64 architectures on different OS versions. So, I'm quite confident it works as intended, at least in the runtime environment the MCJIT execution engine as of LLVM 11.0. http://gerrit.cloudera.org:8080/#/c/23925/5/src/kudu/codegen/jit_frame_manager.cc File src/kudu/codegen/jit_frame_manager.cc: http://gerrit.cloudera.org:8080/#/c/23925/5/src/kudu/codegen/jit_frame_manager.cc@76 PS5, Line 76: (if present) > When is it expected to be present? It's present for other than pre-allocation cases. It's not documented in this in-line docs blurb as is, but it's clear if glancing over the in-line comments in the implementation below. http://gerrit.cloudera.org:8080/#/c/23925/5/src/kudu/codegen/jit_frame_manager.cc@110 PS5, Line 110: (num_bytes + page_size - 1) > nit: do you think overflow check is required before this? num_bytes is always greater than 0 here because of the if() condition at line 86, and page_size cannot be less than 1. MCJIT does not request a lot of memory for this: we can add a check for overflow, but it would be superfluous. http://gerrit.cloudera.org:8080/#/c/23925/5/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/23925/5/thirdparty/download-thirdparty.sh@349 PS5, Line 349: 10 > nit: I think this is more of a logical level than number of patches applied That's a good point, thanks. However, I'd like to push this patch ASAP, so I'll leave this 10 as of PS5. -- 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: 5 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[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 15:57:42 +0000 Gerrit-HasComments: Yes
