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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
