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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel 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)
----------------
jdoerfert wrote:
> 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).
> For OpenMP we look into LLVM during SEMA 

How do we do that?

There's certainly an interesting philosophical issue around whether changes in LLVM should directly manifest as Clang behavioral changes, especially in -fsyntax-only. The answer to this question might be different for extensions vs. core language features (although alignment restrictions might implicate both). AFAIKT, historically , our answer has been to insist on separation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72996