Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions bolt/include/bolt/Core/BinaryContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,10 @@ class BinaryContext {
/// DWARF line info for CUs.
std::map<unsigned, DwarfLineTable> DwarfLineTablesCUMap;

/// Container to cache results about functions that their paths lead to
/// a no-return function.
std::unordered_map<BinaryFunction *, bool> CallsNoReturnFunction;
Copy link
Contributor

@atrosinenko atrosinenko Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it looks like LLVM Programmer's Manual advises against the usage of std::unordered_map, though there are several tens of occurrences even in the existing code under llvm/lib. The same in DiscoverNoReturnPass.h below.


/// Internal helper for removing section name from a lookup table.
void deregisterSectionName(const BinarySection &Section);

Expand All @@ -282,6 +286,16 @@ class BinaryContext {
std::unique_ptr<DWARFContext> DwCtx,
JournalingStreams Logger);

void setHasPathToNoReturn(BinaryFunction *Func, bool value = true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] value -> Value

CallsNoReturnFunction[Func] = value;
}

bool cachedInNoReturnMap(BinaryFunction *Func) {
return CallsNoReturnFunction.find(Func) != CallsNoReturnFunction.end();
}

bool hasPathToNoReturn(BinaryFunction *Func);

/// Superset of compiler units that will contain overwritten code that needs
/// new debug info. In a few cases, functions may end up not being
/// overwritten, but it is okay to re-generate debug info for them.
Expand Down
36 changes: 36 additions & 0 deletions bolt/include/bolt/Passes/DiscoverNoReturnPass.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//===- bolt/Passes/Discover.h -----------------------------------*- C++ -*-===//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Invalid file name in the comment.

//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef BOLT_PASSES_DISCOVERNORETURN_H
#define BOLT_PASSES_DISCOVERNORETURN_H
Comment on lines +9 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Defined name is out of sync with the file name.


#include "bolt/Core/BinaryContext.h"
#include "bolt/Core/BinaryFunction.h"
#include "bolt/Passes/BinaryPasses.h"
#include <unordered_map>

namespace llvm {

namespace bolt {

class DiscoverNoReturnPass : public BinaryFunctionPass {
public:
explicit DiscoverNoReturnPass() : BinaryFunctionPass(false) {}

const char *getName() const override { return "discover-no-return"; }

Error runOnFunctions(BinaryContext &BC) override;

private:
std::unordered_map<BinaryFunction *, bool> Visited;
bool traverseFromFunction(BinaryFunction *Func, BinaryContext &BC);
};
} // namespace bolt
} // namespace llvm

#endif
14 changes: 14 additions & 0 deletions bolt/lib/Core/BinaryContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,20 @@ Expected<std::unique_ptr<BinaryContext>> BinaryContext::createBinaryContext(
return std::move(BC);
}

bool BinaryContext::hasPathToNoReturn(BinaryFunction *Func) {
// Dummy way to mark no-return functions.
// FIXME: Find a better way.
if (std::string FuncName = Func->getPrintName();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if getPrintName() is the right accessor to call - maybe getOneName() (or even asking hasName(...)) is more appropriate?

FuncName == "__cxa_throw@PLT" || FuncName != "_Unwind_Resume@PLT" ||
FuncName == "__cxa_rethrow@PLT" || FuncName != "exit@PLT" ||
FuncName == "abort@PLT" || FuncName == "setjmp@PLT" ||
FuncName == "longjmp@PLT")
return true;
Comment on lines +314 to +319
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FuncName != "A" || FuncName != "B" is always true.


auto itr = CallsNoReturnFunction.find(Func);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] itr -> Itr

return itr != CallsNoReturnFunction.end() && itr->second;
}

bool BinaryContext::forceSymbolRelocations(StringRef SymbolName) const {
if (opts::HotText &&
(SymbolName == "__hot_start" || SymbolName == "__hot_end"))
Expand Down
1 change: 1 addition & 0 deletions bolt/lib/Passes/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ add_llvm_library(LLVMBOLTPasses
CacheMetrics.cpp
DataflowAnalysis.cpp
DataflowInfoManager.cpp
DiscoverNoReturnPass.cpp
FrameAnalysis.cpp
FrameOptimizer.cpp
FixRelaxationPass.cpp
Expand Down
84 changes: 84 additions & 0 deletions bolt/lib/Passes/DiscoverNoReturnPass.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
//===- bolt/Passes/ReorderSection.cpp - Reordering of section data --------===//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Wrong file name in the comment.

//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// This file implements DiscoverNoReturnPass class.
//
//===----------------------------------------------------------------------===//

#include "bolt/Passes/DiscoverNoReturnPass.h"

using namespace llvm;

namespace opts {
extern cl::OptionCategory BoltCategory;

static cl::opt<bool> DiscoverNoReturnAnalysis(
"discover-no-return",
cl::desc("analyze the binary and mark no-return functions"), cl::init(true),
cl::cat(BoltCategory), cl::ReallyHidden);
} // namespace opts

namespace llvm {
namespace bolt {

Error DiscoverNoReturnPass::runOnFunctions(BinaryContext &BC) {
bool Changed;
do {
Changed = false;
for (auto &BFI : BC.getBinaryFunctions()) {
auto &Func = BFI.second;
bool PrevStat = BC.hasPathToNoReturn(&Func);
bool CurStat = traverseFromFunction(&Func, BC);
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Was "State" intended here instead of "Stat"?

Changed |= (PrevStat != CurStat);
}
} while (Changed);

return Error::success();
}

bool DiscoverNoReturnPass::traverseFromFunction(BinaryFunction *Func,
BinaryContext &BC) {
// The Function cached before, so return its value
if (BC.cachedInNoReturnMap(Func))
return BC.hasPathToNoReturn(Func);

Visited[Func] = true;
bool Result = true;
bool hasCalls = 0;
bool hasReturns = 0;
Comment on lines +52 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit]

Suggested change
bool hasCalls = 0;
bool hasReturns = 0;
bool HasCalls = 0;
bool HasReturns = 0;

for (auto &BB : *Func) {
if (!BB.getNumCalls())
continue;
for (auto &Inst : BB) {
if (BC.MIB->isCall(Inst)) {
hasCalls = true;
const MCSymbol *TargetSymbol = BC.MIB->getTargetSymbol(Inst);
BinaryFunction *TargetFunction = BC.SymbolToFunctionMap[TargetSymbol];
if (!Visited.count(TargetFunction))
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TargetFuntion needs to be checked for nullptr

Result &= traverseFromFunction(TargetFunction, BC);
}
hasReturns |= BC.MIB->isReturn(Inst);
}
}

// This functions is represented as a leaf in the call graph and doesn't
// have a no-return attribute.
if (!hasCalls && hasReturns)
Result = false;

// If the function doens't have a return instruction then it's a
// no-return function.
if (!hasReturns)
Result = true;
Comment on lines +71 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tailcalls should also be taken into account (BC.MIB->isTailCall(Inst)). If a function ends in a tailcall instruction, it depends on the Called function if it is NoReturn.


BC.setHasPathToNoReturn(Func, Result);
return Result;
}

} // end namespace bolt
} // end namespace llvm
2 changes: 2 additions & 0 deletions bolt/lib/Rewrite/BinaryPassManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "bolt/Passes/AsmDump.h"
#include "bolt/Passes/CMOVConversion.h"
#include "bolt/Passes/ContinuityStats.h"
#include "bolt/Passes/DiscoverNoReturnPass.h"
#include "bolt/Passes/FixRISCVCallsPass.h"
#include "bolt/Passes/FixRelaxationPass.h"
#include "bolt/Passes/FrameOptimizer.h"
Expand Down Expand Up @@ -443,6 +444,7 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {

Manager.registerPass(std::make_unique<CMOVConversion>(),
opts::CMOVConversionFlag);
Manager.registerPass(std::make_unique<DiscoverNoReturnPass>());

// This pass syncs local branches with CFG. If any of the following
// passes breaks the sync - they either need to re-run the pass or
Expand Down
Loading