Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
lib/Sema/SemaExpr.cpp
- This file is larger than 256 KB, so syntax highlighting is disabled by default.
| Show First 20 Lines • Show All 10,508 Lines • ▼ Show 20 Lines | QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) { | ||||
| // OpenCL v2.0 s6.12.5 - The unary operators & cannot be used with a block. | // OpenCL v2.0 s6.12.5 - The unary operators & cannot be used with a block. | ||||
| if (getLangOpts().OpenCL && OrigOp.get()->getType()->isBlockPointerType()) { | if (getLangOpts().OpenCL && OrigOp.get()->getType()->isBlockPointerType()) { | ||||
| Diag(OpLoc, diag::err_typecheck_unary_expr) << OrigOp.get()->getType() | Diag(OpLoc, diag::err_typecheck_unary_expr) << OrigOp.get()->getType() | ||||
| << op->getSourceRange(); | << op->getSourceRange(); | ||||
| return QualType(); | return QualType(); | ||||
| } | } | ||||
| // Taking the address of a data member/field of a packed | |||||
| // struct may be a problem if the pointer value is dereferenced. | |||||
aaron.ballman: Missing a full stop at the end of the sentence. | |||||
| Expr *rhs = OrigOp.get(); | |||||
Can use const auto * here instead of repeating the type name. aaron.ballman: Can use `const auto *` here instead of repeating the type name. | |||||
The * binds to the identifier instead of the type; you should run the patch through clang-format (formatting issues are also in the test files). aaron.ballman: The * binds to the identifier instead of the type; you should run the patch through clang… | |||||
| const auto *ME = dyn_cast<MemberExpr>(rhs); | |||||
| while (ME && isa<FieldDecl>(ME->getMemberDecl())) { | |||||
| QualType BaseType = ME->getBase()->getType(); | |||||
| if (ME->isArrow()) | |||||
| BaseType = BaseType->getPointeeType(); | |||||
| RecordDecl *RD = BaseType->getAs<RecordType>()->getDecl(); | |||||
| ValueDecl *MD = ME->getMemberDecl(); | |||||
Not Done ReplyInline ActionsI wonder if we should diagnose only when the resulting pointer is actually misaligned. For instance, the following code should be just fine: struct __attribute__((packed)) S {
int i;
};
void f(S s) {
int *ip = &s.i; // Why should this warn?
}Have you run this patch over any large code bases to see how high the false-positive rate is? How do you envision users silencing this diagnostic if they have a false-positive? Something like &(s.x) (since I you're not ignoring paren imp casts)? aaron.ballman: I wonder if we should diagnose only when the resulting pointer is actually misaligned. For… | |||||
Not Done ReplyInline ActionsI think it should warn because in this case &s == &s.i and the alignment of the whole struct is 1 (due to the packed attribute) so s.i is also aligned to 1 which would not be the suitable alignment for an int. I'm currently building Firefox, I'll add a comment once it ends. Regarding silencing the diagnostic: -Wno-address-of-packed-member should do but I understand it might be too coarse in some cases. rogfer01: I think it should warn because in this case &s == &s.i and the alignment of the whole struct is… | |||||
Ah, I forgot that the attribute also affected the alignment of the struct itself. Amending my false-positive idea, the following should be fine, shouldn't it?: struct __attribute__((packed)) S {
char c;
int i;
char c2;
};
void f(S s) {
char *cp = &s.c;
char *cp2 = &s.c2;
}I think perhaps we should not diagnose if the member's type also has a one-byte alignment (or, for instance, uses alignas(1) to achieve that). What do you think? aaron.ballman: Ah, I forgot that the attribute also affected the alignment of the struct itself. Amending my… | |||||
Yes. I like the idea since the packed attribute has no effect on already 1-aligned data (like char). But note that C++11 does not allow reducing the alignment through alignas. rogfer01: Yes. I like the idea since the packed attribute has no effect on already 1-aligned data (like… | |||||
Not Done ReplyInline ActionsHmm, okay, I was not remembering properly; I thought one of alignas, __declspec(align), or __attribute__((aligned)) allowed for decreasing the alignment, but documentation implies otherwise. aaron.ballman: Hmm, okay, I was not remembering properly; I thought one of `alignas`, `__declspec(align)`, or… | |||||
| bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne(); | |||||
| if (ByteAligned) // Attribute packed does not have any effect. | |||||
Comments should be complete sentences including capitalization and punctuation, so perhaps instead: // Packed does not have any effect. aaron.ballman: Comments should be complete sentences including capitalization and punctuation, so perhaps… | |||||
| break; | |||||
| if (!ByteAligned && | |||||
| (RD->hasAttr<PackedAttr>() || (MD->hasAttr<PackedAttr>()))) { | |||||
| Diag(OpLoc, diag::warn_taking_address_of_packed_member) | |||||
| << MD << RD << rhs->getSourceRange(); | |||||
| break; | |||||
| } | |||||
| ME = dyn_cast<MemberExpr>(ME->getBase()); | |||||
I do not think this diagnostic should have a FixIt hint. This isn't actually *fixing* the problem, it is simply a way to silence the diagnostic while retaining the same behavior. aaron.ballman: I do not think this diagnostic should have a FixIt hint. This isn't actually *fixing* the… | |||||
Not Done ReplyInline ActionsOK. I will remove it. rogfer01: OK. I will remove it. | |||||
| } | |||||
| return Context.getPointerType(op->getType()); | return Context.getPointerType(op->getType()); | ||||
| } | } | ||||
| static void RecordModifiableNonNullParam(Sema &S, const Expr *Exp) { | static void RecordModifiableNonNullParam(Sema &S, const Expr *Exp) { | ||||
| const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp); | const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp); | ||||
| if (!DRE) | if (!DRE) | ||||
| return; | return; | ||||
| const Decl *D = DRE->getDecl(); | const Decl *D = DRE->getDecl(); | ||||
| ▲ Show 20 Lines • Show All 4,497 Lines • Show Last 20 Lines | |||||
Missing a full stop at the end of the sentence.