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

1088

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

1136–1137

Changed = true?

1165

here and two ifs below?

[serge-sans-paille]

Take remarks into account.

[serge-sans-paille]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

1088

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

1128–1130

You don't need braces around a single line.

[serge-sans-paille]

Take review into account.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

1162

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

public:
   bool instrumentLandingPads(SmallVectorImpl<Instruction *> &RetVec);
   Value *getNextTagWithCall(IRBuilder<> &IRB);
   Value *getStackBaseTag(IRBuilder<> &IRB);
   Value *getAllocaTag(IRBuilder<> &IRB, Value *StackTag, AllocaInst *AI,
                      unsigned AllocaNo);
   Value *getUARTag(IRBuilder<> &IRB, Value *StackTag);
 
   Value *getHwasanThreadSlotPtr(IRBuilder<> &IRB, Type *Ty);
-  void emitPrologue(IRBuilder<> &IRB, bool WithFrameRecord);
+  bool emitPrologue(IRBuilder<> &IRB, bool WithFrameRecord);
 
   void instrumentGlobal(GlobalVariable *GV, uint8_t Tag);
   void instrumentGlobals();
 
   void instrumentPersonalityFunctions();
 
 private:
   LLVMContext *C;
Value *HWAddressSanitizer::getHwasanThreadSlotPtr(IRBuilder<> &IRB, Type *Ty) {
   }
   if (ThreadPtrGlobal)
     return ThreadPtrGlobal;
 
 
   return nullptr;
 }
 
-void HWAddressSanitizer::emitPrologue(IRBuilder<> &IRB, bool WithFrameRecord) {
+bool HWAddressSanitizer::emitPrologue(IRBuilder<> &IRB, bool WithFrameRecord) {
   if (!Mapping.InTls) {
     LocalDynamicShadow = getDynamicShadowNonTls(IRB);
-    return;
+    return LocalDynamicShadow != nullptr;
   }
 
   if (!WithFrameRecord && TargetTriple.isAndroid()) {
     LocalDynamicShadow = getDynamicShadowIfunc(IRB);
-    return;
+    return LocalDynamicShadow != nullptr;
   }
 
   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::emitPrologue(IRBuilder<> &IRB, bool WithFrameRecord) {
   // Note: this is not correct if the pointer is already aligned.
   // Runtime library will make sure this never happens.
   LocalDynamicShadow = IRB.CreateAdd(
       IRB.CreateOr(
           ThreadLongMaybeUntagged,
           ConstantInt::get(IntptrTy, (1ULL << kShadowBaseAlignment) - 1)),
       ConstantInt::get(IntptrTy, 1), "hwasan.shadow");
   LocalDynamicShadow = IRB.CreateIntToPtr(LocalDynamicShadow, Int8PtrTy);
+
+  return true;
 }
 
 Value *HWAddressSanitizer::readRegister(IRBuilder<> &IRB, StringRef Name) {
   Module *M = IRB.GetInsertBlock()->getParent()->getParent();
   Function *ReadRegister =
       Intrinsic::getDeclaration(M, Intrinsic::read_register, IntptrTy);
   MDNode *MD = MDNode::get(*C, {MDString::get(*C, Name)});
   Value *Args[] = {MetadataAsValue::get(*C, MD)};

 
 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());
 
-  if (!LandingPadVec.empty())
-    instrumentLandingPads(LandingPadVec);
+  bool Changed = false;
+
+  if (!LandingPadVec.empty()) {
+    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,
+  Changed |= emitPrologue(EntryIRB,
                /*WithFrameRecord*/ ClRecordStackHistory &&
                    !AllocasToInstrument.empty());
 
-  bool Changed = false;
   if (!AllocasToInstrument.empty()) {
     Value *StackTag =
         ClGenerateTagsWithCalls ? nullptr : getStackBaseTag(EntryIRB);
     Changed |= 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);
       }