[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:
> hfinkel wrote:
> > 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.
> > >     For OpenMP we look into LLVM during SEMA
> > How do we do that?
> 
> I was referring to code like this https://reviews.llvm.org/D71830#C1739755NL11085 
> which is in CodeGen right now but has to move to SemaOverload. The code is completely reusable between Clang and Flang so I put it in lib/Frontend/OpenMP and I think that is the right place for it.
> 
> > 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.
> 
> 
> I get that in a general sense. For the problem at hand, and as far as I known, the restriction stems only from the LLVM-IR restriction, correct? If so, what is the argument for separation? I mean, a change of the value in LLVM might directly impact Clang behavior.
> 
> I could also see us clamping the alignment during codegen. While that might have other problems they seem less practical to me.
> 
> 
> 
> 
> I was referring to code like this https://reviews.llvm.org/D71830#C1739755NL11085
> which is in CodeGen right now but has to move to SemaOverload. The code is completely reusable between Clang and Flang so I put it in lib/Frontend/OpenMP and I think that is the right place for it.

Fair, but that's a library designed to be a home for cross-language frontend components. The variant-selection logic to which you're referring, itself, does not actually need to link to LLVM's IR library, correct?

> I get that in a general sense. For the problem at hand, and as far as I known, the restriction stems only from the LLVM-IR restriction, correct? If so, what is the argument for separation? I mean, a change of the value in LLVM might directly impact Clang behavior.

Yes, I believe that the restriction is necessary because of an underlying LLVM IR restriction. From my perspective, your argument is perfectly rational. Clang only supports code generation using LLVM IR, and a restriction that comes from LLVM should be directly tied to the underlying LLVM threshold regardless of where it is surfaced. We have, however, avoided a linking dependence (I believe, primarily, to help the load times and file sizes of tools based on Clang which don't otherwise need to link to the LLVM IR libraries).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72996