Copilot commented on code in PR #13610: URL: https://github.com/apache/trafficserver/pull/13610#discussion_r3915897056
########## CMakeLists.txt: ########## @@ -449,6 +450,35 @@ 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. Review Comment: The comment asserts non-fatal behavior, but the build flags don’t explicitly select recover/non-recover mode (defaults vary by compiler/checks). To make the behavior consistent and aligned with the comment, explicitly pass a recover-mode flag (e.g., `-fsanitize-recover=undefined`), and optionally document how to make it fatal via flags vs `UBSAN_OPTIONS`. ########## CMakeLists.txt: ########## @@ -449,6 +450,35 @@ 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) + # The checks are instrumentation, but they call into a runtime library that some + # distributions package separately from the compiler (libubsan for GCC). Without it + # the build gets a bare "cannot find libubsan.so" from the linker after compiling + # hundreds of objects, so establish it here and name what is missing. + include(CheckLinkerFlag) + check_linker_flag(CXX -fsanitize=undefined HAS_UBSAN_RUNTIME) + if(NOT HAS_UBSAN_RUNTIME) + message( + FATAL_ERROR + "ENABLE_UBSAN needs the undefined behavior sanitizer runtime, which this toolchain cannot link. On GCC that runtime is the libubsan package." + ) + endif() + add_compile_options(-g -fsanitize=undefined -fno-sanitize=vptr -fno-omit-frame-pointer) + add_link_options(-g -fsanitize=undefined -fno-sanitize=vptr) Review Comment: `-fno-sanitize=vptr` is a compile-time instrumentation control; it doesn’t affect the link step. Keeping it in `add_link_options(...)` is unnecessary and can cause avoidable toolchain-compatibility issues. Recommend removing `-fno-sanitize=vptr` from `add_link_options(...)` and leaving it only in `add_compile_options(...)`. ########## CMakeLists.txt: ########## @@ -449,6 +450,35 @@ 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) + # The checks are instrumentation, but they call into a runtime library that some + # distributions package separately from the compiler (libubsan for GCC). Without it + # the build gets a bare "cannot find libubsan.so" from the linker after compiling + # hundreds of objects, so establish it here and name what is missing. + include(CheckLinkerFlag) + check_linker_flag(CXX -fsanitize=undefined HAS_UBSAN_RUNTIME) + if(NOT HAS_UBSAN_RUNTIME) + message( + FATAL_ERROR + "ENABLE_UBSAN needs the undefined behavior sanitizer runtime, which this toolchain cannot link. On GCC that runtime is the libubsan package." + ) + endif() + add_compile_options(-g -fsanitize=undefined -fno-sanitize=vptr -fno-omit-frame-pointer) + add_link_options(-g -fsanitize=undefined -fno-sanitize=vptr) Review Comment: The comment asserts non-fatal behavior, but the build flags don’t explicitly select recover/non-recover mode (defaults vary by compiler/checks). To make the behavior consistent and aligned with the comment, explicitly pass a recover-mode flag (e.g., `-fsanitize-recover=undefined`), and optionally document how to make it fatal via flags vs `UBSAN_OPTIONS`. -- 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]
