ahatanak added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13122
+ if (Base->isVirtual()) {
+ BaseAlignment = Ctx.getTypeAlignInChars(Base->getType());
+ Offset = CharUnits::Zero();
----------------
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`).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78767/new/
https://reviews.llvm.org/D78767
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
