Correctly report modified status for HWAddressSanitizer

[serge-sans-paille]

Summary

Related to https://reviews.llvm.org/D80916

[foad]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

919

I don't understand this false. getDynamicShadowNonTls creates IR (unless it returns nullptr).

924

Likewise.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

1086

maybe do not bother with Changed and after this point always return true?

1133–1134

Changed = true?

1161

here and two ifs below?

[serge-sans-paille]

Take remarks into account.

[serge-sans-paille]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

1086

It's possible that AllocasToInstrument ends up being empty, or AllocaToPaddedAllocaMap is empty, or OperandsToInstrument is non empty but with a mask that make all instrumentation fail.

[foad]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

1129–1131

You don't need braces around a single line.

[serge-sans-paille]

Take review into account.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

1158

there are changes below. if we assume Changed is true by this point, then probably we should return true below?
maybe remove Changed and always return true after line 1140

[serge-sans-paille]

Take review into account

[vitalybuka]

HWAddressSanitizer LGTM, but looks like simplifyCFG is here by mistake?

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

2765 ↗

(On Diff #270157)

maybe move next after "IRBuilder<> Builder(PBI);"

[serge-sans-paille]

Remove irrelevant part of the patch to focus on HWAdressSanitizer. Sorry for the noise.

[serge-sans-paille]

@vitalybuka I removed the extra part of the patch, is it ok now?

[vitalybuka]

LGTM

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

Value *HWAddressSanitizer::getHwasanThreadSlotPtr(IRBuilder<> &IRB, Type *Ty) {
 
 
   return nullptr;
 }
 
 void HWAddressSanitizer::emitPrologue(IRBuilder<> &IRB, bool WithFrameRecord) {
   if (!Mapping.InTls) {
     LocalDynamicShadow = getDynamicShadowNonTls(IRB);
     return;
   }
 
   if (!WithFrameRecord && TargetTriple.isAndroid()) {
     LocalDynamicShadow = getDynamicShadowIfunc(IRB);
     return;
   }
 
   Value *SlotPtr = getHwasanThreadSlotPtr(IRB, IntptrTy);
   assert(SlotPtr);
 
   Value *ThreadLong = IRB.CreateLoad(IntptrTy, SlotPtr);
   // Extract the address field from ThreadLong. Unnecessary on AArch64 with TBI.
   Value *ThreadLongMaybeUntagged =

 
 bool HWAddressSanitizer::sanitizeFunction(Function &F) {
   if (&F == HwasanCtorFunction)
     return false;
 
   if (!F.hasFnAttribute(Attribute::SanitizeHWAddress))
     return false;
 
   LLVM_DEBUG(dbgs() << "Function: " << F.getName() << "\n");
 
   SmallVector<InterestingMemoryOperand, 16> OperandsToInstrument;
   SmallVector<MemIntrinsic *, 16> IntrinToInstrument;
   SmallVector<AllocaInst*, 8> AllocasToInstrument;
   SmallVector<Instruction*, 8> RetVec;
   SmallVector<Instruction*, 8> LandingPadVec;
   DenseMap<AllocaInst *, std::vector<DbgVariableIntrinsic *>> AllocaDbgMap;
   for (auto &BB : F) {
for (auto &Inst : BB) {
 
       if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&Inst))
         IntrinToInstrument.push_back(MI);
     }
   }
 
   initializeCallbacks(*F.getParent());
 
+  bool Changed = false;
+
   if (!LandingPadVec.empty())
-    instrumentLandingPads(LandingPadVec);
+    Changed |= instrumentLandingPads(LandingPadVec);
 
   if (AllocasToInstrument.empty() && F.hasPersonalityFn() &&
       F.getPersonalityFn()->getName() == kHwasanPersonalityThunkName) {
     // __hwasan_personality_thunk is a no-op for functions without an
     // instrumented stack, so we can drop it.
     F.setPersonalityFn(nullptr);
+    Changed = true;
   }
 
   if (AllocasToInstrument.empty() && OperandsToInstrument.empty() &&
       IntrinToInstrument.empty())
-    return false;
+    return Changed;
 
   assert(!LocalDynamicShadow);
 
   Instruction *InsertPt = &*F.getEntryBlock().begin();
   IRBuilder<> EntryIRB(InsertPt);
   emitPrologue(EntryIRB,
                /*WithFrameRecord*/ ClRecordStackHistory &&
                    !AllocasToInstrument.empty());
 
-  bool Changed = false;
   if (!AllocasToInstrument.empty()) {
     Value *StackTag =
         ClGenerateTagsWithCalls ? nullptr : getStackBaseTag(EntryIRB);
-    Changed |= instrumentStack(AllocasToInstrument, AllocaDbgMap, RetVec,
-                               StackTag);
+    instrumentStack(AllocasToInstrument, AllocaDbgMap, RetVec, StackTag);
   }
-
   // Pad and align each of the allocas that we instrumented to stop small
   // uninteresting allocas from hiding in instrumented alloca's padding and so
   // that we have enough space to store real tags for short granules.
   DenseMap<AllocaInst *, AllocaInst *> AllocaToPaddedAllocaMap;
   for (AllocaInst *AI : AllocasToInstrument) {
     uint64_t Size = getAllocaSizeInBytes(*AI);
     uint64_t AlignedSize = alignTo(Size, Mapping.getObjectAlignment());
     AI->setAlignment(
         Align(std::max(AI->getAlignment(), Mapping.getObjectAlignment())));
     if (Size != AlignedSize) {
       Type *AllocatedType = AI->getAllocatedType();
       if (AI->isArrayAllocation()) {
         uint64_t ArraySize =
             cast<ConstantInt>(AI->getArraySize())->getZExtValue();
         AllocatedType = ArrayType::get(AllocatedType, ArraySize);
       }
for (auto II = EntryIRB.GetInsertBlock()->begin(),
       Instruction *I = &*II++;
       if (auto *AI = dyn_cast<AllocaInst>(I))
         if (isa<ConstantInt>(AI->getArraySize()))
           I->moveBefore(InsertPt);
     }
   }
 
   for (auto &Operand : OperandsToInstrument)
-    Changed |= instrumentMemAccess(Operand);
+    instrumentMemAccess(Operand);
 
   if (ClInstrumentMemIntrinsics && !IntrinToInstrument.empty()) {
     for (auto Inst : IntrinToInstrument)
       instrumentMemIntrinsic(cast<MemIntrinsic>(Inst));
-    Changed = true;
   }
 
   LocalDynamicShadow = nullptr;
   StackBaseTag = nullptr;
 
-  return Changed;
+  return true;
 }
 
 void HWAddressSanitizer::instrumentGlobal(GlobalVariable *GV, uint8_t Tag) {
   Constant *Initializer = GV->getInitializer();
   uint64_t SizeInBytes =
       M.getDataLayout().getTypeAllocSize(Initializer->getType());
   uint64_t NewSize = alignTo(SizeInBytes, Mapping.getObjectAlignment());
   if (SizeInBytes != NewSize) {