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

[Vitaly Buka via Phabricator via libcxx-commits]

vitalybuka added a comment.

can you please add a test here compiler-rt/test/asan/TestCases/contiguous_container.cpp ?



================
Comment at: compiler-rt/include/sanitizer/common_interface_defs.h:162-165
+/// Similar to <c>__sanitizer_annotate_contiguous_container(const void *beg,
+///                                            const void *end,
+///                                            const void *old_mid,
+///                                            const void *new_mid)</c>
----------------



================
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);
----------------
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);


================
Comment at: compiler-rt/include/sanitizer/common_interface_defs.h:188
+                                                        const void *end,
+                                                        const void *new_con_beg,
+                                                        const void *old_con_beg,
----------------
inconsistency
here order "new, old"
and below is "old, new" (I would prefer if we stick with existing this one)




================
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,
----------------
Would be nice to find something better than _de_ in the name,  double_ended ?


================
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) {
----------------

maybe 
beg_p -> storage_beg_p
end_p -> storage_end_p



================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:421
+  uptr granularity = ASAN_SHADOW_GRANULARITY;
+  if (!(beg <= new_con_beg && beg <= old_con_beg && new_con_beg <= end &&
+        old_con_beg <= end && con_end <= end && old_con_beg <= con_end &&
----------------
this check and blow is hard to read, can we improve this
maybe

(beg <= con_end_p && con_end_p <= end) && 
(beg <= new_con_beg && new_con_beg <= con_end_p) &&
(beg <= old_con_beg_p && old_con_beg_p <= con_end_p)


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:433-434
+
+  uptr a = RoundDownTo(Min(new_con_beg, old_con_beg), granularity);
+  uptr c = RoundDownTo(Max(new_con_beg, old_con_beg), granularity);
+  // WARNING: at the moment we do not poison prefixes of blocks described by one
----------------
instead of Min/max, probably better to simplify and sink them into corresponding branches below.


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:472
+        c != RoundUpTo(
+                 new_con_beg,
+                 granularity))  // is empty && ends in the middle of a block
----------------
IsAligned


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:500
+  uptr granularity = ASAN_SHADOW_GRANULARITY;
+  if (!(beg <= old_con_end && beg <= new_con_end && old_con_end <= end &&
+        new_con_end <= end && IsAligned(beg, granularity))) {
----------------
same with if restructuring.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132090