[PATCH] D77244: [part 1] sancov/inline-bool-flag instrumentation.

[Pratyai Mazumder via Phabricator via llvm-commits]

pratyai added a comment.

In D77244#1970820 <https://reviews.llvm.org/D77244#1970820>, @vitalybuka wrote:

> In D77244#1956930 <https://reviews.llvm.org/D77244#1956930>, @pratyai wrote:
>
> > It looks like I broke the tests after the `i8 `-> `i1` switch.
> >
> > I think it's because of an existing bug. From https://llvm.org/docs/LangRef.html
> >
> > > i1:8:8 - i1 is 8-bit (byte) aligned
> >
> > OTOH, in `SanitizerCoverage.cpp`, we have in `CreateFunctionLocalArrayInSection()`:
> >
> >   Array->setAlignment(Align(Ty->isPointerTy()
> >                                ? DL->getPointerSize()
> >                                : Ty->getPrimitiveSizeInBits() / 8));
> >   
> >
> > IIUC `getPrimitiveSizeInBits() / 8` would be `1 / 8 => 0` for `i1` type (it works for other `int` types which have multiple-of-8 bits.
> >
> > PLMK if my assessment is correct, and if so if I should fix it in a separate patch, or just keep that in here.
> >
> > I'll leave this patch "un-split" for now.
> >
> > Thanks!
>
>
> Looks like a bug. 
>  Separate patch would be nice. If it goes before this one we can expect NOP.
>  However, if you don't have committer access, I am going to submit this for you, and I can split the patch myself.


That'd be cool, thanks!



================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:922
+    auto Store = IRB.CreateStore(ConstantInt::getTrue(Int1Ty), FlagPtr);
+    Store->setAtomic(AtomicOrdering::NotAtomic);
+    Store->setAlignment(llvm::MaybeAlign(FunctionBoolArray->getAlign()));
----------------
vitalybuka wrote:
> Store->setAtomic(AtomicOrdering::NotAtomic); should be defaul
Right; dropping.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:923
+    Store->setAtomic(AtomicOrdering::NotAtomic);
+    Store->setAlignment(llvm::MaybeAlign(FunctionBoolArray->getAlign()));
+    SetNoSanitizeMetadata(Store);
----------------
vitalybuka wrote:
> isn't alignment is always 1 which is already default?
> 
Right; the default is NoneType::None, which is already 1. Dropping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77244