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

.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

.gitignore

@@ -131,7 +131,7 @@ fuzz_bindisasm
 fuzz_disasm
 fuzz_decode_platform
 capstone_get_setup
-suite/fuzz/
+suite/fuzz/corpus
 suite/cstest/cmocka/
 
 *.s

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()
 
 

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

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

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

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(">"));

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:

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 {

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,36 +1569,36 @@ 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:
 		ARM_set_detail_op_imm(MI, OpNum, ARM_OP_IMM,
 				      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;
 	}
 	}

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

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];

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;
 

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

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

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: ");