[NFC][MSan] Round up OffsetPtr in PoisonMembers

[vitalybuka]

Summary

getFieldOffset(layoutStartOffset) is expected to point to the first trivial
field or the one which follows non-trivial. So it must be byte aligned already.
However this is not obvious without assumptions about callers.
This patch will avoid the need in such assumptions.

Depends on D92727.

[morehouse]

clang/lib/CodeGen/CGClass.cpp

1742

How does this interact with bit fields?

[eugenis]

Don't you want to similarly align down PoisonEnd?

But if this is something that should never happen, as your comment rightly suggests, wouldn't it be better to add an assert()?
The same in the case when PoisonSize < 0 - it should never happen.

[vitalybuka]

In D92728#2437506, @eugenis wrote:

Don't you want to similarly align down PoisonEnd?

But if this is something that should never happen, as your comment rightly suggests, wouldn't it be better to add an assert()?
The same in the case when PoisonSize < 0 - it should never happen.

clang/lib/CodeGen/CGClass.cpp

1742

I guess existing code work because is bit fields are trivial and the one aligned to the char is going to be layoutStartOffset
I assume if we call this function for layoutStartOffset pointing to the bitfield in the middle of char it will poison entire byte which is incorrect.
With the patch it will not poison such chars: now it will be either all bits or nothing which I believe better.

1742

toCharUnitsFromBits already down aligns by definition: it's just a bitsoffset/charwidth

regarding assert: I am not 100% that this is not possible, maybe some corner-case in language allows that.
if so, it's better not to poison partial byte at all than poison it completely.

[eugenis]

LGTM

clang/lib/CodeGen/CGClass.cpp

1742

I assume if we call this function for layoutStartOffset pointing to the bitfield in the middle of char it will poison entire byte which is incorrect.

Right. We never do this, of course, because the previous non-trivial field can not possibly end in the middle of a char.

clang/lib/CodeGen/CGClass.cpp

 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "TargetInfo.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "llvm/IR/Intrinsics.h"
public:
     }
 
   private:
     /// \param layoutStartOffset index of the ASTRecordLayout field to
     ///     start poisoning (inclusive)
     /// \param layoutEndOffset index of the ASTRecordLayout field to
     ///     end poisoning (exclusive)
     void PoisonMembers(CodeGenFunction &CGF, unsigned layoutStartOffset,
                        unsigned layoutEndOffset) {
       ASTContext &Context = CGF.getContext();
       const ASTRecordLayout &Layout =
           Context.getASTRecordLayout(Dtor->getParent());
 
-      llvm::ConstantInt *OffsetSizePtr = llvm::ConstantInt::get(
-          CGF.SizeTy,
-          Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset))
-              .getQuantity());
+      // It's a first trivia field so it should be at the begining of char,
+      // still round up start offset just in case.
+      CharUnits PoisonStart =
+          Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset) +
+                                      Context.getCharWidth() - 1);
+      llvm::ConstantInt *OffsetSizePtr =
+          llvm::ConstantInt::get(CGF.SizeTy, PoisonStart.getQuantity());
 
       llvm::Value *OffsetPtr = CGF.Builder.CreateGEP(
           CGF.Builder.CreateBitCast(CGF.LoadCXXThis(), CGF.Int8PtrTy),
           OffsetSizePtr);
 
-      CharUnits::QuantityType PoisonSize;
+      CharUnits PoisonEnd;
       if (layoutEndOffset >= Layout.getFieldCount()) {
-        PoisonSize = Layout.getNonVirtualSize().getQuantity() -
-                     Context.toCharUnitsFromBits(
-                                Layout.getFieldOffset(layoutStartOffset))
-                         .getQuantity();
+        PoisonEnd = Layout.getNonVirtualSize();
       } else {
-        PoisonSize = Context.toCharUnitsFromBits(
-                                Layout.getFieldOffset(layoutEndOffset) -
-                                Layout.getFieldOffset(layoutStartOffset))
-                         .getQuantity();
+        PoisonEnd =
+            Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutEndOffset));
       }
-
-      if (PoisonSize == 0)
+      CharUnits PoisonSize = PoisonEnd - PoisonStart;
+      if (!PoisonSize.isPositive())
         return;
 
-      EmitSanitizerDtorCallback(CGF, OffsetPtr, PoisonSize);
+      EmitSanitizerDtorCallback(CGF, OffsetPtr, PoisonSize.getQuantity());
     }
   };
 
  class SanitizeDtorVTable final : public EHScopeStack::Cleanup {
     const CXXDestructorDecl *Dtor;
 
   public:
     SanitizeDtorVTable(const CXXDestructorDecl *Dtor) : Dtor(Dtor) {}