[OpaquePointers][ThreadSanitizer] Cleanup calls to PointerType::getElementType()

[dblaikie]

Is this able to be tested with an opaque pointer IR input to run this pass?

[aeubanks]

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

[dblaikie]

In D105710#2867596, @aeubanks wrote:

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.

[nikic]

In D105710#2867611, @dblaikie wrote:

In D105710#2867596, @aeubanks wrote:

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.

My 2c here is that we mainly need to test cases where we're doing something non-obvious for opaque pointers. E.g. D105398 uses a different code path for opaque pointers, so that's worth testing. Here we use the same code and only adjust the APIs used.

Main problem with testing these is that making the code change is easy without any domain knowledge about the pass, while writing tests for it requires you to actually figure out what it is supposed to be doing and how you can trigger the relevant code paths.

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

617–618

Could you getLoadStorePointerOperand() here.

[dblaikie]

In D105710#2867842, @nikic wrote:

In D105710#2867611, @dblaikie wrote:

In D105710#2867596, @aeubanks wrote:

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.

My 2c here is that we mainly need to test cases where we're doing something non-obvious for opaque pointers. E.g. D105398 uses a different code path for opaque pointers, so that's worth testing. Here we use the same code and only adjust the APIs used.

But we did so for a reason - to enable some functionality that wasn't enabled before - and we haven't tested that we successfully did what we intended to do.

Main problem with testing these is that making the code change is easy without any domain knowledge about the pass, while writing tests for it requires you to actually figure out what it is supposed to be doing and how you can trigger the relevant code paths.

Yeah, it's not cheap, no doubt. My usual approach/suggestion would be to put an "assert(false)" in the code being changed, run check-llvm and see which tests fail, pick a fairly simple one and try the same test with opaque pointers.

Alternatively, maybe starting from the tests rather than the code would be good at this point - running chunks of the test suite with opaque pointers forced on, seeing what fails and fixing them - then it's easy to know what testing to add because it started with a failing test case.

[aeubanks]

I still think it's not worth it to find some test that covers this case.

Getting rid of calls to PointerType::getElementType() in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.

IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.

[dblaikie]

In D105710#2872101, @aeubanks wrote:

I still think it's not worth it to find some test that covers this case.

Getting rid of calls to PointerType::getElementType() in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.

IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.

I feel pretty strongly against (on principle - if it's testable, it should be tested - the change isn't NFC now that we have opaque pointer types), but won't hold this up on that.

Might be worth an assert in getMemoryAccessFuncIndex that we've tracked the pointer element type correctly (assert isOpaqueOrPointeeTypeMatches) to demonstrate that the type hasn't changed, we're just tracking it differently.

[Though, OK, I'm a bit hung up on this: Would it be terribly expensive to take an existing test and add a new RUN line to run it with opaque pointers & then it'd be clear that these changes were adequate/necessary/correct to make this code work correctly with opaque pointers]

[aeubanks]

add assert

[dblaikie]

In D105710#2872325, @dblaikie wrote:

In D105710#2872101, @aeubanks wrote:

I still think it's not worth it to find some test that covers this case.

Getting rid of calls to PointerType::getElementType() in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.

IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.

I feel pretty strongly against (on principle - if it's testable, it should be tested - the change isn't NFC now that we have opaque pointer types), but won't hold this up on that.

Might be worth an assert in getMemoryAccessFuncIndex that we've tracked the pointer element type correctly (assert isOpaqueOrPointeeTypeMatches) to demonstrate that the type hasn't changed, we're just tracking it differently.

[Though, OK, I'm a bit hung up on this: Would it be terribly expensive to take an existing test and add a new RUN line to run it with opaque pointers & then it'd be clear that these changes were adequate/necessary/correct to make this code work correctly with opaque pointers]

Any further thoughts on this ^ ? Adding testing in other cases has been possible/practical, such as D105398

[aeubanks]

In D105710#2874527, @dblaikie wrote:

In D105710#2872325, @dblaikie wrote:

In D105710#2872101, @aeubanks wrote:

I still think it's not worth it to find some test that covers this case.

Getting rid of calls to PointerType::getElementType() in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.

IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.

I feel pretty strongly against (on principle - if it's testable, it should be tested - the change isn't NFC now that we have opaque pointer types), but won't hold this up on that.

Might be worth an assert in getMemoryAccessFuncIndex that we've tracked the pointer element type correctly (assert isOpaqueOrPointeeTypeMatches) to demonstrate that the type hasn't changed, we're just tracking it differently.

[Though, OK, I'm a bit hung up on this: Would it be terribly expensive to take an existing test and add a new RUN line to run it with opaque pointers & then it'd be clear that these changes were adequate/necessary/correct to make this code work correctly with opaque pointers]

Any further thoughts on this ^ ? Adding testing in other cases has been possible/practical, such as D105398

I agree that D105398 requires testing because it's actually doing something different depending on whether or not we're looking at opaque pointers, so we need to test the new branch being added.

IMO there are a couple ways to fix issues that pop up due to opaque pointers. One is to run check-llvm with --force-opaque-pointers on and look at/fix crashes. This is probably the most "proper" way to go about this. But I think a faster way is to first go around cleaning up calls to getElementType(), since a vast majority of those will be issues that will pop up later. Even if not all of these are real issues, it's still a quick way to clean up a significant amount of the crashes we'd see later. Either way, in the end we'll be running check-llvm with --force-opaque-pointers and get the same test coverage. But cleaning up with easy fixes to remove getElementType() makes things go faster to begin with.

Perhaps the more involved fixes that take a while to fix should have tests. But for easy/obvious things I'd like to get them out of the way quickly.

[dblaikie]

In D105710#2874705, @aeubanks wrote:

In D105710#2874527, @dblaikie wrote:

In D105710#2872325, @dblaikie wrote:

In D105710#2872101, @aeubanks wrote:

I still think it's not worth it to find some test that covers this case.

Getting rid of calls to PointerType::getElementType() in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.

IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.

I feel pretty strongly against (on principle - if it's testable, it should be tested - the change isn't NFC now that we have opaque pointer types), but won't hold this up on that.

Might be worth an assert in getMemoryAccessFuncIndex that we've tracked the pointer element type correctly (assert isOpaqueOrPointeeTypeMatches) to demonstrate that the type hasn't changed, we're just tracking it differently.

[Though, OK, I'm a bit hung up on this: Would it be terribly expensive to take an existing test and add a new RUN line to run it with opaque pointers & then it'd be clear that these changes were adequate/necessary/correct to make this code work correctly with opaque pointers]

Any further thoughts on this ^ ? Adding testing in other cases has been possible/practical, such as D105398

I agree that D105398 requires testing because it's actually doing something different depending on whether or not we're looking at opaque pointers, so we need to test the new branch being added.

IMO there are a couple ways to fix issues that pop up due to opaque pointers. One is to run check-llvm with --force-opaque-pointers on and look at/fix crashes. This is probably the most "proper" way to go about this. But I think a faster way is to first go around cleaning up calls to getElementType(), since a vast majority of those will be issues that will pop up later. Even if not all of these are real issues, it's still a quick way to clean up a significant amount of the crashes we'd see later. Either way, in the end we'll be running check-llvm with --force-opaque-pointers and get the same test coverage. But cleaning up with easy fixes to remove getElementType() makes things go faster to begin with.

Perhaps the more involved fixes that take a while to fix should have tests. But for easy/obvious things I'd like to get them out of the way quickly.

I thought of an alternative, though may require differently more work: What if we had a green/red list of test directories that pass with --force-opaque-pointers and a buildbot that ran in this configuration, but only alerted for whole directories that were meant to be green that contained failing tests? Then we could update the green list when fixes are made that make a difference, ensuring the fixes are correct/effective, validated, and don't regress?

It'd maybe mean part-way between the pure source based solution, and the test based solution - because you could still identify issues by pure source inspection, but I think then expand it to "what else in this transform/tool/use case isn't able to handle opaque pointers" until there's at least enough to make some test change to green?

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Analysis/CaptureTracking.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/ProfileData/InstrProf.h"
private:
   void initialize(Module &M);
   bool instrumentLoadOrStore(const InstructionInfo &II, const DataLayout &DL);
   bool instrumentAtomic(Instruction *I, const DataLayout &DL);
   bool instrumentMemIntrinsic(Instruction *I);
   void chooseInstructionsToInstrument(SmallVectorImpl<Instruction *> &Local,
                                       SmallVectorImpl<InstructionInfo> &All,
                                       const DataLayout &DL);
   bool addrPointsToConstantData(Value *Addr);
-  int getMemoryAccessFuncIndex(Value *Addr, const DataLayout &DL);
+  int getMemoryAccessFuncIndex(Type *OrigTy, Value *Addr, const DataLayout &DL);
   void InsertRuntimeIgnores(Function &F);
 
   Type *IntptrTy;
   FunctionCallee TsanFuncEntry;
   FunctionCallee TsanFuncExit;
   FunctionCallee TsanIgnoreBegin;
   FunctionCallee TsanIgnoreEnd;
   // Accesses sizes are powers of two: 1, 2, 4, 8, 16.
bool ThreadSanitizer::sanitizeFunction(Function &F,
   return Res;
 }
 
 bool ThreadSanitizer::instrumentLoadOrStore(const InstructionInfo &II,
                                             const DataLayout &DL) {
   IRBuilder<> IRB(II.Inst);
   const bool IsWrite = isa<StoreInst>(*II.Inst);
   Value *Addr = IsWrite ? cast<StoreInst>(II.Inst)->getPointerOperand()
                         : cast<LoadInst>(II.Inst)->getPointerOperand();
+  Type *OrigTy = getLoadStoreType(II.Inst);
 
   // swifterror memory addresses are mem2reg promoted by instruction selection.
   // As such they cannot have regular uses like an instrumentation function and
   // it makes no sense to track them as memory.
   if (Addr->isSwiftError())
     return false;
 
-  int Idx = getMemoryAccessFuncIndex(Addr, DL);
+  int Idx = getMemoryAccessFuncIndex(OrigTy, Addr, DL);
   if (Idx < 0)
     return false;
   if (IsWrite && isVtableAccess(II.Inst)) {
     LLVM_DEBUG(dbgs() << "  VPTR : " << *II.Inst << "\n");
     Value *StoredValue = cast<StoreInst>(II.Inst)->getValueOperand();
     // StoredValue may be a vector type if we are storing several vptrs at once.
     // In this case, just take the first element of the vector since this is
     // enough to find vptr races.
const unsigned Alignment = IsWrite ? cast<StoreInst>(II.Inst)->getAlignment()
                                      : cast<LoadInst>(II.Inst)->getAlignment();
   const bool IsCompoundRW =
       ClCompoundReadBeforeWrite && (II.Flags & InstructionInfo::kCompoundRW);
   const bool IsVolatile = ClDistinguishVolatile &&
                           (IsWrite ? cast<StoreInst>(II.Inst)->isVolatile()
                                    : cast<LoadInst>(II.Inst)->isVolatile());
   assert((!IsVolatile || !IsCompoundRW) && "Compound volatile invalid!");
 
-  Type *OrigTy = cast<PointerType>(Addr->getType())->getElementType();
   const uint32_t TypeSize = DL.getTypeStoreSizeInBits(OrigTy);
   FunctionCallee OnAccessFunc = nullptr;
   if (Alignment == 0 || Alignment >= 8 || (Alignment % (TypeSize / 8)) == 0) {
     if (IsCompoundRW)
       OnAccessFunc = TsanCompoundRW[Idx];
     else if (IsVolatile)
       OnAccessFunc = IsWrite ? TsanVolatileWrite[Idx] : TsanVolatileRead[Idx];
     else

 // http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf
 // The following page contains more background information:
 // http://www.hpl.hp.com/personal/Hans_Boehm/c++mm/
 
 bool ThreadSanitizer::instrumentAtomic(Instruction *I, const DataLayout &DL) {
   IRBuilder<> IRB(I);
   if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
     Value *Addr = LI->getPointerOperand();
-    int Idx = getMemoryAccessFuncIndex(Addr, DL);
+    Type *OrigTy = LI->getType();
+    int Idx = getMemoryAccessFuncIndex(OrigTy, Addr, DL);
     if (Idx < 0)
       return false;
     const unsigned ByteSize = 1U << Idx;
     const unsigned BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      createOrdering(&IRB, LI->getOrdering())};
-    Type *OrigTy = cast<PointerType>(Addr->getType())->getElementType();
     Value *C = IRB.CreateCall(TsanAtomicLoad[Idx], Args);
     Value *Cast = IRB.CreateBitOrPointerCast(C, OrigTy);
     I->replaceAllUsesWith(Cast);
   } else if (StoreInst *SI = dyn_cast<StoreInst>(I)) {
     Value *Addr = SI->getPointerOperand();
-    int Idx = getMemoryAccessFuncIndex(Addr, DL);
+    int Idx =
+        getMemoryAccessFuncIndex(SI->getValueOperand()->getType(), Addr, DL);
     if (Idx < 0)
       return false;
     const unsigned ByteSize = 1U << Idx;
     const unsigned BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      IRB.CreateBitOrPointerCast(SI->getValueOperand(), Ty),
                      createOrdering(&IRB, SI->getOrdering())};
     CallInst *C = CallInst::Create(TsanAtomicStore[Idx], Args);
     ReplaceInstWithInst(I, C);
   } else if (AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I)) {
     Value *Addr = RMWI->getPointerOperand();
-    int Idx = getMemoryAccessFuncIndex(Addr, DL);
+    int Idx =
+        getMemoryAccessFuncIndex(RMWI->getValOperand()->getType(), Addr, DL);
     if (Idx < 0)
       return false;
     FunctionCallee F = TsanAtomicRMW[RMWI->getOperation()][Idx];
     if (!F)
       return false;
     const unsigned ByteSize = 1U << Idx;
     const unsigned BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      IRB.CreateIntCast(RMWI->getValOperand(), Ty, false),
                      createOrdering(&IRB, RMWI->getOrdering())};
     CallInst *C = CallInst::Create(F, Args);
     ReplaceInstWithInst(I, C);
   } else if (AtomicCmpXchgInst *CASI = dyn_cast<AtomicCmpXchgInst>(I)) {
     Value *Addr = CASI->getPointerOperand();
-    int Idx = getMemoryAccessFuncIndex(Addr, DL);
+    Type *OrigOldValTy = CASI->getNewValOperand()->getType();
+    int Idx = getMemoryAccessFuncIndex(OrigOldValTy, Addr, DL);
     if (Idx < 0)
       return false;
     const unsigned ByteSize = 1U << Idx;
     const unsigned BitSize = ByteSize * 8;
     Type *Ty = Type::getIntNTy(IRB.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     Value *CmpOperand =
       IRB.CreateBitOrPointerCast(CASI->getCompareOperand(), Ty);
     Value *NewOperand =
       IRB.CreateBitOrPointerCast(CASI->getNewValOperand(), Ty);
     Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                      CmpOperand,
                      NewOperand,
                      createOrdering(&IRB, CASI->getSuccessOrdering()),
                      createOrdering(&IRB, CASI->getFailureOrdering())};
     CallInst *C = IRB.CreateCall(TsanAtomicCAS[Idx], Args);
     Value *Success = IRB.CreateICmpEQ(C, CmpOperand);
     Value *OldVal = C;
-    Type *OrigOldValTy = CASI->getNewValOperand()->getType();
     if (Ty != OrigOldValTy) {
       // The value is a pointer, so we need to cast the return value.
       OldVal = IRB.CreateIntToPtr(C, OrigOldValTy);
     }
 
     Value *Res =
       IRB.CreateInsertValue(UndefValue::get(CASI->getType()), OldVal, 0);
     Res = IRB.CreateInsertValue(Res, Success, 1);
 
     I->replaceAllUsesWith(Res);
     I->eraseFromParent();
   } else if (FenceInst *FI = dyn_cast<FenceInst>(I)) {
     Value *Args[] = {createOrdering(&IRB, FI->getOrdering())};
     FunctionCallee F = FI->getSyncScopeID() == SyncScope::SingleThread
                            ? TsanAtomicSignalFence
                            : TsanAtomicThreadFence;
     CallInst *C = CallInst::Create(F, Args);
     ReplaceInstWithInst(I, C);
   }
   return true;
 }
 
-int ThreadSanitizer::getMemoryAccessFuncIndex(Value *Addr,
+int ThreadSanitizer::getMemoryAccessFuncIndex(Type *OrigTy, Value *Addr,
                                               const DataLayout &DL) {
-  Type *OrigPtrTy = Addr->getType();
-  Type *OrigTy = cast<PointerType>(OrigPtrTy)->getElementType();
   assert(OrigTy->isSized());
+  assert(
+      cast<PointerType>(Addr->getType())->isOpaqueOrPointeeTypeMatches(OrigTy));
   uint32_t TypeSize = DL.getTypeStoreSizeInBits(OrigTy);
   if (TypeSize != 8  && TypeSize != 16 &&
       TypeSize != 32 && TypeSize != 64 && TypeSize != 128) {
     NumAccessesWithBadSize++;
     // Ignore all unusual sizes.
     return -1;
   }
   size_t Idx = countTrailingZeros(TypeSize / 8);
   assert(Idx < kNumberOfAccessSizes);
   return Idx;
 }