[PATCH] D23657: Remove some false positives when taking the address of packed members
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 3 15:48:20 PDT 2016
rsmith added inline comments.
> SemaChecking.cpp:11053-11056
> + if ((RequiredAlignment > AlignRecord) ||
> + ((Context.toCharUnitsFromBits(
> + Context.getFieldOffset(cast<FieldDecl>(MD))) %
> + RequiredAlignment) != 0)) {
This doesn't seem to correctly handle the case where the base expression gives you some guaranteed alignment; it only handles the case where the base expression reduces the alignment. Suppose `__alignof__(double) == 8` and I have this:
struct __attribute__((packed, aligned(1))) B { double d; };
struct __attribute__((aligned(8))) A { B b; };
A *p;
double *q = &p->b.d;
For the top `MemberExpr` here, we see `RequiredAlignment == AlignField == 8`, `AlignRecord == 1`, and we report that we have a reference to a field with reduced alignment. But we don't, because the reference is within an 8-byte-aligned `B` object within the enclosing `A` object.
Do you care about handling that case correctly? It seems like one way to handle all the nontrivial cases here would be to recurse to the innermost `MemberExpr`, accumulating an offset on the way, and then check that the innermost expression's type implies a correct alignment and offset for the type of `E`.
> SemaChecking.cpp:11069-11071
> + // If this field already has the least alignment possible it will never
> + // yield a misaligned pointer.
> + break;
Too much indentation.
> SemaChecking.cpp:11072
> + break;
> ME = dyn_cast<MemberExpr>(ME->getBase());
> }
You should at least `IgnoreParens` here, and maybe `IgnoreParenNoopCasts`.
https://reviews.llvm.org/D23657
More information about the cfe-commits
mailing list