[PATCH] D38971: Enhance libsanitizer support for invalid-pointer-pair.

[Kostya Serebryany via Phabricator via llvm-commits]

kcc added a comment.

Please use the arc tool to submit the patch or otherwise paste the patch with full context. 
Please always CC llvm-commits for changes in compiler-rt (phabricator doesn't do it automatically)

(My next response is likely to be on the next week)



================
Comment at: lib/asan/asan_descriptions.cc:225
 
+bool GetStackVariableBeginning(uptr addr, uptr *shadow_addr) {
+  if (AsanThread *t = FindThreadByStackAddress(addr)) {
----------------
You can simplify the interface and the implementation if you return shadow_addr or nullptr if it's not found. 


================
Comment at: lib/asan/asan_descriptions.cc:229
+    return true;
+  } else
+    return false;
----------------
no else after return


================
Comment at: lib/asan/asan_descriptions.cc:347
+bool GlobalAddressDescription::PointsInsideASameVariable
+  (const GlobalAddressDescription &other) const {
+  if (size == 0 || other.size == 0)
----------------
indentation doesn't look right. 
If in doubt, use clang-format 


================
Comment at: lib/asan/asan_descriptions.cc:353
+
+  for (unsigned i = 0; i < size; i++)
+    for (unsigned j = 0; j < other.size; j++) {
----------------
kcc wrote:
> We don't use 'unsigned' in this code for this purpose. 
int is even worse. 
In proper C++ the only good type for iteration is size_t. 
asan is not a proper C++ sadly, so we use uptr. 


================
Comment at: lib/asan/asan_descriptions.h:150
+
+  // Return true when this descriptions points inside a same global variable
+  // as other. Descriptions can have different address within the variable
----------------
Returns true


================
Comment at: lib/asan/asan_descriptions.h:152
+  // as other. Descriptions can have different address within the variable
+  bool PointsInsideASameVariable(const GlobalAddressDescription &other) const;
 };
----------------
naming nit pick: 
  points inside the same var
or 
  points inside a same var
?

Not native speaker myself, but I think *the* is right. 


================
Comment at: lib/asan/asan_report.cc:300
 
+#define REPORT_INVALID_POINTER_PAIR do {          \
+    GET_CALLER_PC_BP_SP;                          \
----------------
a macro is better than a goto, but still bad. Can this be expressed in just C++? 
(e.g. wrap the main logic into a separate function and return from it on error). 


================
Comment at: lib/asan/asan_report.cc:329
+    {
+      ThreadRegistryLock l(&asanThreadRegistry());
+      is_left_on_stack = GetStackVariableBeginning(left, &shadow_offset1);
----------------
ouch. We have a lock in a function that is called on every pointer cmp? 
This is bad. 
We need to avoid the lock. 
Probably by relaxing the check a bit (e.g. never compare pointers from other thread's stacks). 

Also, your tests don't seem to have threads, so this code is probably untested. 


================
Comment at: lib/asan/asan_thread.cc:357
+  }
+  uptr aligned_addr = addr & ~(SANITIZER_WORDSIZE/8 - 1);  // align addr.
+  u8 *shadow_ptr = (u8*)MemToShadow(aligned_addr);
----------------
Please use RoundUpTo() or some such 


================
Comment at: lib/asan/asan_thread.cc:364
+          *shadow_ptr != kAsanStackMidRedzoneMagic &&
+          *shadow_ptr != kAsanStackRightRedzoneMagic)) {
+    shadow_ptr--;
----------------
no need for {}


================
Comment at: test/asan/TestCases/invalid-pointer-pairs-compare-1.cc:1
+// RUN: %clangxx_asan -O0 %s -o %t -mllvm -asan-detect-invalid-pointer-pair
+
----------------
What does the file name mean (-1.cc vs -2.cc)? 
If these are positive vs negtive, either express this in the name or (better) put everything into one file. 
E.g. put all positives in the beginning and use CHECK-NOT to ensure we don't report anything there 



================
Comment at: test/asan/TestCases/invalid-pointer-pairs-subtract-1.cc:1
+// RUN: %clangxx_asan -O0 %s -o %t -mllvm -asan-detect-invalid-pointer-pair
+
----------------
same here. 


https://reviews.llvm.org/D38971