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

[Kostya Serebryany via Phabricator via llvm-commits]

kcc added a comment.

In https://reviews.llvm.org/D38971#901428, @marxin wrote:

> Analyzing errors spotted on GCC itself, I would suggest to have 3 values for detect_invalid_pointer_pairs:
>
> - 0 = disabled
> - 1 = all except comparison with zero (0x0)
> - 2 = all
>
>   Thoughts?


What does the standard say about comparisons with zero and subtracting zero from a pointer?



================
Comment at: lib/asan/asan_report.cc:329
+    {
+      ThreadRegistryLock l(&asanThreadRegistry());
+      is_left_on_stack = GetStackVariableBeginning(left, &shadow_offset1);
----------------
marxin wrote:
> kcc wrote:
> > 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. 
> Unfortunately yes. Do you mean to call FindThreadByStackAddress (which currently needs lock as it calls FindThreadContextLocked)?
> Then comparing the variables if threads do match?
> 
> I can come up with a test-case for that.
We should not call anything that grabs a lock here. 
Let's start from simply ignoring any pointers from threads other than the current. 


================
Comment at: lib/asan/asan_report.cc:308
+  if (offset <= 2048) {
+    if (__asan_region_is_poisoned(left, offset) == 0)
+      return false;
----------------
isn't this the same as
   return __asan_region_is_poisoned(left, offset);


https://reviews.llvm.org/D38971