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

[Johannes Doerfert via Phabricator via cfe-commits]

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)
----------------
hfinkel wrote:
> 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).
> The variant-selection logic to which you're referring, itself, does not actually need to link to LLVM's IR library, correct?

Correct.

> 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).

That makes total sense to me. The idea of a separate header for magic constants, e.g., `llvm/include/llvm/IR/IRConstants.h` or `llvm/include/llvm/Frontend/Constants.h`, was to prevent any linking issues. Anyway, if people feel having the constant defined twice is better for now I won't object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72996