From 36e2faad3d1f57e53e261b6e73b294afc221b5aa Mon Sep 17 00:00:00 2001 From: Alessandro Di Federico Date: Fri, 18 Oct 2024 11:19:42 +0200 Subject: [PATCH] Introduce -Wunreachable-code-break --- CMakeLists.txt | 1 + lib/Backend/DecompileFunction.cpp | 20 +++++++--------- lib/Canonicalize/MakeModelGEPPass.cpp | 19 +++++++-------- .../Middleend/ResolveLeafUnions.cpp | 8 +++---- lib/InitModelTypes/InitModelTypes.cpp | 2 +- lib/RestructureCFG/ASTNodeUtils.cpp | 11 ++++----- lib/RestructureCFG/BeautifyGHAST.cpp | 20 ++++++++-------- .../FallThroughScopeAnalysis.cpp | 16 ++++++------- lib/RestructureCFG/InlineDispatcherSwitch.cpp | 24 +++++++++---------- lib/RestructureCFG/PromoteCallNoReturn.cpp | 4 ++-- lib/RestructureCFG/SimplifyDualSwitch.cpp | 2 +- lib/Support/ModelHelpers.cpp | 9 ++++--- lib/TypeNames/LLVMTypeNames.cpp | 18 +++++--------- 13 files changed, 70 insertions(+), 84 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 0c0fe3452..5c4ee837a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -45,6 +45,7 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wextra ") # set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-limit-debug-info") # Enable some additional warnings +add_flag_if_available("-Wunreachable-code-break") add_flag_if_available("-Wimplicit-fallthrough") add_flag_if_available("-Wnon-virtual-dtor") add_flag_if_available("-Winconsistent-missing-destructor-override") diff --git a/lib/Backend/DecompileFunction.cpp b/lib/Backend/DecompileFunction.cpp index f57f87c62..9aa873e1f 100644 --- a/lib/Backend/DecompileFunction.cpp +++ b/lib/Backend/DecompileFunction.cpp @@ -561,7 +561,7 @@ CCodeGenerator::getConstantToken(const llvm::Value *C) const { rc_return buildCastExpr(rc_recur getConstantToken(Operand), SrcType, DstType); - } break; + } default: revng_abort(dumpToString(ConstExpr).c_str()); @@ -1135,8 +1135,7 @@ CCodeGenerator::getInstructionToken(const llvm::Instruction *I) const { revng_abort(Error.c_str()); rc_return ""; - - } break; + } case llvm::Instruction::Ret: { @@ -1147,8 +1146,7 @@ CCodeGenerator::getInstructionToken(const llvm::Instruction *I) const { Result += " " + rc_recur getToken(ReturnedVal); rc_return addDebugInfo(I, Result, B); - - } break; + } case llvm::Instruction::Unreachable: rc_return addDebugInfo(I, "__builtin_trap()", B); @@ -1168,8 +1166,7 @@ CCodeGenerator::getInstructionToken(const llvm::Instruction *I) const { + addParentheses(Op1String) + " : " + addParentheses(Op2String), B); - - } break; + } default: { std::string Error = "Cannot getToken for llvm::Instruction: " @@ -1431,8 +1428,7 @@ CCodeGenerator::buildGHASTCondition(const ExprNode *E, bool EmitBB) { } rc_return CompareNodeString; - - } break; + } case NodeKind::NK_Atomic: { revng_log(VisitLog, "(atomic)"); @@ -1476,7 +1472,7 @@ CCodeGenerator::buildGHASTCondition(const ExprNode *E, bool EmitBB) { } } rc_return rc_recur getToken(Br->getCondition()); - } break; + } case NodeKind::NK_Not: { revng_log(VisitLog, "(not)"); @@ -1485,7 +1481,7 @@ CCodeGenerator::buildGHASTCondition(const ExprNode *E, bool EmitBB) { ExprNode *Negated = N->getNegatedNode(); rc_return B.getOperator(ptml::CBuilder::Operator::BoolNot) + addAlwaysParentheses(rc_recur buildGHASTCondition(Negated, EmitBB)); - } break; + } case NodeKind::NK_And: case NodeKind::NK_Or: { @@ -1502,7 +1498,7 @@ CCodeGenerator::buildGHASTCondition(const ExprNode *E, bool EmitBB) { B.getOperator(PTMLOperator::BoolOr); rc_return addAlwaysParentheses(Child1Token) + " " + OpToken.toString() + " " + addAlwaysParentheses(Child2Token); - } break; + } default: revng_abort("Unknown ExprNode kind"); diff --git a/lib/Canonicalize/MakeModelGEPPass.cpp b/lib/Canonicalize/MakeModelGEPPass.cpp index 46fc5b3d5..2f1662c22 100644 --- a/lib/Canonicalize/MakeModelGEPPass.cpp +++ b/lib/Canonicalize/MakeModelGEPPass.cpp @@ -572,7 +572,7 @@ getIRArithmetic(Use &AddressUse, const ModelTypesMap &PointerTypes) { IRArithmetic Result = LHSIsAddress ? LHS : RHS; Result.Summation += LHSIsAddress ? RHS.Summation : LHS.Summation; rc_return Result; - } break; + } case Instruction::ZExt: { // Zero extension is the only thing we traverse that allows the size to @@ -592,7 +592,7 @@ getIRArithmetic(Use &AddressUse, const ModelTypesMap &PointerTypes) { revng_log(ModelGEPLog, "Traverse cast!"); rc_return rc_recur getIRArithmetic(AddrArithmeticInst->getOperandUse(0), PointerTypes); - } break; + } case Instruction::Mul: { @@ -614,8 +614,7 @@ getIRArithmetic(Use &AddressUse, const ModelTypesMap &PointerTypes) { // non-address and non-strided instruction, just like e.g. division. rc_return IRArithmetic::unknown(AddrArithmeticInst); } - - } break; + } case Instruction::Shl: { @@ -633,7 +632,6 @@ getIRArithmetic(Use &AddressUse, const ModelTypesMap &PointerTypes) { auto *IndexForStridedAccess = AddrArithmeticInst->getOperand(0); rc_return IRArithmetic::index(Stride, IndexForStridedAccess); - break; } } } @@ -642,16 +640,15 @@ getIRArithmetic(Use &AddressUse, const ModelTypesMap &PointerTypes) { // and non-strided instruction, just like e.g. division. rc_return IRArithmetic::unknown(AddrArithmeticInst); - - } break; + } case Instruction::Alloca: { rc_return std::nullopt; - } break; + } case Instruction::GetElementPtr: { revng_abort("TODO: gep is not supported by make-model-gep yet"); - } break; + } case Instruction::Load: case Instruction::Call: @@ -674,7 +671,7 @@ getIRArithmetic(Use &AddressUse, const ModelTypesMap &PointerTypes) { // address, but it's just considered as regular offset arithmetic of // an unknown offset. rc_return IRArithmetic::unknown(AddrArithmeticInst); - } break; + } default: { revng_abort("Unexpected instruction for address arithmetic"); @@ -1622,7 +1619,7 @@ getAccessedTypeOnIR(const llvm::Use &U, revng_log(ModelGEPLog, "AccessedTypeOnIR: " << toString(Result)); return Result; - } break; + } case llvm::Instruction::Store: { auto *Store = cast(UserInstr); diff --git a/lib/DataLayoutAnalysis/Middleend/ResolveLeafUnions.cpp b/lib/DataLayoutAnalysis/Middleend/ResolveLeafUnions.cpp index ae4f622ce..aea70e9bc 100644 --- a/lib/DataLayoutAnalysis/Middleend/ResolveLeafUnions.cpp +++ b/lib/DataLayoutAnalysis/Middleend/ResolveLeafUnions.cpp @@ -140,11 +140,11 @@ static std::partial_ordering compareLeafTypes(const LayoutTypeSystemNode *A, case LeafType::Generic: { return std::partial_ordering::equivalent; - } break; + } case LeafType::Pointer: { return std::partial_ordering::less; - } break; + } default: revng_abort(); @@ -158,11 +158,11 @@ static std::partial_ordering compareLeafTypes(const LayoutTypeSystemNode *A, case LeafType::Generic: { return std::partial_ordering::greater; - } break; + } case LeafType::Pointer: { return comparePointee(AType.Pointee, BType.Pointee); - } break; + } default: revng_abort(); diff --git a/lib/InitModelTypes/InitModelTypes.cpp b/lib/InitModelTypes/InitModelTypes.cpp index 7413d5ddc..ea861f8ec 100644 --- a/lib/InitModelTypes/InitModelTypes.cpp +++ b/lib/InitModelTypes/InitModelTypes.cpp @@ -503,7 +503,7 @@ initModelTypesImpl(const llvm::Instruction &I, revng_assert(BaseType->isSingleValueType()); rc_return model::PointerType::make(llvmIntToModelType(BaseType, Model), Model.Architecture()); - } break; + } case Instruction::Select: { auto *Select = dyn_cast(&I); diff --git a/lib/RestructureCFG/ASTNodeUtils.cpp b/lib/RestructureCFG/ASTNodeUtils.cpp index bfe81f5e8..fb6162b56 100644 --- a/lib/RestructureCFG/ASTNodeUtils.cpp +++ b/lib/RestructureCFG/ASTNodeUtils.cpp @@ -26,7 +26,6 @@ static RecursiveCoroutine needsLoopVarImpl(const ASTNode *N) { case ASTNode::NodeKind::NK_Continue: case ASTNode::NodeKind::NK_Code: rc_return false; - break; case ASTNode::NodeKind::NK_If: { const IfNode *If = llvm::cast(N); @@ -40,12 +39,12 @@ static RecursiveCoroutine needsLoopVarImpl(const ASTNode *N) { rc_return true; rc_return false; - } break; + } case ASTNode::NodeKind::NK_Scs: { const ScsNode *LoopBody = llvm::cast(N); rc_return rc_recur needsLoopVarImpl(LoopBody->getBody()); - } break; + } case ASTNode::NodeKind::NK_List: { const SequenceNode *Seq = llvm::cast(N); @@ -54,7 +53,7 @@ static RecursiveCoroutine needsLoopVarImpl(const ASTNode *N) { rc_return true; rc_return false; - } break; + } case ASTNode::NodeKind::NK_Switch: { const SwitchNode *Switch = llvm::cast(N); @@ -68,11 +67,11 @@ static RecursiveCoroutine needsLoopVarImpl(const ASTNode *N) { rc_return true; rc_return false; - } break; + } case ASTNode::NodeKind::NK_Set: { rc_return true; - } break; + } } } diff --git a/lib/RestructureCFG/BeautifyGHAST.cpp b/lib/RestructureCFG/BeautifyGHAST.cpp index 9f02a48f9..b6e300cc5 100644 --- a/lib/RestructureCFG/BeautifyGHAST.cpp +++ b/lib/RestructureCFG/BeautifyGHAST.cpp @@ -98,24 +98,24 @@ static RecursiveCoroutine hasSideEffects(ExprNode *Expr) { } } rc_return false; - } break; + } case ExprNode::NodeKind::NK_Not: { auto *Not = llvm::cast(Expr); rc_return rc_recur hasSideEffects(Not->getNegatedNode()); - } break; + } case ExprNode::NodeKind::NK_And: { auto *And = llvm::cast(Expr); const auto [LHS, RHS] = And->getInternalNodes(); rc_return rc_recur hasSideEffects(LHS) or rc_recur hasSideEffects(RHS); - } break; + } case ExprNode::NodeKind::NK_Or: { auto *Or = llvm::cast(Expr); const auto [LHS, RHS] = Or->getInternalNodes(); rc_return rc_recur hasSideEffects(LHS) or rc_recur hasSideEffects(RHS); - } break; + } default: revng_abort(); @@ -688,7 +688,7 @@ computeCumulativeNodeWeight(ASTNode *Node, Accum += NWeight; } rc_return Accum; - } break; + } case ASTNode::NK_Scs: { ScsNode *Loop = llvm::cast(Node); if (Loop->hasBody()) { @@ -700,7 +700,7 @@ computeCumulativeNodeWeight(ASTNode *Node, } else { rc_return 1; } - } break; + } case ASTNode::NK_If: { IfNode *If = llvm::cast(Node); @@ -717,7 +717,7 @@ computeCumulativeNodeWeight(ASTNode *Node, NodeWeight[Else] = ElseWeight; } rc_return ThenWeight + ElseWeight + 1; - } break; + } case ASTNode::NK_Switch: { SwitchNode *Switch = llvm::cast(Node); @@ -730,7 +730,7 @@ computeCumulativeNodeWeight(ASTNode *Node, SwitchWeight += CaseWeight; } rc_return SwitchWeight + 1; - } break; + } case ASTNode::NK_Code: { // TODO: At the moment we use the BasicBlock size to assign a weight to the @@ -740,7 +740,7 @@ computeCumulativeNodeWeight(ASTNode *Node, CodeNode *Code = llvm::cast(Node); llvm::BasicBlock *BB = Code->getBB(); rc_return BB->size(); - } break; + } case ASTNode::NK_Continue: { // The weight of a continue node, contrary to what intuition would suggest, @@ -768,7 +768,7 @@ computeCumulativeNodeWeight(ASTNode *Node, // If we assign weight 1 to all these cases, no distinction is needed for // them. rc_return 1; - } break; + } default: revng_abort(); } diff --git a/lib/RestructureCFG/FallThroughScopeAnalysis.cpp b/lib/RestructureCFG/FallThroughScopeAnalysis.cpp index d0cbc7711..1f3ead9c1 100644 --- a/lib/RestructureCFG/FallThroughScopeAnalysis.cpp +++ b/lib/RestructureCFG/FallThroughScopeAnalysis.cpp @@ -81,7 +81,7 @@ fallThroughScopeImpl(const model::Binary &Model, // sequence node is nofallthrough. ASTNode *Last = Seq->getNodeN(Seq->length() - 1); rc_return ResultMap.at(Last); - } break; + } case ASTNode::NK_Scs: { ScsNode *Loop = llvm::cast(Node); @@ -99,7 +99,7 @@ fallThroughScopeImpl(const model::Binary &Model, // perform fallthrough (which is reasonable, considering that the body of a // loop will end with `break` and `continue` statements) rc_return FallThroughScopeType::FallThrough; - } break; + } case ASTNode::NK_If: { IfNode *If = llvm::cast(Node); @@ -119,7 +119,7 @@ fallThroughScopeImpl(const model::Binary &Model, } rc_return combineTypes(ThenFallThrough, ElseFallThrough); - } break; + } case ASTNode::NK_Switch: { SwitchNode *Switch = llvm::cast(Node); @@ -164,7 +164,7 @@ fallThroughScopeImpl(const model::Binary &Model, } else { rc_return FallThroughScopeType::FallThrough; } - } break; + } case ASTNode::NK_Code: { CodeNode *Code = llvm::cast(Node); llvm::BasicBlock *BB = Code->getBB(); @@ -222,22 +222,22 @@ fallThroughScopeImpl(const model::Binary &Model, } rc_return FallThroughScopeType::FallThrough; - } break; + } case ASTNode::NK_Set: { rc_return FallThroughScopeType::FallThrough; - } break; + } case ASTNode::NK_SwitchBreak: { // `The `SwitchBreak` represents the fact that we fallthrough from the // switch out rc_return FallThroughScopeType::FallThrough; - } break; + } case ASTNode::NK_Continue: { rc_return FallThroughScopeType::Continue; } case ASTNode::NK_Break: { rc_return FallThroughScopeType::LoopBreak; - } break; + } default: revng_abort(); } diff --git a/lib/RestructureCFG/InlineDispatcherSwitch.cpp b/lib/RestructureCFG/InlineDispatcherSwitch.cpp index 6f792c1d4..abb3bc92d 100644 --- a/lib/RestructureCFG/InlineDispatcherSwitch.cpp +++ b/lib/RestructureCFG/InlineDispatcherSwitch.cpp @@ -397,7 +397,7 @@ static RecursiveCoroutine containsSet(ASTNode *Node) { HasSet = HasSet or rc_recur containsSet(N); } rc_return HasSet; - } break; + } case ASTNode::NK_Scs: { ScsNode *Loop = llvm::cast(Node); @@ -407,7 +407,7 @@ static RecursiveCoroutine containsSet(ASTNode *Node) { } else { rc_return false; } - } break; + } case ASTNode::NK_If: { IfNode *If = llvm::cast(Node); @@ -423,7 +423,7 @@ static RecursiveCoroutine containsSet(ASTNode *Node) { } rc_return HasSet; - } break; + } case ASTNode::NK_Switch: { auto *Switch = llvm::cast(Node); @@ -435,17 +435,17 @@ static RecursiveCoroutine containsSet(ASTNode *Node) { } rc_return HasSet; - } break; + } case ASTNode::NK_Set: { rc_return true; - } break; + } case ASTNode::NK_Code: case ASTNode::NK_SwitchBreak: case ASTNode::NK_Continue: case ASTNode::NK_Break: { rc_return false; - } break; + } default: revng_unreachable(); } @@ -534,7 +534,7 @@ static RecursiveCoroutine containsContinueOrBreak(ASTNode *Node) { // This will be executed only if no non local CF has been found in the // above iterations rc_return false; - } break; + } case ASTNode::NK_Scs: { // At the current stage, we do not consider the possibility of inlining a @@ -542,7 +542,7 @@ static RecursiveCoroutine containsContinueOrBreak(ASTNode *Node) { // eventual exit dispatcher associated to the `ScsNode`. // We may want, however, to enable this possibility in the future. rc_return true; - } break; + } case ASTNode::NK_If: { IfNode *If = llvm::cast(Node); @@ -563,7 +563,7 @@ static RecursiveCoroutine containsContinueOrBreak(ASTNode *Node) { // This will be executed only if no non local CF has been found in both the // `then` and `else` branches rc_return false; - } break; + } case ASTNode::NK_Switch: { auto *Switch = llvm::cast(Node); @@ -577,7 +577,7 @@ static RecursiveCoroutine containsContinueOrBreak(ASTNode *Node) { // This will be executed only if no non local CF has been found in any of // the `case`s of the `switch` rc_return false; - } break; + } case ASTNode::NK_Set: case ASTNode::NK_Code: case ASTNode::NK_SwitchBreak: { @@ -586,11 +586,11 @@ static RecursiveCoroutine containsContinueOrBreak(ASTNode *Node) { // `break` statements related to loops. The `SwitchBreak` statement does not // fall in this category. rc_return false; - } break; + } case ASTNode::NK_Continue: case ASTNode::NK_Break: { rc_return true; - } break; + } default: revng_unreachable(); } diff --git a/lib/RestructureCFG/PromoteCallNoReturn.cpp b/lib/RestructureCFG/PromoteCallNoReturn.cpp index 879d033ed..96c754f48 100644 --- a/lib/RestructureCFG/PromoteCallNoReturn.cpp +++ b/lib/RestructureCFG/PromoteCallNoReturn.cpp @@ -32,12 +32,12 @@ static bool isPreferredAsFallThrough(FallThroughScopeType Element) { case FallThroughScopeType::Continue: case FallThroughScopeType::LoopBreak: { return true; - } break; + } case FallThroughScopeType::FallThrough: case FallThroughScopeType::MixedNoFallThrough: case FallThroughScopeType::CallNoReturn: { return false; - } break; + } } } diff --git a/lib/RestructureCFG/SimplifyDualSwitch.cpp b/lib/RestructureCFG/SimplifyDualSwitch.cpp index 4a8bac2b3..2ec356058 100644 --- a/lib/RestructureCFG/SimplifyDualSwitch.cpp +++ b/lib/RestructureCFG/SimplifyDualSwitch.cpp @@ -244,7 +244,7 @@ simplifyDualSwitch(ASTTree &AST, ASTNode *Node) { } rc_return If; - } break; + } case ASTNode::NK_Code: case ASTNode::NK_Set: case ASTNode::NK_SwitchBreak: diff --git a/lib/Support/ModelHelpers.cpp b/lib/Support/ModelHelpers.cpp index fbba092cf..a7b0275ed 100644 --- a/lib/Support/ModelHelpers.cpp +++ b/lib/Support/ModelHelpers.cpp @@ -252,7 +252,6 @@ handleReturnValue(const model::TypeDefinition &Prototype, revng_assert(Layout.ReturnValues.size() == 1); revng_assert(Layout.ReturnValues[0].Type->isScalar()); return { Layout.ReturnValues[0].Type }; - break; case abi::FunctionType::ReturnMethod::RegisterSet: return flattenReturnTypes(Layout, Model); default: @@ -448,14 +447,14 @@ getExpectedModelType(const llvm::Use *U, const model::Binary &Model) { auto BitWidth = U->get()->getType()->getIntegerBitWidth(); revng_assert(BitWidth >= 8 and std::has_single_bit(BitWidth)); return { model::PrimitiveType::makeSigned(BitWidth / 8) }; - } break; + } case llvm::Instruction::UDiv: case llvm::Instruction::URem: { auto BitWidth = U->get()->getType()->getIntegerBitWidth(); revng_assert(BitWidth >= 8 and std::has_single_bit(BitWidth)); return { model::PrimitiveType::makeUnsigned(BitWidth / 8) }; - } break; + } case llvm::Instruction::AShr: case llvm::Instruction::LShr: @@ -494,7 +493,7 @@ getExpectedModelType(const llvm::Use *U, const model::Binary &Model) { return { model::PrimitiveType::makeNumber(Bytes) }; else return { model::PrimitiveType::makePointerOrNumber(Bytes) }; - } break; + } case llvm::Instruction::Mul: case llvm::Instruction::And: case llvm::Instruction::Or: @@ -504,7 +503,7 @@ getExpectedModelType(const llvm::Use *U, const model::Binary &Model) { and (BitWidth == 1 or BitWidth >= 8)); auto Bytes = (BitWidth == 1) ? 1 : BitWidth / 8; return { model::PrimitiveType::makeNumber(Bytes) }; - } break; + } case llvm::Instruction::FAdd: case llvm::Instruction::FSub: case llvm::Instruction::FMul: diff --git a/lib/TypeNames/LLVMTypeNames.cpp b/lib/TypeNames/LLVMTypeNames.cpp index eb5393807..df1b4add3 100644 --- a/lib/TypeNames/LLVMTypeNames.cpp +++ b/lib/TypeNames/LLVMTypeNames.cpp @@ -58,7 +58,7 @@ bool isScalarCType(const llvm::Type *LLVMType) { default: return false; } - } break; + } default: return false; @@ -73,25 +73,23 @@ std::string getScalarCType(const llvm::Type *LLVMType, const CBuilder &B) { case llvm::Type::HalfTyID: case llvm::Type::BFloatTyID: return B.tokenTag("float16_t", ptml::c::tokens::Type).toString(); - break; + case llvm::Type::FloatTyID: return B.tokenTag("float32_t", ptml::c::tokens::Type).toString(); - break; + case llvm::Type::DoubleTyID: return B.tokenTag("float64_t", ptml::c::tokens::Type).toString(); - break; + case llvm::Type::X86_FP80TyID: // TODO: 80-bit float have 96 bit storage, how should we call them? return B.tokenTag("float96_t", ptml::c::tokens::Type).toString(); - break; + case llvm::Type::FP128TyID: case llvm::Type::PPC_FP128TyID: return B.tokenTag("float128_t", ptml::c::tokens::Type).toString(); - break; case llvm::Type::VoidTyID: { return B.tokenTag("void", ptml::c::tokens::Type).toString(); - break; case llvm::Type::IntegerTyID: { auto *IntType = cast(LLVMType); @@ -102,16 +100,12 @@ std::string getScalarCType(const llvm::Type *LLVMType, const CBuilder &B) { return B.tokenTag("uint8_t", ptml::c::tokens::Type).toString(); case 16: return B.tokenTag("uint16_t", ptml::c::tokens::Type).toString(); - break; case 32: return B.tokenTag("uint32_t", ptml::c::tokens::Type).toString(); - break; case 64: return B.tokenTag("uint64_t", ptml::c::tokens::Type).toString(); - break; case 128: return B.tokenTag("uint128_t", ptml::c::tokens::Type).toString(); - break; default: revng_abort("Found an LLVM integer with a size that is not a power of " "two"); @@ -121,7 +115,7 @@ std::string getScalarCType(const llvm::Type *LLVMType, const CBuilder &B) { case llvm::Type::PointerTyID: { return B.tokenTag("void", ptml::c::tokens::Type) + " " + B.getOperator(Operator::PointerDereference); - } break; + } default: revng_abort("Cannot convert this type directly to a C type.");