diff options
author | Rot127 <45763064+Rot127@users.noreply.github.com> | 2024-04-26 07:11:46 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-04-26 15:11:46 +0800 |
commit | 6c7b54817f792dd8d69959d3efb9fedbe740b648 (patch) | |
tree | 457af451c85457648d64badb268b84eb6b834e50 | |
parent | c4d09930710c99ab8469a4d7fc45d28b5b1b106e (diff) | |
download | capstone-6c7b54817f792dd8d69959d3efb9fedbe740b648.tar.gz |
Add a clang-tidy checks and warnings (#2312)
-rw-r--r-- | .github/workflows/clang-tidy.yml | 29 | ||||
-rw-r--r-- | .gitignore | 2 | ||||
-rw-r--r-- | CMakeLists.txt | 14 | ||||
-rw-r--r-- | arch/AArch64/AArch64BaseInfo.h | 2 | ||||
-rw-r--r-- | arch/AArch64/AArch64Disassembler.c | 16 | ||||
-rw-r--r-- | arch/AArch64/AArch64GenAsmWriter.inc | 2 | ||||
-rw-r--r-- | arch/AArch64/AArch64InstPrinter.c | 2 | ||||
-rw-r--r-- | arch/AArch64/AArch64Mapping.c | 2 | ||||
-rw-r--r-- | arch/ARM/ARMDisassembler.c | 2 | ||||
-rw-r--r-- | arch/ARM/ARMMapping.c | 58 | ||||
-rw-r--r-- | arch/HPPA/HPPADisassembler.c | 2 | ||||
-rw-r--r-- | arch/M68K/M68KDisassembler.c | 4 | ||||
-rw-r--r-- | cstool/cstool.c | 2 | ||||
-rw-r--r-- | docs/cs_v6_release_guide.md | 4 | ||||
-rwxr-xr-x | run-clang-tidy.sh | 30 | ||||
-rw-r--r-- | suite/fuzz/fuzz_disasm.c | 8 |
16 files changed, 123 insertions, 56 deletions
diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml new file mode 100644 index 00000000..1a814cae --- /dev/null +++ b/.github/workflows/clang-tidy.yml @@ -0,0 +1,29 @@ +name: Run clang-tidy +on: + push: + paths: + - '**.c' + - '**.h' + pull_request: + +jobs: + analyze: + runs-on: ubuntu-latest + + name: Install clang-tidy + steps: + - uses: actions/checkout@v3 + - name: Install clang-tidy + run: | + sudo apt install clang-tidy + + - name: Build + run: | + mkdir build && cd build + cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_SHARED_LIBS=1 .. + sudo cmake --build . --config Release + cd .. + + - name: Check for warnings + run: | + ./run-clang-tidy.sh build @@ -131,7 +131,7 @@ fuzz_bindisasm fuzz_disasm fuzz_decode_platform capstone_get_setup -suite/fuzz/ +suite/fuzz/corpus suite/cstest/cmocka/ *.s diff --git a/CMakeLists.txt b/CMakeLists.txt index 2a01ae8b..0a42ef28 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -25,22 +25,32 @@ project(capstone VERSION 5.0 ) -set(UNIX_COMPILER_OPTIONS -Werror -Wshift-negative-value -Wreturn-type -Wformat -Wmissing-braces -Wunused-function -Warray-bounds -Wunused-variable -Wparentheses -Wint-in-bool-context -Wmisleading-indentation) +set(UNIX_COMPILER_OPTIONS -Werror -Wall -Warray-bounds -Wshift-negative-value -Wreturn-type -Wformat -Wmissing-braces -Wunused-function -Warray-bounds -Wunused-variable -Wparentheses -Wint-in-bool-context -Wmisleading-indentation) # maybe-unitialzied is only supported by newer versions of GCC. # Unfortunately, it is pretty unreliable and reports wrong results. # So we disable it for all compilers versions which support it. include(CheckCCompilerFlag) check_c_compiler_flag("-Wno-maybe-unitialized" SUPPORTS_MU) +check_c_compiler_flag("-Wshadow=local" SUPPORTS_SHADOWING) +check_c_compiler_flag("-Wsometimes-uninitialized" SUPPORTS_SUNINIT) if (SUPPORTS_MU) set(UNIX_COMPILER_OPTIONS ${UNIX_COMPILER_OPTIONS} -Wno-maybe-unitialized) endif() +if (SUPPORTS_SHADOWING) + set(UNIX_COMPILER_OPTIONS ${UNIX_COMPILER_OPTIONS} -Wshadow=local) +endif() + +if (SUPPORTS_SUNINIT) + set(UNIX_COMPILER_OPTIONS ${UNIX_COMPILER_OPTIONS} -Wsometimes-uninitialized) +endif() + if (MSVC) add_compile_options(/W1 /w14189) else() - add_compile_options(${UNIX_COMPILE_OPTIONS}) + add_compile_options(${UNIX_COMPILER_OPTIONS}) endif() diff --git a/arch/AArch64/AArch64BaseInfo.h b/arch/AArch64/AArch64BaseInfo.h index db1c6969..b5f0a17a 100644 --- a/arch/AArch64/AArch64BaseInfo.h +++ b/arch/AArch64/AArch64BaseInfo.h @@ -852,6 +852,7 @@ inline static const char *AArch64PACKeyIDToString(AArch64PACKey_ID KeyID) case AArch64PACKey_DB: return "db"; } + return NULL; } /// Return numeric key ID for 2-letter identifier string. @@ -867,6 +868,7 @@ AArch64StringToPACKeyID(const char *Name) if (strcmp(Name, "db") == 0) return AArch64PACKey_DB; assert(0 && "Invalid PAC key"); + return AArch64PACKey_LAST; } // end namespace AArch64 diff --git a/arch/AArch64/AArch64Disassembler.c b/arch/AArch64/AArch64Disassembler.c index 93bf9510..1f5ff17a 100644 --- a/arch/AArch64/AArch64Disassembler.c +++ b/arch/AArch64/AArch64Disassembler.c @@ -360,25 +360,25 @@ static DecodeStatus getInstruction(csh handle, const uint8_t *Bytes, size_t Byte // For Scalable Matrix Extension (SME) instructions that have an // implicit operand for the accumulator (ZA) or implicit immediate zero // which isn't encoded, manually insert operand. - for (unsigned i = 0; i < Desc.NumOperands; i++) { - if (Desc.OpInfo[i].OperandType == MCOI_OPERAND_REGISTER) { - switch (Desc.OpInfo[i].RegClass) { + for (unsigned j = 0; j < Desc.NumOperands; j++) { + if (Desc.OpInfo[j].OperandType == MCOI_OPERAND_REGISTER) { + switch (Desc.OpInfo[j].RegClass) { default: break; case AArch64_MPRRegClassID: - MCInst_insert0(MI, i, MCOperand_CreateReg1(MI, AArch64_ZA)); + MCInst_insert0(MI, j, MCOperand_CreateReg1(MI, AArch64_ZA)); break; case AArch64_MPR8RegClassID: - MCInst_insert0(MI, i, + MCInst_insert0(MI, j, MCOperand_CreateReg1(MI, AArch64_ZAB0)); break; case AArch64_ZTRRegClassID: - MCInst_insert0(MI, i, MCOperand_CreateReg1(MI, AArch64_ZT0)); + MCInst_insert0(MI, j, MCOperand_CreateReg1(MI, AArch64_ZT0)); break; } - } else if (Desc.OpInfo[i].OperandType == + } else if (Desc.OpInfo[j].OperandType == AArch64_OP_IMPLICIT_IMM_0) { - MCInst_insert0(MI, i, MCOperand_CreateImm1(MI, 0)); + MCInst_insert0(MI, j, MCOperand_CreateImm1(MI, 0)); } } diff --git a/arch/AArch64/AArch64GenAsmWriter.inc b/arch/AArch64/AArch64GenAsmWriter.inc index 58e1e0d3..7e8a0e63 100644 --- a/arch/AArch64/AArch64GenAsmWriter.inc +++ b/arch/AArch64/AArch64GenAsmWriter.inc @@ -33535,7 +33535,7 @@ static bool AArch64InstPrinterValidateMCOperand(const MCOperand *MCOp, switch (PredicateIndex) { default: assert(0 && "Unknown MCOperandPredicate kind"); - break; + return false; case 1: { if (!MCOperand_isImm(MCOp)) diff --git a/arch/AArch64/AArch64InstPrinter.c b/arch/AArch64/AArch64InstPrinter.c index 043d351f..f5bcf705 100644 --- a/arch/AArch64/AArch64InstPrinter.c +++ b/arch/AArch64/AArch64InstPrinter.c @@ -1358,7 +1358,7 @@ void printOperand(MCInst *MI, unsigned OpNo, SStream *O) unsigned Reg = MCOperand_getReg(Op); printRegName(O, Reg); } else if (MCOperand_isImm(Op)) { - MCOperand *Op = MCInst_getOperand(MI, (OpNo)); + Op = MCInst_getOperand(MI, (OpNo)); SStream_concat(O, "%s", markup("<imm:")); printInt64Bang(O, MCOperand_getImm(Op)); SStream_concat0(O, markup(">")); diff --git a/arch/AArch64/AArch64Mapping.c b/arch/AArch64/AArch64Mapping.c index bb3f294f..7b2fa560 100644 --- a/arch/AArch64/AArch64Mapping.c +++ b/arch/AArch64/AArch64Mapping.c @@ -1483,7 +1483,7 @@ static void add_cs_detail_template_1(MCInst *MI, aarch64_op_group op_group, case AArch64_OP_GROUP_ZPRasFPR_32: case AArch64_OP_GROUP_ZPRasFPR_64: case AArch64_OP_GROUP_ZPRasFPR_8: { - unsigned Base; + unsigned Base = AArch64_NoRegister; unsigned Width = temp_arg_0; switch (Width) { case 8: diff --git a/arch/ARM/ARMDisassembler.c b/arch/ARM/ARMDisassembler.c index 43d81e87..da91a758 100644 --- a/arch/ARM/ARMDisassembler.c +++ b/arch/ARM/ARMDisassembler.c @@ -2082,7 +2082,7 @@ static DecodeStatus DecodeAddrMode2IdxInstruction(MCInst *Inst, unsigned Insn, unsigned amt = fieldFromInstruction_4(Insn, 7, 5); if (Opc == ARM_AM_ror && amt == 0) Opc = ARM_AM_rrx; - unsigned imm = ARM_AM_getAM2Opc(Op, amt, Opc, idx_mode); + imm = ARM_AM_getAM2Opc(Op, amt, Opc, idx_mode); MCOperand_CreateImm0(Inst, (imm)); } else { diff --git a/arch/ARM/ARMMapping.c b/arch/ARM/ARMMapping.c index 9ec29c2d..19cbcd11 100644 --- a/arch/ARM/ARMMapping.c +++ b/arch/ARM/ARMMapping.c @@ -1354,11 +1354,11 @@ static void add_cs_detail_general(MCInst *MI, arm_op_group op_group, MCInst_getOpVal(MI, OpNum) + 1); break; case ARM_OP_GROUP_RotImmOperand: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); - if (Imm == 0) + unsigned RotImm = MCInst_getOpVal(MI, OpNum); + if (RotImm == 0) return; ARM_get_detail_op(MI, -1)->shift.type = ARM_SFT_ROR; - ARM_get_detail_op(MI, -1)->shift.value = Imm * 8; + ARM_get_detail_op(MI, -1)->shift.value = RotImm * 8; break; } case ARM_OP_GROUP_FBits16: @@ -1390,16 +1390,16 @@ static void add_cs_detail_general(MCInst *MI, arm_op_group op_group, break; } case ARM_OP_GROUP_PostIdxImm8Operand: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); - bool sub = !(Imm & 256); - ARM_set_detail_op_mem_offset(MI, OpNum, (Imm & 0xff), sub); + unsigned Imm8 = MCInst_getOpVal(MI, OpNum); + bool sub = !(Imm8 & 256); + ARM_set_detail_op_mem_offset(MI, OpNum, (Imm8 & 0xff), sub); ARM_get_detail(MI)->post_index = true; break; } case ARM_OP_GROUP_PostIdxImm8s4Operand: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); - bool sub = !(Imm & 256); - ARM_set_detail_op_mem_offset(MI, OpNum, (Imm & 0xff) << 2, sub); + unsigned Imm8s = MCInst_getOpVal(MI, OpNum); + bool sub = !(Imm8s & 256); + ARM_set_detail_op_mem_offset(MI, OpNum, (Imm8s & 0xff) << 2, sub); ARM_get_detail(MI)->post_index = true; break; } @@ -1569,26 +1569,26 @@ static void add_cs_detail_general(MCInst *MI, arm_op_group op_group, ARM_set_mem_access(MI, true); ARM_set_detail_op_mem(MI, OpNum, false, 0, 0, MCInst_getOpVal(MI, OpNum)); - int64_t Imm = MCInst_getOpVal(MI, OpNum + 1); - if (Imm) + int64_t Imm0_1024s4 = MCInst_getOpVal(MI, OpNum + 1); + if (Imm0_1024s4) ARM_set_detail_op_mem(MI, OpNum + 1, false, 0, 0, - Imm * 4); + Imm0_1024s4 * 4); ARM_set_mem_access(MI, false); break; case ARM_OP_GROUP_PKHLSLShiftImm: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); - if (Imm == 0) + unsigned ShiftImm = MCInst_getOpVal(MI, OpNum); + if (ShiftImm == 0) return; ARM_get_detail_op(MI, -1)->shift.type = ARM_SFT_LSL; - ARM_get_detail_op(MI, -1)->shift.value = Imm; + ARM_get_detail_op(MI, -1)->shift.value = ShiftImm; break; } case ARM_OP_GROUP_PKHASRShiftImm: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); - if (Imm == 0) - Imm = 32; + unsigned RShiftImm = MCInst_getOpVal(MI, OpNum); + if (RShiftImm == 0) + RShiftImm = 32; ARM_get_detail_op(MI, -1)->shift.type = ARM_SFT_ASR; - ARM_get_detail_op(MI, -1)->shift.value = Imm; + ARM_get_detail_op(MI, -1)->shift.value = RShiftImm; break; } case ARM_OP_GROUP_ThumbS4ImmOperand: @@ -1596,9 +1596,9 @@ static void add_cs_detail_general(MCInst *MI, arm_op_group op_group, MCInst_getOpVal(MI, OpNum) * 4); break; case ARM_OP_GROUP_ThumbSRImm: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); + unsigned SRImm = MCInst_getOpVal(MI, OpNum); ARM_set_detail_op_imm(MI, OpNum, ARM_OP_IMM, - Imm == 0 ? 32 : Imm); + SRImm == 0 ? 32 : SRImm); break; } case ARM_OP_GROUP_BitfieldInvMaskImmOperand: { @@ -1610,8 +1610,8 @@ static void add_cs_detail_general(MCInst *MI, arm_op_group op_group, break; } case ARM_OP_GROUP_CPSIMod: { - unsigned Imm = MCInst_getOpVal(MI, OpNum); - ARM_get_detail(MI)->cps_mode = Imm; + unsigned Mode = MCInst_getOpVal(MI, OpNum); + ARM_get_detail(MI)->cps_mode = Mode; break; } case ARM_OP_GROUP_CPSIFlag: { @@ -1730,10 +1730,10 @@ static void add_cs_detail_template_1(MCInst *MI, arm_op_group op_group, ARM_set_mem_access(MI, true); ARM_set_detail_op_mem(MI, OpNum, false, 0, 0, MCInst_getOpVal(MI, OpNum)); - int32_t Imm = MCInst_getOpVal(MI, OpNum + 1); - if (Imm == INT32_MIN) - Imm = 0; - ARM_set_detail_op_mem(MI, OpNum + 1, false, 0, 0, Imm); + int32_t Imm8 = MCInst_getOpVal(MI, OpNum + 1); + if (Imm8 == INT32_MIN) + Imm8 = 0; + ARM_set_detail_op_mem(MI, OpNum + 1, false, 0, 0, Imm8); if (AlwaysPrintImm0) map_add_implicit_write(MI, MCInst_getOpVal(MI, OpNum)); @@ -1864,8 +1864,8 @@ static void add_cs_detail_template_2(MCInst *MI, arm_op_group op_group, case ARM_OP_GROUP_ComplexRotationOp_180_90: { unsigned Angle = temp_arg_0; unsigned Remainder = temp_arg_1; - unsigned Imm = (MCInst_getOpVal(MI, OpNum) * Angle) + Remainder; - ARM_set_detail_op_imm(MI, OpNum, ARM_OP_IMM, Imm); + unsigned Rotation = (MCInst_getOpVal(MI, OpNum) * Angle) + Remainder; + ARM_set_detail_op_imm(MI, OpNum, ARM_OP_IMM, Rotation); break; } } diff --git a/arch/HPPA/HPPADisassembler.c b/arch/HPPA/HPPADisassembler.c index a73c6eaf..5da27156 100644 --- a/arch/HPPA/HPPADisassembler.c +++ b/arch/HPPA/HPPADisassembler.c @@ -2776,7 +2776,7 @@ static void fill_copr_mods(uint32_t insn, uint32_t uid, uint32_t class, push_str_modifier(hppa_ext, "n"); } } else { - uint32_t uid = get_insn_field(insn, 23, 25); + uid = get_insn_field(insn, 23, 25); uint32_t sop = (get_insn_field(insn, 6, 22) << 5) | get_insn_field(insn, 27, 31); push_int_modifier(hppa_ext, uid); diff --git a/arch/M68K/M68KDisassembler.c b/arch/M68K/M68KDisassembler.c index a1df81c3..b502e5ae 100644 --- a/arch/M68K/M68KDisassembler.c +++ b/arch/M68K/M68KDisassembler.c @@ -1944,9 +1944,7 @@ static void d68020_cpgen(m68k_info *info) // special handling for fmovecr if (BITFIELD(info->ir, 5, 0) == 0 && BITFIELD(next, 15, 10) == 0x17) { - cs_m68k_op* op0; - cs_m68k_op* op1; - cs_m68k* ext = build_init_op(info, M68K_INS_FMOVECR, 2, 0); + ext = build_init_op(info, M68K_INS_FMOVECR, 2, 0); op0 = &ext->operands[0]; op1 = &ext->operands[1]; diff --git a/cstool/cstool.c b/cstool/cstool.c index 88bc94a1..79585084 100644 --- a/cstool/cstool.c +++ b/cstool/cstool.c @@ -645,8 +645,6 @@ int main(int argc, char **argv) count = cs_disasm(handle, assembly, size, address, 0, &insn); if (count > 0) { - size_t i; - for (i = 0; i < count; i++) { int j; diff --git a/docs/cs_v6_release_guide.md b/docs/cs_v6_release_guide.md index aef1184e..4ac1b2bc 100644 --- a/docs/cs_v6_release_guide.md +++ b/docs/cs_v6_release_guide.md @@ -157,9 +157,9 @@ Write it into `rename_arm64.sh` and run it on files with `sh rename_arm64.sh <sr These features are only supported by `auto-sync`-enabled architectures. -**Instruction Encoding** +**More code quality checks** -TODO +- `clang-tidy` is now run on all files changed by a PR. **Instruction formats for PPC** diff --git a/run-clang-tidy.sh b/run-clang-tidy.sh new file mode 100755 index 00000000..4617884a --- /dev/null +++ b/run-clang-tidy.sh @@ -0,0 +1,30 @@ +#!/bin/sh + +if [ $# -ne 1 ] || [ "$1" = "-h" ] || [ "$1" = "--help" ]; then + echo "$0 <build-path>" + exit 1 +fi + +BUILD_PATH="$1" + +clang-tidy $(find ./arch ./*.c -type f -iregex ".*\.[c]") -p "$BUILD_PATH" -checks=clang-analyzer-*,-clang-analyzer-cplusplus* | tee ct-warnings.txt + +tmp=$(mktemp) +grep ": warning" ct-warnings.txt | grep -oE "^[/a-zA-Z0-9]*\.[ch]" | sort | uniq > $tmp +top_level=$(git rev-parse --show-toplevel) + +echo "\n\n###### REPORT\n\n" + +for modified in $(git diff --name-only origin/next); do + full_path="$top_level/$modified" + if grep -q "$full_path" $tmp; then + echo "$full_path as warnings. Please fix them." + needs_fixes=1 + fi +done + +if [ -z $needs_fixes ]; then + echo "All good" + exit 0 +fi +exit 1 diff --git a/suite/fuzz/fuzz_disasm.c b/suite/fuzz/fuzz_disasm.c index 0dd7ef72..b402e7ee 100644 --- a/suite/fuzz/fuzz_disasm.c +++ b/suite/fuzz/fuzz_disasm.c @@ -60,12 +60,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) { unsigned int n; for (j = 0; j < count; j++) { - cs_insn *i = &(all_insn[j]); + cs_insn *insn = &(all_insn[j]); fprintf(outfile, "0x%"PRIx64":\t%s\t\t%s // insn-ID: %u, insn-mnem: %s\n", - i->address, i->mnemonic, i->op_str, - i->id, cs_insn_name(handle, i->id)); + insn->address, insn->mnemonic, insn->op_str, + insn->id, cs_insn_name(handle, insn->id)); - detail = i->detail; + detail = insn->detail; if (detail->regs_read_count > 0) { fprintf(outfile, "\tImplicit registers read: "); |