On Fri, Sep 13, 2024 at 6:47 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Fri, 13 Sept 2024 at 16:55, Peter Xu <pet...@redhat.com> wrote: > > > > On Thu, Sep 12, 2024 at 03:27:55PM +0100, Peter Maydell wrote: > > > Coverity is pretty unhappy about this trick, because it isn't able > > > to recognise that we can figure out the address of 'bounce' > > > from the address of 'bounce->buffer' and free it in the > > > address_space_unmap() code, so it thinks that every use > > > of address_space_map(), pci_dma_map(), etc, is a memory leak. > > > We can mark all those as false positives, of course, but it got > > > me wondering whether maybe we should have this function return > > > a struct that has all the information address_space_unmap() > > > needs rather than relying on it being able to figure it out > > > from the host memory pointer... > > > > Indeed that sounds like a viable option. Looks like we don't have a lot of > > address_space_map() users. > > There's quite a few wrappers of it too, so it's a little hard to count. > We might want to avoid the memory allocation in the common case > by having the caller pass in an ASMapInfo struct to be filled > in rather than having address_space_map() allocate-and-return one.
Hm, this would work, but not only does it complicate the code consuming address_space_map, but it also increases memory footprint (a pointer being replaced by a struct of sizeof(BounceBuffer) if done naively), plus there's an additional pointer indirection (I'm doubtful whether this can be optimized away by the compiler). I haven't done any measurements of these effects, so can't say anything definitive, but this seems pretty costly just to appease coverity... Is there no way to inform coverity that a resource pointer is being transmuted into a handle, so it can track that instead? Given that pointer tricks like this and container_of usage is quite frequent, I would expect coverity to have a better strategy to handle these rather than suppressing false positive leak reports? If coverity can be made to understand this, it seems we should at least be able to mark the allocation in question as intentional leak (and thus not to report) at the position in the code where we allocate the bounce buffer, rather than having to create false positive suppressions for every caller. Perhaps this would be simplified if we extract out a function to allocate and initialize the bounce buffer, and then tell coverity to assume that anything it returns is not a leak? (My coverity experience is rather limited, just thinking out loud based on what I've seen in other projects to help static analysis tools in general)
