================
Comment at: lib/CodeGen/CGExpr.cpp:3379-3382
@@ -3378,2 +3378,6 @@
- return EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+ RValue Ret = EmitCall(FnInfo, Callee, ReturnValue, Args, TargetDecl);
+
+ if (Ret.isScalar() && TargetDecl) {
+ if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
+ llvm::Value *OffsetValue = nullptr;
----------------
rsmith wrote:
> hfinkel wrote:
> > rsmith wrote:
> > > Is this really the right place for this code, rather than in the called
> > > `EmitCall` function? Various places call the lower-level overload
> > > directly.
> > When I first looked, I thought that there were no other relevant callers,
> > but maybe EmitCallAndReturnForThunk and EmitForwardingCallToLambda would
> > need this handling too?
> >
> > Because of the way that the lower-level EmitCall is structured, I'd need to
> > wrap it (it has too many early returns). What would I call it?
> `EmitCXXMemberOrOperatorCall`, `EmitCXXMemberPointerCallExpr`, and
> `EmitNewDeleteCall` should also get this handling.
>
> Maybe factor out the `switch` that produces the returned `RValue` from the
> call?
I tried using a lambda, let me know what you think.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1130
@@ -1129,2 +1129,3 @@
+bool Sema::isValidPointerAttrType(QualType T) {
T = T.getNonReferenceType();
----------------
rsmith wrote:
> hfinkel wrote:
> > rsmith wrote:
> > > hfinkel wrote:
> > > > rsmith wrote:
> > > > > Is this really appropriate for your attribute? (It makes almost no
> > > > > sense for `__attribute__((nonnull))` either, but I couldn't find
> > > > > anyone at Apple to tell me what the relevant radar says, so I have no
> > > > > idea why we have this "feature".)
> > > > Nice :-) -- Honestly, I don't have a strong opinion regarding whether
> > > > or not we strip references, but I like the idea of consistency between
> > > > the two attributes. The set of returned pointers about which I can
> > > > assert something about the alignment should be the same as that about
> > > > which I can assert a nonnull property.
> > > >
> > > > That having been said, it looks like the CodeGen for returns_nonnull
> > > > that adds the nonnull IR attribute (and the associated UBSan code) does
> > > > not handle references-to-pointers correctly. I'll fix that as a
> > > > follow-up too.
> > > It seems really weird for either of these to look through references to
> > > the referenced pointer. And it's ambiguous: does the attribute apply to
> > > the reference or to the pointer that the reference refers to? (For the
> > > nonnull attribute, GCC and Clang make different choices here, at least in
> > > the cases where you can persuade GCC not to reject the attribute!)
> > Indeed, unlike nonnull, this does become ambiguous. Perhaps it is best to
> > prohibit it?
> >
> > How do Clang and GCC make different choices for nonnull; is that a bug?
> > Because a reference cannot be null, it would seem that it should apply to
> > the pointer being bound.
> Here's what I can piece together:
>
> * GCC does not intend to support `nonnull` on reference parameters. It
> rejects `nonnull(N)` where parameter `N` is a reference, *but* if you apply
> `nonnull` with no `N`, it applies to both pointer and reference parameters,
> and happens to have the semantics of checking for "null references".
> * Clang made up its own thing in response to some non-publicly-visible radar
> issue, where it treats the `nonnull` attribute on references-to-pointers as
> applying to the pointer within the reference. This makes little sense to me,
> but there it is. We naturally can't optimize on the basis of `nonnull` on a
> reference-to-pointer like we can on a real pointer parameter/return value (we
> have no IR representation for that).
>
> It seems our options are to either not support `assume_aligned` on references
> (which would be a shame, because it's natural and useful there just as it is
> for pointers), or make `nonnull` and `assume_aligned` inconsistent on
> pointers-to-references, or remove our (dubious, IMO) extension of the GNU
> `nonnull` semantics. I'm not happy with the third option since I don't know
> what the motivation for the extension is nor how many people might be relying
> on it, but either of the first two seem fine to me. I don't think the
> connection between `nonnull` and `assume_aligned` is strong enough that the
> difference would be jarring for people.
Alright, I'll go with option 1, we'll have it apply to references in the
natural way.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:203-205
@@ +202,5 @@
+ const AssumeAlignedAttr *AssumeAligned =
dyn_cast<AssumeAlignedAttr>(TmplAttr);
+ if (AssumeAligned && (AssumeAligned->getAlignment()->isValueDependent() ||
+ (AssumeAligned->getOffset() &&
+ AssumeAligned->getOffset()->isValueDependent()))) {
+ instantiateDependentAssumeAlignedAttr(*this, TemplateArgs,
AssumeAligned, New);
----------------
rsmith wrote:
> hfinkel wrote:
> > rsmith wrote:
> > > You shouldn't have these 'is dependent' checks here; the other code above
> > > and below is wrong to include them. Testcase:
> > >
> > > template<typename T> __attribute__((assume_aligned(sizeof(int(T())))))
> > > T *f();
> > > void *p = f<void>();
> > >
> > > This should result in a hard error, because `int(void())` is invalid, but
> > > I think you'll accept it, because `sizeof(int(T()))` is not
> > > value-dependent, so you never actually perform the substitution.
> > Interestingly, the current code does generate an error:
> > invalid application of 'sizeof' to a function type
> >
> > Removing the type-dependent checks does not seem to change any of the
> > current behavior.
> Ha, sorry, bitten by a parse ambiguity. Six closing parens wasn't enough;
> let's go up to seven:
>
> template<typename T> __attribute__((assume_aligned(sizeof((int(T()))))))
> T *f();
> void *p = f<void>();
>
Test case added.
http://reviews.llvm.org/D4601
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
