[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:13122
+    if (Base->isVirtual()) {
+      BaseAlignment = Ctx.getTypeAlignInChars(Base->getType());
+      Offset = CharUnits::Zero();
----------------
ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Oh, this — and all the other places that do presumed alignment based on a pointee type — needs a special case for C++ records with virtual bases, because you need to get its presumed alignment as a base sub-object, not its presumed alignment as a complete object, which is what `getTypeAlignInChars` will return.  The right way to merge that information is to get the normal alignment — which may be lower than expected if there's a typedef in play — and then `min` that with the base sub-object alignment.
> > > I think the base sub-object alignment in this case is the `NonVirtualAlignment` of the class (the `CXXRecordDecl` of `Base` in this function), but it's not clear to me what the normal alignment is. I don't think it's what `Ctx.getTypeAlignInChars(Base->getType())` returns, is it? I see `CodeGenModule::getDynamicOffsetAlignment` seems to be doing what you are suggesting, but I'm not sure whether that's what we should be doing here. It looks like it's comparing the alignment of the derived class and the non-virtual alignment of the base class.
> > The base sub-object alignment is the class's non-virtual alignment, right.
> > 
> > By the normal alignment, I mean the alignment you're starting from for the outer object — if that's less than the base's alignment, the base may be misaligned as well.  For the non-base-conversion case, that's probably not necessary to consider.
> > 
> Let me know if the comment explaining why we are taking the minimum makes sense. I added a test case for this too (`&t10`).
Looks good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78767/new/

https://reviews.llvm.org/D78767