[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)

[via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/72402 at github.com>


DonatNagyE wrote:

> I think the original `alloca(0)` warning message might be clearer/easier to understand.

The difficulty is that we switch from the `SymbolicRegion` that represents the heap allocation to an `AllocaRegion` that doesn't have an associated symbol, while the code that produces the warnings about zero-allocated memory areas (essentially) maintains a set of _symbols_ that correspond to zero-allocated regions (I'd guess that this was implemented before the introduction of dynamic extent).

It would be possible to reintroduce the original `alloca(0)` message (while keeping the improvement that `alloca` returns an `AllocaRegion` and not a `SymbolicRegion` on the heap), but that would require significant refactoring (and/or ugly code duplication) and frankly I feel that `alloca` is rare and this would be a waste of time.

> While it might have platform or compiler dependent meaning, those behaviors are non-portable, so I think it is undesirable in most cases and people probably want to be notified about it.

I'd say that "non-portable" and "undesirable in most cases" is true for _practically all_ applications of `alloca`, not just the `alloca(0)` corner case. Experienced programmers who use `alloca` check that their code works with _their own particular_ `alloca` and wouldn't want to see generic messages that may or may not be relevant for them; while novices ( / projects that don't want to deal with `alloca` issues) would be better served by a generic "don't use `alloca`, it's platform-specific" warning.

> Regarding binding the return value outside of `evalCall`, I think that could be addressed in a separate PR. This one does not make the current situation any worse. But the very least we should add a FIXME now (and potentially open a ticket), to reduce the chances of this getting forgotten.
> 
> Edit: we already have a TODO, but I think that one does not really do justice to the urgency of the problem, it does not mention that doing bindings in checkers outside of `evalCall` is not a good idea.

I'll spice up that TODO note; and after merging this commit I could start working on fixing it.

https://github.com/llvm/llvm-project/pull/72402