Fix null pointer dereference in `ProfileSummaryInfo::getPSI()`

[fez]

Summary

This fixes a regression introduced by a cosmetic change
here: https://reviews.llvm.org/D54669

The cosmetic change in the link above introduces a scenario where
calling getPSI() may dereference a NULL pointer (namely if the PSI
member of ProfileSummaryInfo is NULL).
When compiling with clang-9 with -O2, calls to getPSI are inlined.
The compiler sees the nullptr dereference (which is undefined behavior)
and is entitled to remove any nullptr check.

This causes snippets like this to crash:

// The following call to getPSI is inlined, and it contains a pointer
// dereference.
auto *PSI = &Stuff.getPSI();

// This check is assumed to be always true, because if it was false
// the following line would summon undefined behavior.
// So the compiler emits the code for `if (true)`
if (PSI)
  PSI->call_any_method(); // This dereferences PSI which is nullptr

This commit changes the APIs of ProfileSummaryInfo to return naked
pointers instead of dereferencing a possibly null pointer.
In this way, the pointer (null or not) is never dereferenced, and the
compiler is not entitled to remove the if (PSI) check.

[fez]

I added @vsk as a reviewer, because he's the one who accepted the patch set that introduced the code that causes the bug.

[fez]

Updated the patch to build on master. The previous one did not apply cleanly to trunk.

[vsk]

When is a null PSI accessed? It's not clear to me how this happens (based on ProfileSummaryInfoWrapperPass::doInitialization).

Could you share a reduced regression test that reproduces the crash without this patch applied?

[davidxl]

Can you revert D54669 and remove the pointer to reference change and recommit?

[fez]

In D81269#2077071, @vsk wrote:

When is a null PSI accessed? It's not clear to me how this happens (based on ProfileSummaryInfoWrapperPass::doInitialization).

Could you share a reduced regression test that reproduces the crash without this patch applied?

At the moment, I can reliably reproduce it by compiling llvm itself with clang-9 -O2. In lib/Transforms/InstCombine/InstructionCombining.cpp the nullptr check on PSI gets removed. I checked in the compiled object file InstructionCombining.cpp.o and the nullptr check at line 3785 (PSI && PSI->hasProfileSummary()) is gone.

However, I cannot reproduce the crash directly inside llvm's codebase. I tracked down the offending code to a library we're using in our codebase. It was working until upgrading llvm. I have the source of the library and I have minimized the offending code to the following:

legacy::FunctionPassManager PM(MyModule);
PM.add(createInstructionCombiningPass());
PM.run(MyFunction);

In this case doInitialization is not called, so PSI is null.

On one hand, I agree that without calling PM.doInitialization() the API of the legacy::FunctionPassManager is being misused.
The contract of the API is violated, because without calling doInitialization one could end up using some uninitialized state.

On the other hand, the crash I got with the snipped above was surprising. I was debugging my code to figure out the problem, and I saw that getPSI was called in InstructionCombining.cpp, returning nullptr. After the call to getPSI I saw a null pointer check, so I assumed that the presence of PSI is optional, as the rest of the code seemed to suggest. So, I expected the nullptr check to fail, but inspecting stepping through it I jumped directly to the next instruction, and checking the underlying assembly I realized that the compiler had optimized away the check. Everything was even more confused by the fact that compiling llvm (not the snippet) with -O0 the bug went away, because the optimizer just left the nullptr check on PSI in its right place.
After all, if the new API after the cosmetic change returns a reference, why hasn't the call sites be updated to have references instead of pointers? And why are the returned values from getPSI even checked for null if they are actually references? You see what I mean?

So, it seems to me that there is some kind of ambiguity here. In llvm's codebase doInitialization is always properly called. Hence, whenever getPSI gets called, PSI is never null. This is good. And this is also the reason why I cannot reproduce the bug directly in llvm's codebase.

However nobody theoretically stops a user to do something similar to the small snippet above. And in that case what happens is that, before the cosmetic patch I referenced, the code would work just fine, simply skipping over the initialization of BFI (which is guarded by the nullptr check on PSI). Instead, after the cosmetic change I referenced, the code crashes. In principle, there's nothing wrong with the crash per se, because it happens when someone violates the contract (always call doInitialization before run). But the interaction of the cosmetic change with undefined behavior makes very hard for someone debugging the code to find out what the real root cause, because the optimizer gets in the way. It's not an API that is hard to misuse. And for a user may not be straightforward to figure out that a crash in runOnFunction depends on the fact that doInitialization was not called AND the compiler decided to optimize away the nullptr check.

I realize that strictly speaking this may not be a bug, because it gets triggered only when breaking the contract, but the fact that it depends on optimization levels makes it kinda surprising. So, I don't know where do you want to go from here. Do you think that this is worth discussing or I should just drop it?

[fez]

In D81269#2077113, @davidxl wrote:

Can you revert D54669 and remove the pointer to reference change and recommit?

Right now it does not revert cleanly.

And according to the additional discussion that I posted above it might not be necessary to revert the change after all, given that the crash is only triggered by someone misusing the API, forgetting to call doInitialize before run.

Anyway, I'll wait to hear what @vsk has to say, and if we decide to go on with the fix (or some variants of it), I will revert the D54669, erase the offending parts from the cosmetic change, and recommit it.

[vsk]

@fez thanks for explaining. Changing the library to initialize passes properly seems like the way to go, otherwise we'd be advertising that the pass initialization API is in general optional to call (which it's not). Re: chasing down the UB, I realize that's not a great experience, but istm the way to fix that is to modernize getPSI usage to expect a reference and test with -fsanitize=null to qualify the change.

[fez]

Ok then. I'll drop this patch. Is there a way to close the diff?

[vsk]

Near the web UI for adding comments, you should be able to "Abandon Revision" from the "Add Action..." dropdown menu. If not, that's ok, the history here should be sufficient info for future readers.

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

 /// An analysis pass based on legacy pass manager to deliver ProfileSummaryInfo.
 class ProfileSummaryInfoWrapperPass : public ImmutablePass {
   std::unique_ptr<ProfileSummaryInfo> PSI;
 
 public:
   static char ID;
   ProfileSummaryInfoWrapperPass();
 
-  ProfileSummaryInfo &getPSI() { return *PSI; }
-  const ProfileSummaryInfo &getPSI() const { return *PSI; }
+  ProfileSummaryInfo *getPSI() { return PSI.get(); }
+  const ProfileSummaryInfo *getPSI() const { return PSI.get(); }
 
   bool doInitialization(Module &M) override;
   bool doFinalization(Module &M) override;
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
   }
 };
 

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

 }
 
 ModuleSummaryIndexWrapperPass::ModuleSummaryIndexWrapperPass()
     : ModulePass(ID) {
   initializeModuleSummaryIndexWrapperPassPass(*PassRegistry::getPassRegistry());
 }
 
 bool ModuleSummaryIndexWrapperPass::runOnModule(Module &M) {
-  auto *PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+  auto *PSI = getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   Index.emplace(buildModuleSummaryIndex(
       M,
       [this](const Function &F) {
         return &(this->getAnalysis<BlockFrequencyInfoWrapperPass>(
                          *const_cast<Function *>(&F))
                      .getBFI());
       },
       PSI));

llvm/lib/CodeGen/CodeGenPrepare.cpp

bool CodeGenPrepare::runOnFunction(Function &F) {
   SubtargetInfo = TM->getSubtargetImpl(F);
   TLI = SubtargetInfo->getTargetLowering();
   TRI = SubtargetInfo->getRegisterInfo();
   TLInfo = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
   TTI = &getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
   LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
   BPI.reset(new BranchProbabilityInfo(F, *LI));
   BFI.reset(new BlockFrequencyInfo(F, *BPI, *LI));
-  PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+  PSI = getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   OptSize = F.hasOptSize();
   if (ProfileGuidedSectionPrefix) {
     if (PSI->isFunctionHotInCallGraph(&F, *BFI))
       F.setSectionPrefix(".hot");
     else if (PSI->isFunctionColdInCallGraph(&F, *BFI))
       F.setSectionPrefix(".unlikely");
     else if (ProfileUnknownInSpecialSection && PSI->hasPartialSampleProfile() &&
              PSI->isFunctionHotnessUnknown(F))

llvm/lib/Transforms/IPO/HotColdSplitting.cpp

bool HotColdSplitting::run(Module &M) {
   }
   return Changed;
 }
 
 bool HotColdSplittingLegacyPass::runOnModule(Module &M) {
   if (skipModule(M))
     return false;
   ProfileSummaryInfo *PSI =
-      &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+      getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   auto GTTI = [this](Function &F) -> TargetTransformInfo & {
     return this->getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
   };
   auto GBFI = [this](Function &F) {
     return &this->getAnalysis<BlockFrequencyInfoWrapperPass>(F).getBFI();
   };
   std::unique_ptr<OptimizationRemarkEmitter> ORE;
   std::function<OptimizationRemarkEmitter &(Function &)> GetORE =

llvm/lib/Transforms/IPO/Inliner.cpp

#endif
   } while (LocalChange);
 
   return Changed;
 }
 
 bool LegacyInlinerBase::inlineCalls(CallGraphSCC &SCC) {
   CallGraph &CG = getAnalysis<CallGraphWrapperPass>().getCallGraph();
   ACT = &getAnalysis<AssumptionCacheTracker>();
-  PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+  PSI = getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   GetTLI = [&](Function &F) -> const TargetLibraryInfo & {
     return getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
   };
   auto GetAssumptionCache = [&](Function &F) -> AssumptionCache & {
     return ACT->getAssumptionCache(F);
   };
   return inlineCallsImpl(
       SCC, CG, GetAssumptionCache, PSI, GetTLI, InsertLifetime,

llvm/lib/Transforms/IPO/PartialInlining.cpp

struct PartialInlinerLegacyPass : public ModulePass {
 
   bool runOnModule(Module &M) override {
     if (skipModule(M))
       return false;
 
     AssumptionCacheTracker *ACT = &getAnalysis<AssumptionCacheTracker>();
     TargetTransformInfoWrapperPass *TTIWP =
         &getAnalysis<TargetTransformInfoWrapperPass>();
-    ProfileSummaryInfo &PSI =
+    ProfileSummaryInfo *PSI =
         getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
 
     auto GetAssumptionCache = [&ACT](Function &F) -> AssumptionCache & {
       return ACT->getAssumptionCache(F);
     };
 
     auto LookupAssumptionCache = [ACT](Function &F) -> AssumptionCache * {
       return ACT->lookupAssumptionCache(F);
     };
 
     auto GetTTI = [&TTIWP](Function &F) -> TargetTransformInfo & {
       return TTIWP->getTTI(F);
     };
 
     auto GetTLI = [this](Function &F) -> TargetLibraryInfo & {
       return this->getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
     };
 
     return PartialInlinerImpl(GetAssumptionCache, LookupAssumptionCache, GetTTI,
-                              GetTLI, PSI)
+                              GetTLI, *PSI)
         .run(M);
   }
 };
 
 } // end anonymous namespace
 
 std::unique_ptr<FunctionOutliningMultiRegionInfo>
 PartialInlinerImpl::computeOutliningColdRegionsInfo(Function *F,

llvm/lib/Transforms/IPO/SampleProfile.cpp

bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM,
   return retval;
 }
 
 bool SampleProfileLoaderLegacyPass::runOnModule(Module &M) {
   ACT = &getAnalysis<AssumptionCacheTracker>();
   TTIWP = &getAnalysis<TargetTransformInfoWrapperPass>();
   TLIWP = &getAnalysis<TargetLibraryInfoWrapperPass>();
   ProfileSummaryInfo *PSI =
-      &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+      getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   return SampleLoader.runOnModule(M, nullptr, PSI, nullptr);
 }
 
 bool SampleProfileLoader::runOnFunction(Function &F, ModuleAnalysisManager *AM) {
 
   DILocation2SampleMap.clear();
   // By default the entry count is initialized to -1, which will be treated
   // conservatively by getEntryCount as the same as unknown (None). This is

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

bool InstructionCombiningPass::runOnFunction(Function &F) {
   auto &TLI = getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
   auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
   auto &ORE = getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE();
 
   // Optional analyses.
   auto *LIWP = getAnalysisIfAvailable<LoopInfoWrapperPass>();
   auto *LI = LIWP ? &LIWP->getLoopInfo() : nullptr;
   ProfileSummaryInfo *PSI =
-      &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+      getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   BlockFrequencyInfo *BFI =
       (PSI && PSI->hasProfileSummary()) ?
       &getAnalysis<LazyBlockFrequencyInfoPass>().getBFI() :
       nullptr;
 
   return combineInstructionsOverFunction(F, Worklist, AA, AC, TLI, DT, ORE, BFI,
                                          PSI, MaxIterations, LI);
 }

llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp

bool CHR::run() {
   return Changed;
 }
 
 bool ControlHeightReductionLegacyPass::runOnFunction(Function &F) {
   BlockFrequencyInfo &BFI =
       getAnalysis<BlockFrequencyInfoWrapperPass>().getBFI();
   DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
   ProfileSummaryInfo &PSI =
-      getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+      *getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   RegionInfo &RI = getAnalysis<RegionInfoPass>().getRegionInfo();
   std::unique_ptr<OptimizationRemarkEmitter> OwnedORE =
       std::make_unique<OptimizationRemarkEmitter>(&F);
   return CHR(F, BFI, DT, PSI, RI, *OwnedORE.get()).run();
 }
 
 namespace llvm {
 

llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

if (ICPCutOff != 0 && NumOfPGOICallPromotion >= ICPCutOff) {
       break;
     }
   }
   return Changed;
 }
 
 bool PGOIndirectCallPromotionLegacyPass::runOnModule(Module &M) {
   ProfileSummaryInfo *PSI =
-      &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+      getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
 
   // Command-line option has the priority for InLTO.
   return promoteIndirectCalls(M, PSI, InLTO | ICPLTOMode,
                               SamplePGO | ICPSamplePGOMode);
 }
 
 PreservedAnalyses PGOIndirectCallPromotion::run(Module &M,
                                                 ModuleAnalysisManager &AM) {
   ProfileSummaryInfo *PSI = &AM.getResult<ProfileSummaryAnalysis>(M);
 
   if (!promoteIndirectCalls(M, PSI, InLTO | ICPLTOMode,
                             SamplePGO | ICPSamplePGOMode, &AM))
     return PreservedAnalyses::all();
 
   return PreservedAnalyses::none();
 }

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

bool ConstantHoistingLegacyPass::runOnFunction(Function &Fn) {
 
   bool MadeChange =
       Impl.runImpl(Fn, getAnalysis<TargetTransformInfoWrapperPass>().getTTI(Fn),
                    getAnalysis<DominatorTreeWrapperPass>().getDomTree(),
                    ConstHoistWithBlockFrequency
                        ? &getAnalysis<BlockFrequencyInfoWrapperPass>().getBFI()
                        : nullptr,
                    Fn.getEntryBlock(),
-                   &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI());
+                   getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI());
 
   if (MadeChange) {
     LLVM_DEBUG(dbgs() << "********** Function after Constant Hoisting: "
                       << Fn.getName() << '\n');
     LLVM_DEBUG(dbgs() << Fn);
   }
   LLVM_DEBUG(dbgs() << "********** End Constant Hoisting **********\n");
 

llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp

public:
 
   bool runOnFunction(Function &F) override {
     if (skipFunction(F))
       return false;
 
     auto &LI = getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
     auto &LAA = getAnalysis<LoopAccessLegacyAnalysis>();
     auto &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-    auto *PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+    auto *PSI = getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
     auto *BFI = (PSI && PSI->hasProfileSummary()) ?
                 &getAnalysis<LazyBlockFrequencyInfoPass>().getBFI() :
                 nullptr;
 
     // Process each loop nest in the function.
     return eliminateLoadsAcrossLoops(
         F, LI, DT, BFI, PSI,
         [&LAA](Loop &L) -> const LoopAccessInfo & { return LAA.getInfo(&L); });

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

bool runOnFunction(Function &F) override {
     auto *BFI = &getAnalysis<BlockFrequencyInfoWrapperPass>().getBFI();
     auto *TLIP = getAnalysisIfAvailable<TargetLibraryInfoWrapperPass>();
     auto *TLI = TLIP ? &TLIP->getTLI(F) : nullptr;
     auto *AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
     auto *AC = &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
     auto *LAA = &getAnalysis<LoopAccessLegacyAnalysis>();
     auto *DB = &getAnalysis<DemandedBitsWrapperPass>().getDemandedBits();
     auto *ORE = &getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE();
-    auto *PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
+    auto *PSI = getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
 
     std::function<const LoopAccessInfo &(Loop &)> GetLAA =
         [&](Loop &L) -> const LoopAccessInfo & { return LAA->getInfo(&L); };
 
     return Impl.runImpl(F, *SE, *LI, *TTI, *DT, *BFI, TLI, *DB, *AA, *AC,
                         GetLAA, *ORE, PSI).MadeAnyChange;
   }