[clang] [Bounds Safety][NFC] Add `SemaBoundsSafety` class and move existing Sema checks there (PR #98954)

Dan Liew via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 16 05:44:04 PDT 2024


delcypher wrote:

> Unfortunately, I think this should be held off until we have a bigger picture for the future of `Sema`. I'll elaborate below.
> 
> If you take a close look at the existing set of `Sema` parts, it's almost entirely comprised of other languages or language extensions (from C and C++ standpoint) and backends. They were considered natural enough to be split off `Sema`, which, among other things, means that it's easy to explain their place in the big picture, like I did in the previous sentence.

As noted above `-fbounds-safety` **is a C language extension** which makes it seem like it would fit nicely into the existing division of Sema into multiple objects and relevant source files.

> I don't think this patch makes it easier to explain people where things are in this codebase, because (I think) `counted_by` is a declaration attribute, and we already have place for them. Having only one function also makes it a very small part of `Sema`, which doesn't sound compelling. 

It doesn't right now because most of `-fbounds-safety` implementation isn't upstream yet.  I simply moved what is **currently upstream**. As we (Apple) upstream more and more of the implementation it makes a lot sense to try to keep as much of it in its own source file to:

* Create a clean separation between `-fbounds-safety` Sema checks and everything else. This will ultimately make exploring the code easier once a significant amount of our implementation has been upstreamed.
* Avoid growing all the existing `Sema*.cpp` files unnecessarily. They are already huge and we don't want to make the problem worse when there's an easy option available to avoid that.


> A bigger picture this fits in would be a compelling argument (like it was for small backend parts), but to my knowledge there's none.

The `counted_by` attribute is just one of several attributes that are part of the `-fbounds-safety` language extension which is documented here https://clang.llvm.org/docs/BoundsSafety.html. That's "bigger picture" for the feature, or did you mean something else by "bigger picture this fits in"?

> Coming up with one and getting maintainers on board is certainly out of scope of this PR, so don't feel obligated to do that. Hopefully I'll get back to this whole topic later.
>

My end goal is to have "somewhere" to put all of the Sema code that support `-fbounds-safety` as we upstream it. I think it's very much preferable to have this code go into its own source file and header file for the reasons I gave above.

We don't have to use the sub-class `SemaBase` mechanism that I've used here if this is really a problem. However, the mechanism does seem like a good fit for `-fbounds-safety` and would be a shame not to use it.

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


More information about the cfe-commits mailing list