================
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;
----------------
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?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1130
@@ -1129,2 +1129,3 @@
+bool Sema::isValidPointerAttrType(QualType T) {
T = T.getNonReferenceType();
----------------
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!)
================
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);
----------------
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>();
http://reviews.llvm.org/D4601
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
