Copilot commented on code in PR #13610: URL: https://github.com/apache/trafficserver/pull/13610#discussion_r3905387989
########## CMakeLists.txt: ########## @@ -449,6 +450,23 @@ elseif(ENABLE_TSAN) add_link_options(-g -fsanitize=thread) endif() +# UndefinedBehaviorSanitizer. Deliberately not part of the either/or chain above: +# UBSan instruments arithmetic, shifts, and type loads rather than replacing the +# allocator or the thread runtime, so it composes with an asan build +# (-fsanitize=address,undefined) as well as standing on its own. +# +# vptr is excluded. It needs a matching type_info at every polymorphic access, and +# a plugin loaded with dlopen does not reliably share type identity with the main +# image, so the check reports the plugin boundary rather than a real bug. +# +# Findings are non-fatal by default so a single run reports every distinct site +# instead of stopping at the first. Set UBSAN_OPTIONS=halt_on_error=1 at runtime +# to abort instead, which is what a gating job would want. +if(ENABLE_UBSAN) + add_compile_options(-g -fsanitize=undefined -fno-sanitize=vptr -fno-omit-frame-pointer) + add_link_options(-g -fsanitize=undefined -fno-sanitize=vptr) Review Comment: `add_compile_options(...)` applies to all languages, including C. The flag `-fno-sanitize=vptr` is C++-specific and can cause C compilation to fail when `ENABLE_UBSAN` is enabled (depending on compiler/toolchain). Restrict `-fno-sanitize=vptr` to C++ compilation via a generator expression (e.g., only for `COMPILE_LANGUAGE:CXX`), and consider dropping/guarding the link-time `-fno-sanitize=vptr` entirely since it’s a compile-time instrumentation control. ########## CMakeLists.txt: ########## @@ -449,6 +450,23 @@ elseif(ENABLE_TSAN) add_link_options(-g -fsanitize=thread) endif() +# UndefinedBehaviorSanitizer. Deliberately not part of the either/or chain above: +# UBSan instruments arithmetic, shifts, and type loads rather than replacing the +# allocator or the thread runtime, so it composes with an asan build +# (-fsanitize=address,undefined) as well as standing on its own. +# +# vptr is excluded. It needs a matching type_info at every polymorphic access, and +# a plugin loaded with dlopen does not reliably share type identity with the main +# image, so the check reports the plugin boundary rather than a real bug. +# +# Findings are non-fatal by default so a single run reports every distinct site +# instead of stopping at the first. Set UBSAN_OPTIONS=halt_on_error=1 at runtime +# to abort instead, which is what a gating job would want. +if(ENABLE_UBSAN) + add_compile_options(-g -fsanitize=undefined -fno-sanitize=vptr -fno-omit-frame-pointer) + add_link_options(-g -fsanitize=undefined -fno-sanitize=vptr) +endif() Review Comment: The UBSan block unconditionally adds toolchain-specific flags without checking whether the current compiler/linker supports them. This can break configuration/builds on unsupported compilers/platforms when `ENABLE_UBSAN` is turned on. Use CMake feature checks (e.g., `check_cxx_compiler_flag` / `check_linker_flag`) and conditionally append only the supported flags (and emit a clear fatal error if `ENABLE_UBSAN` is requested but `-fsanitize=undefined` isn’t available). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
