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

[Akira Hatanaka via Phabricator via cfe-commits]

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:
> 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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13266
+      if (!RHS->isIntegerConstantExpr(RHSRes, Ctx))
+        return Optional<std::pair<CharUnits, CharUnits>>();
+      CharUnits Offset = LHSRes->second;
----------------
rjmccall wrote:
> This should be handled the same as array subscripting.  Maybe you can extract a helper for that that's given a pointer expression, an integer expression, and a flag indicating whether it's a subtraction?
The offset computation was wrong when the element type wasn't a char. The bug is fixed in the helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78767