[PATCH] D92728: [NFC][MSan] Round up OffsetPtr in PoisonMembers

[Vitaly Buka via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6e614b0c7ed3: [NFC][MSan] Round up OffsetPtr in 
PoisonMembers (authored by vitalybuka).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92728

Files:
  clang/lib/CodeGen/CGClass.cpp


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -18,6 +18,7 @@
 #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"
@@ -1729,37 +1730,35 @@
 /// \param layoutEndOffset index of the ASTRecordLayout field to
 /// end poisoning (exclusive)
 void PoisonMembers(CodeGenFunction &CGF, unsigned layoutStartOffset,
- unsigned layoutEndOffset) {
+   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());
 }
   };
 


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -18,6 +18,7 @@
 #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"
@@ -1729,37 +1730,35 @@
 /// \param layoutEndOffset index of the ASTRecordLayout field to
 /// end poisoning (exclusive)
 void PoisonMembers(CodeGenFunction &CGF, unsigned layoutStartOffset,
- unsigned layoutEndOffset) {
+   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 =

[PATCH] D92728: [NFC][MSan] Round up OffsetPtr in PoisonMembers

[Evgenii Stepanov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/CodeGen/CGClass.cpp:1742
+  Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset) 
+
+  Context.getCharWidth() - 1);
+  llvm::ConstantInt *OffsetSizePtr =

vitalybuka wrote:
> vitalybuka wrote:
> > morehouse wrote:
> > > How does this interact with bit fields?
> > 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.
> 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.
> 
> 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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92728

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92728: [NFC][MSan] Round up OffsetPtr in PoisonMembers

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

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.






Comment at: clang/lib/CodeGen/CGClass.cpp:1742
+  Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset) 
+
+  Context.getCharWidth() - 1);
+  llvm::ConstantInt *OffsetSizePtr =

morehouse wrote:
> How does this interact with bit fields?
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.



Comment at: clang/lib/CodeGen/CGClass.cpp:1742
+  Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset) 
+
+  Context.getCharWidth() - 1);
+  llvm::ConstantInt *OffsetSizePtr =

vitalybuka wrote:
> morehouse wrote:
> > How does this interact with bit fields?
> 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.
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92728

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits