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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

In D92728#2437506 <https://reviews.llvm.org/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