[libc-commits] [PATCH] D78612: [libc] Add sanitizer instrumentation to the SIMD strlen implementation.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

I'm not sure if this discussion belongs here or D77949 <https://reviews.llvm.org/D77949>, but as this complicates that patch even more I will do so here.

I really wonder if the added complexity is worth the speedup we are hoping to achieve. Firstly even if this implements ASan which is by far the most common and ubiquitously useful, it makes sense to implement it here, we also have fuzz tests set up which rely on `-fsanitize=fuzzer`. That won't be implemented here. The whole appeal of sanitizers is that nothing needs to change in our code, but it's not true here no matter what we do if we rely on UB.

This patch is well done and it's very clever and easy to understand way to implement by hand what `-fsanitize=address` is doing. But it is undeniably making this much less maintainable. Is the cost we are paying in maintainability worth the speedup we are hoping to get using this implementation? If this patch is only for `strlen` and not an example for how we will handle all such cases which rely on UB for clever tricks, then I would say it is not worth it.

FWIW all of that would be moot if this was performance critical, but I'm not convinced that `strlen` is a particularly hot function, especially given its use outside of C is very limited. Even in C++ it would be used much less frequently and for programs written in other languages, that would be even less than that is my guess. Even if it were a hot function, how large does the string need to be before the implementation in D77949 <https://reviews.llvm.org/D77949> is better than what we currently have?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78612/new/

https://reviews.llvm.org/D78612