[clang] [nfc][analyzer] Add MemSpace trait to program state (PR #123003)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 24 11:25:12 PST 2025
steakhal wrote:
> > Have you considered changing `MemRegion::getMemorySpace()` into `MemRegion::getMemorySpace(ProgramStateRef)`?
>
> I thought about this, but I decided against it since I am thinking that MemSpaces will eventually be their own separate thing, not part of the MemRegion hierarchy, i.e., MemSpaces will eventually just be a trait thing, maybe with its own checker depending on how much functionality can be added with this different implementation?
>
> All this being said, I am happy to change the API of how we get memory spaces, I am not tied to the current approach/API, just that we can move towards better results for #115410.
Let express that I don't think we will ever drop memspaces from memregions, even if we strive for. The return of investment is not going to be there once we cover the unknownspace case - which you plan to fix the issue you referred to.
So I think we should aim for a consistent and better landscape to achieve that minimal goal.
I think having a single API (or overload set) is important for this. This is why I'd push for patching the `getMemorySpace()` overloads.
> > (Maybe in some contexts where we currently call `getMemorySpace` we just don't have a State, and can't easily get one - and that would kill this approach.)
>
> This has been the main thing I'm working through on this PR. This PR doesn't totally remove all `getMemorySpace` calls; this PR covers most of the files that use memory spaces, but there still remain some spots that are harder for me to change right now that I plan to work through. These are in RegionStore and ClusterAnalysis which don't take state, MemRegion (including the changes that would happen after removing MemSpace from the hierarchy) and all the print/dump methods which don't take state, and a dozen spots in StackAddrEscapeChecker that don't take state that I couldn't think of an immediate way to add state.
Yea, in those cases I think we should just do what we currently do: don't care about dynamically deduced memspaces, aka. let them use the `getMemorySpace()` without the `ProgramStateRef` parameter. I'm not even sure if those usecases in the Store and the Cluster analysis are really justified. Probably they don't even bring any benefit, so we should worry about them too much.
Given that StackAddrEscapeChecker is a checker, we should always have some State. I wonder what do you have in mind there exactly.
> This is also related to @Xazax-hun's above question about whether it would be possible to make all the changes here at once, do you have thoughts on that @steakhal, I think you tried some stuff here?
I think the best experience for everyone is if we make the smallest consistent step fixing #115410.
That is, be able to associate a deduced memspace with a memregion that was created with the Unknown memspace.
Once we have that, we can discuss how to move forward if we really want to move anywhere.
I've spent about one bit more than a day to completely get rid of memspaces from the memregions hierarchy. I should have chunk it up, but difficult to make incremental refactors with this so I gave up on that. I figured that I should spend my time on other things.
https://github.com/llvm/llvm-project/pull/123003
More information about the cfe-commits
mailing list