Add a clang-tidy checks and warnings (#2312)

2024-11-23 05:29:53 +00:00 · 2024-04-26 07:11:46 +00:00 · 2024-04-26 07:11:46 +00:00 · 6c7b54817f
commit 6c7b54817f
parent c4d0993071
16 changed files with 123 additions and 56 deletions
--- a/.github/workflows/clang-tidy.yml
+++ 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
--- a/.gitignore
+++ b/.gitignore
@ -131,7 +131,7 @@ fuzz_bindisasm
 fuzz_disasm
 fuzz_decode_platform
 capstone_get_setup
-suite/fuzz/
+suite/fuzz/corpus
 suite/cstest/cmocka/

 *.s
--- 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()


--- 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
--- 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));
 			}
 		}

--- 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))
--- 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(">"));
--- 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:
--- 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 {
--- 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;
 	}
 	}
--- 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);
--- 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];
--- 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;

--- 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**

--- a/run-clang-tidy.sh
+++ 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
--- 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: ");