[clang] [nfc][analyzer] Add MemSpace trait to program state (PR #123003)
DonĂ¡t Nagy via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 27 02:40:46 PST 2025
NagyDonat 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.
I would strongly support changing `MemRegion::getMemorySpace()` into `MemRegion::getMemorySpace(ProgramStateRef)` even within this commit if it's feasible (and perhaps leave behind a `MemRegion::getRawMemorySpace()` if needed e.g. for the ArrayBoundCheckerV2 lower bound check) -- simply because there is no reason to leave behind the `ProgramStateRef`-less version, even temporarily.
However, I would strongly support continuing this refactoring effort by dropping the memspaces from the memregions in follow-up commits -- I don't think that it would be terribly difficult and it would make it easier to reason about memregions. (If nobody else does it, I will eventually do it when I get fed up on some confusing `MemRegion`-related code.)
https://github.com/llvm/llvm-project/pull/123003
More information about the cfe-commits
mailing list