[libc++] std::deque loops over the deque for ASAN even when annotations are disabled

[ldionne]

This seems unintended: https://github.com/llvm/llvm-project/blob/main/libcxx/include/deque#L987

[AdvenamTacet]

We rely here on compiler optimizations. The loop comprises no-op operations and is thus expected to be eliminated during the optimization process. That this optimization can happez can be confirmed by examining the assembly code. When compiled with the -O1 flag, LLVM16 and LLVM17 produce identical output.

This code also iterates over modified chunks only (not even elements), so it should never impact complexity. When one object is added/removed and annotations are not optimized-out, the loop is executed only once.

It should also affect only programs without optimizations (-O0) in any way.
Annotations should not end up in optimized binaries.

When compiled without optimizations (-O0), based on one oversimplified benchmark I was able to run, the performance impact of this code is negligible. (I compared the performance of LLVM16 and LLVM17.)

However, that amount of additional code with -O0 is not desirable, I can prepare a patch on Monday minimizing it (but not fully removing). But should I do the same for the std::vector container? While vector adds much less calculations, the logic is the same: annotate_* is called and some calculations are made. That would double the amount of annotating functions.

The rationale behind maintaining a single set of functions and relying on compiler optimizations lies in the avoidance of redundant code maintenance and minimization of conditional code guarded by #ifdef directives. These optimizations are relatively straightforward, and I anticipate encountering no instances where annotations persist during optimization.

In summary, I propose retaining the current implementation unless there are instances where these functions fail to undergo optimization. However, I acknowledge that __annotate_from_to is more extensive than the other annotating functions, potentially warranting an exception. While I currently favor maintaining the existing code, I am open to creating separate versions of __annotate_from_to or all functions if deemed necessary. I will conduct further research and provide an update early next week.

[ldionne]

Thanks a lot for your analysis. Would it be possible to do this instead?

_LIBCPP_HIDE_FROM_ABI
void __annotate_from_to([[maybe_unused]] size_type __beg, 
                        [[maybe_unused]] size_type __end, 
                        [[maybe_unused]] __asan_annotation_type __annotation_type, 
                        [[maybe_unused]] __asan_annotation_place __place) const _NOEXCEPT {
#if !defined(_LIBCPP_HAS_NO_ASAN)
    // __beg - index of the first item to annotate
    // __end - index behind the last item to annotate (so last item + 1)
    // __annotation_type - __asan_unposion or __asan_poison
    // __place - __asan_front_moved or __asan_back_moved
    // Note: All indexes in __map_
    if (__beg == __end)
        return;

    ...

#endif
}

The same approach could be done for __annotate_double_ended_contiguous_container in deque but also for the other functions in vector. This would not require having multiple versions of each function but there would indeed be some more #ifdefs in the code. In this case I think this is reasonable though, I don't think we should be relying on the compiler's ability to remove dead code that much in such widely used classes.

[AdvenamTacet]

I wasn't aware that [[maybe_unused]] exists, that resolves the problem of duplicated functions.

That should work. I can test it and create the PR (Monday at the latest). I agree it's reasonable.

I will do it in deque and vector (and string PR) for functions with two versions, as well as __annotate_from_to, because it has a lot of code inside.

If you think that we should do that for every __annotate... function, I can create a separate PR, but I think we should ask more people for feedback. Throughout the development of these patches, I received feedback from 2+ persons, asking to minimize the code guarded by macros (it wasn't my idea). And all other functions have minimal code inside, often even only a single call, so I'm unsure about benefits.

[AdvenamTacet]

Hey, I created #74023 based on suggestion from discord ([[__maybe_unused__]] instead of [[maybe_unused]], thx @philnik777), it seems to work with C++03, the failed test seems to be unrelated. Seems ready for the review, I will rebase the commit soon and hopefully it fixes the test case fail.