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

[Martin Liška via Phabricator via llvm-commits]

marxin marked 11 inline comments as done.
marxin added a comment.

In https://reviews.llvm.org/D38971#900253, @kcc wrote:

> In https://reviews.llvm.org/D38971#899407, @marxin wrote:
>
> > Thanks, I'll add some tests. May I please ask you how to run a single test-case (I guess using llvm-lit) ?
>
>
> Frankly, dunno. The entire ninja check-llvm takes 40 seconds for me
>  Doesn't  this work?
>
>   llvm-lit <test path>


Agree that make check-sanitizer is really fast.



================
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:
> 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. 
I basically copied what's done in GlobalAddressDescription::Print:
  for (int i = 0; i < size; i++) {

I'm changing that to uptr.


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


================
Comment at: lib/asan/asan_descriptions.h:152
+  // as other. Descriptions can have different address within the variable
+  bool PointsInsideASameVariable(const GlobalAddressDescription &other) const;
 };
----------------
kcc wrote:
> naming nit pick: 
>   points inside the same var
> or 
>   points inside a same var
> ?
> 
> Not native speaker myself, but I think *the* is right. 
Agree, your name is better.


================
Comment at: lib/asan/asan_report.cc:329
+    {
+      ThreadRegistryLock l(&asanThreadRegistry());
+      is_left_on_stack = GetStackVariableBeginning(left, &shadow_offset1);
----------------
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.


https://reviews.llvm.org/D38971