[PATCH] D72996: [Sema] Attempt to perform call-size-specific `__attribute__((alloc_align(param_idx)))` validation

Johannes Doerfert via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 21 15:09:29 PST 2020


jdoerfert added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:4489
+        // Alignment calculations can wrap around if it's greater than 2**29.
+        unsigned MaximumAlignment = 536870912;
+        if (I > MaximumAlignment)
----------------
erichkeane wrote:
> jdoerfert wrote:
> > erichkeane wrote:
> > > I thought we had this stored somewhere else?  We probably should have this be a constant somewhere in the frontend.  I THINK I remember doing a review where I pulled this value into clang somewhere...
> > That was D72998, and I don't think Clang is the right place for this constant. It is a property of the llvm alignment attribute and it should live there. Thus, llvm/include/Attributes.h or some similar place. Can't we "fix" the linker error by making it a constexpr global or are the errors because of other file content? If the latter, we could go with a llvm/include/magic_constants.h ;)
> The one I was thinking of was this one: https://reviews.llvm.org/D68824
> 
> I don't remember what we came up with on the linking issue.  It would be really nice if it was just something included from LLVM, but I think SEMA typically doesn't include stuff from LLVM either.
I'm not too happy with the duplication of the constant but defining it once in clang is certainly better than having it in N places. For OpenMP we look into LLVM during SEMA and here there is an argument to be made that we should as well. I imagine more cases would pop up over time.

FWIW, if we allow to include LLVM headers, e.g., from IR or Frontend, we could still have a wrapper in SEMA to get the information so it doesn't expose the llvm:: namespace at the use sides (if that helps).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72996/new/

https://reviews.llvm.org/D72996





More information about the cfe-commits mailing list