[PATCH] D23657: Remove some false positives when taking the address of packed members

[Richard Smith via cfe-commits]

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