Page MenuHomePhabricator

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

Authored by lebedev.ri on Jan 19 2020, 3:53 AM.

Details

Summary

alloc_align attribute takes parameter number, not the alignment itself,
so given just the attribute/function declaration we can't do any
sanity checking for said alignment.

However, at call site, given the actual Expr that is passed
into that parameter, we might be able to evaluate said Expr
as Integer Constant Expression, and perform the sanity checks.
But since there is no requirement for that argument to be an immediate,
we may fail, and that's okay.

However if we did evaluate, we should enforce the same constraints
as with __builtin_assume_aligned()/__attribute__((assume_aligned(imm))):
said alignment is a power of two, and is not greater than our magic threshold

Diff Detail

Event Timeline

lebedev.ri created this revision.Jan 19 2020, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2020, 3:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lebedev.ri removed a project: Restricted Project.Jan 19 2020, 3:59 AM
lebedev.ri removed a subscriber: llvm-commits.
erichkeane added inline comments.Jan 21 2020, 7:50 AM
clang/lib/Sema/SemaChecking.cpp
3659

Does this need to be isInstantionDependent?

3669

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

jdoerfert added inline comments.Jan 21 2020, 12:11 PM
clang/lib/Sema/SemaChecking.cpp
3669

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

erichkeane added inline comments.Jan 21 2020, 1:18 PM
clang/lib/Sema/SemaChecking.cpp
3669

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.

lebedev.ri marked an inline comment as done.Jan 21 2020, 1:22 PM
lebedev.ri added inline comments.
clang/lib/Sema/SemaChecking.cpp
3659

I'm not sure, should it be?
It seems there is no such check in Sema::SemaBuiltinAssumeAligned().

jdoerfert added inline comments.Jan 21 2020, 3:06 PM
clang/lib/Sema/SemaChecking.cpp
3669

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

lebedev.ri marked 3 inline comments as done.Jan 21 2020, 3:12 PM
hfinkel added inline comments.Jan 21 2020, 3:59 PM
clang/lib/Sema/SemaChecking.cpp
3669

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.

jdoerfert added inline comments.Jan 21 2020, 5:26 PM
clang/lib/Sema/SemaChecking.cpp
3669
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.

hfinkel added inline comments.Jan 21 2020, 6:22 PM
clang/lib/Sema/SemaChecking.cpp
3669

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

lebedev.ri marked 3 inline comments as done.Jan 22 2020, 12:00 AM
jdoerfert added inline comments.Jan 22 2020, 8:25 AM
clang/lib/Sema/SemaChecking.cpp
3669

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.

lebedev.ri marked 4 inline comments as done.

isValueDependent() already implies isTypeDependent(), don't recheck it.

clang/lib/Sema/SemaChecking.cpp
3659

As disscussed in IRC, that's https://clang.llvm.org/doxygen/classclang_1_1Expr.html#a954398acd24dedcfc181659f0b906228 clang::Expr::isInstantiationDependent(), i think this is consistent with other places,
and *might* be correct. Or other places are incorrect too.
It isn't obvious to me how to produce a testcase, so this might be better left as-is.

erichkeane accepted this revision.Jan 23 2020, 9:16 AM
This revision is now accepted and ready to land.Jan 23 2020, 9:16 AM

Thank you for the review!

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Jan 23 2020, 12:11 PM

Seeing some bot failures, reverted this one.

This revision is now accepted and ready to land.Jan 23 2020, 12:11 PM
This revision was automatically updated to reflect the committed changes.