[libcxx-commits] [PATCH] D132090: [1a/3][ASan][compiler-rt] API for double ended containers

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet planned changes to this revision.
AdvenamTacet added inline comments.


================
Comment at: compiler-rt/include/sanitizer/common_interface_defs.h:186-190
+void __sanitizer_annotate_de_contiguous_container_front(const void *beg,
+                                                        const void *end,
+                                                        const void *new_con_beg,
+                                                        const void *old_con_beg,
+                                                        const void *con_end);
----------------
vitalybuka wrote:
> maybe just a single function?
> 
> void __sanitizer_annotate_de_contiguous_container(
>                                                         const void *beg,
>                                                         const void *end,
>                                                         const void *old_con_beg,
>                                                         const void *old_con_end,
>                                                         const void *new_con_beg,
>                                                         const void *new_con_end);
That was my first idea, but after some thinking I decided against it.
- I see almost zero situations when one may want to change the beginning and the end at the same time, so there is no point.
- It may be very small, but it may have a bad impact on performance.
- It's less important, but it would be harder to remember the correct order and some mistakes may not be noticed instantly.
```
// Intended:
__sanitizer_annotate_de_contiguous_container(0x8, 0x1000, 0x10, 0x11, 0x99, 0x100);
// Mistake:
__sanitizer_annotate_de_contiguous_container(0x8, 0x1000, 0x10, 0x99, 0x11, 0x100);
```
I feel that it just complicates usage of those functions and brings no benefits, it's easier to just see `_front` and `_back` instead of checking what two arguments are the same.
One less function in API seems nice, but I suggest two.

Do you know any examples when changing the beginning and the end at the same time may be helpful?
Do you see any benefits, which I don't see?


================
Comment at: compiler-rt/include/sanitizer/common_interface_defs.h:188
+                                                        const void *end,
+                                                        const void *new_con_beg,
+                                                        const void *old_con_beg,
----------------
vitalybuka wrote:
> inconsistency
> here order "new, old"
> and below is "old, new" (I would prefer if we stick with existing this one)
> 
> 
I understand why suggested order may be confusing, so I will change it, but it is consistent with poisoning. (If we move beginning forward, we have to poison, if we move end forward, we have to unpoison). But I understand why having new/old in the same order also makes sense.

Maybe the best order is
```
void __sanitizer_annotate_de_contiguous_container_front(
                                                        const void *storage_beg,
                                                        const void *storage_end,
                                                        const void *old_con_beg,
                                                        const void *con_end,
                                                        const void *new_con_beg);
```
and in `__sanitizer_annotate_de_contiguous_container_back`:
```
    const void *storage_beg,
    const void *storage_end, 
    const void *con_beg,
    const void *old_con_end,
    const void *new_con_end);
```
Instead of con_beg at the end?

What do you think?

Btw. I see additional `_p` in names, I will remove it with next update.


================
Comment at: compiler-rt/include/sanitizer/common_interface_defs.h:214
+/// - new_con_end_p == con_beg_p
+void __sanitizer_annotate_de_contiguous_container_back(
+    const void *beg_p, const void *end_p, const void *old_con_end_p,
----------------
vitalybuka wrote:
> Would be nice to find something better than _de_ in the name,  double_ended ?
I didn't want to create very long names, but it's probably a good idea. I changed it.


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:357-362
   if (!(beg <= old_mid && beg <= new_mid && old_mid <= end && new_mid <= end &&
         IsAligned(beg, granularity))) {
     GET_STACK_TRACE_FATAL_HERE;
     ReportBadParamsToAnnotateContiguousContainer(beg, end, old_mid, new_mid,
                                                  &stack);
   }
----------------
vitalybuka wrote:
> to my taste this is CHECK() case, but no need to change in this patch
> can simplify this in later (on in before) patch
That function is not added by me, therefore I suggest leaving it for another patch. For consistency, I suggest leaving similar code in new functions and possibly changing them in a future patch.

But, I'm not sure how it's possible to use CHECK here, without decreasing feedback for users.


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:400
+void __sanitizer_annotate_de_contiguous_container_front(
+    const void *beg_p, const void *end_p, const void *new_con_beg_p,
+    const void *old_con_beg_p, const void *con_end_p) {
----------------
vitalybuka wrote:
> 
> maybe 
> beg_p -> storage_beg_p
> end_p -> storage_end_p
> 
I changed it. I also changed `_con_` -> `_container_`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132090