[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

Fri May 13 10:28:45 PDT 2022

yln added inline comments.

================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2349
+  if (!cookieOffset.isZero())
+    cookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, cookieOffset);

----------------
Variable names should start with uppercase:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Please also be conservative with "rename cleanups" in reviews.  The smaller your patch is, the easier it is to review.  You can do these in a follow-up NFC commit.

================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:266
+#endif
 }

----------------
I just realized that we are poisoning the cookie, but we are not doing anything on the detection side.
Is this actually required to make detection work/the tests pass?

If this bit isn't needed to make the existing tests pass, then I would like to suggest 1 of 2 things:
* Remove the additional poisoning (recommended), -or-
* Add another (ARM-specific?) test that requires poisoning of the second cookie word.  I think this would require adaptation of `__asan_load_cxx_array_cookie` as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195