[PATCH] D58121: [analyzer][WIP] Attempt to fix traversing bindings of non-base regions in ClusterAnalysis

Gábor Horváth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 21 04:27:49 PDT 2019


xazax.hun added a comment.

First of all, sorry for the inactivity regarding this patch.

In D58121#1437433 <https://reviews.llvm.org/D58121#1437433>, @NoQ wrote:

> Ok, got it! Yeah, this sounds like a valid way of supporting non-base-region-based worklist items, i'm seeing no problems with it and i don't immediately see a solution that'd be better than gradually supporting non-base-regions in more and more places.
>
> Here's the slight modification of the original test case (D57230#1389766 <https://reviews.llvm.org/D57230#1389766>) that my problem finally boiled down to:
>
>   typedef __typeof(sizeof(int)) size_t;
>   void *malloc(size_t);
>  
>   struct S {
>     int x; // Make sure ptr has a non-zero offset within baseR.
>     int *ptr;
>   };
>  
>   struct T {
>     struct S s; // This is baseR.
>   };
>  
>   void escape(struct S *);
>  
>   void foo() {
>     struct T t; // This is realBaseR.
>     t.s.ptr = malloc(sizeof(int));
>     escape(&t.s);
>   }
>
>
> This happens because in this patch you're pin-pointing the value by trying to look it up via an exact binding key (up to default/direct), however it may also reside at a non-zero offset within `baseR`. In my case the offset was also symbolic - which kept me confused for a while because i was thinking that symbolic offsets are the root of all evil. I think you may be able to generalize the patch to cover this scenario by replacing the exact offset lookup with a `collectSubRegionBindings()` (it might be cute to make some sort of `iterSubRegionBindings()` that takes a lambda, and/or combine it with `iterBindings()`).


Oh, I see. Thank you for tracking this down! Your proposed solution sounds reasonable to me. I hope there will be no significant performance hit.

> I'm still worried about D57230#1434161 <https://reviews.llvm.org/D57230#1434161> though. It kinda sounds like a good idea to suppress store invalidation but not escaping, but it's a very annoying thing to implement because our invalidation traits are very inflexible. In order to express complex requirements like "please don't invalidate this region, except this sub-region", the whole data structure has to be changed. This is slightly similar to this problem we have in:
> 
>   void escape(int *x, const int *y);
>   // ...
>   escape(&a, &a);
> 
> 
> where the value of `a` would not be invalidated because `y` is a const pointer, even though it is passed through a non-const pointer `x`. @xazax.hun, do you have enough time/inspiration to dig that deeply into this stuff?

I totally agree that invalidation and escaping are orthogonal. I really glad you found this example, I think you are right, the current data structure is not flexible enough. Unfortunately, I have some unrelated deadlines coming up, so it is very likely that I can only start working on this problem around early May. If you have some time to work on these problems feel free to commandeer this patch and I will definitely try to find time to review your patches.


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

https://reviews.llvm.org/D58121





More information about the cfe-commits mailing list