add gcc optional warnings for duplicated-branches duplicated-cond log…

…ical-op with explanations

compile.py

@@ -7,12 +7,21 @@
 # novice programmers will often be told to ignore scanf's return value
 # when writing their first programs 
 
-COMMON_WARNING_ARGS = "-Wall -Wno-unused -Wunused-variable -Wunused-value -Wno-unused-result".split()
+COMMON_WARNING_ARGS = "-Wall -Wno-unused -Wunused-variable -Wunused-value -Wno-unused-result -Wshadow".split()
 COMMON_COMPILER_ARGS = COMMON_WARNING_ARGS + "-std=gnu11 -g -lm".split()
 
 CLANG_ONLY_ARGS = "-Wunused-comparison -fno-omit-frame-pointer -fno-common -funwind-tables -fno-optimize-sibling-calls -Qunused-arguments".split()
 
-GCC_ARGS = COMMON_COMPILER_ARGS + "-Wunused-but-set-variable -O  -o /dev/null".split()
+# gcc flags some novice programmer mistakes than clang doesn't so
+# we run it has an extra checking pass with several extra warnings enabled
+# -Wduplicated-branches was only added with gcc-8 so it'll break older versions of gcc 
+# but this will be silent - we could fix by  checking gcc version
+#
+# -Wnull-dererefence would be useful here fbut when it flags potential paths
+# the errors look confusing for novice programmers and there appears no way to get only definite null-derefs
+#
+# -O is needed with gcc to get warnings for some things 
+GCC_ONLY_ARGS = "-Wunused-but-set-variable -Wduplicated-cond -Wduplicated-branches -Wlogical-op -O  -o /dev/null".split()
 
 MAXIMUM_SOURCE_FILE_EMBEDDED_BYTES = 1000000
 
@@ -33,9 +42,11 @@ def compile():
 	args = parse_args(sys.argv[1:])
 	# we have to set these explicitly because 
 	clang_args = COMMON_COMPILER_ARGS + CLANG_ONLY_ARGS
+	gcc_args = COMMON_COMPILER_ARGS + GCC_ONLY_ARGS
 	if args.colorize_output:
 		clang_args += ['-fcolor-diagnostics']
 		clang_args += ['-fdiagnostics-color']
+		gcc_args += ['-fdiagnostics-color=always']
 
 	args.unsafe_system_includes = list(args.system_includes_used - DUAL_SANITIZER_SAFE_SYSTEM_INCLUDES)
 
@@ -196,10 +207,10 @@ def compile():
 	# so run gcc as well if available
 
 	if not compiler_stdout and search_path('gcc') and 'gcc' not in args.c_compiler and not args.object_files_being_linked:
-		command = ['gcc'] + args.user_supplied_compiler_args + GCC_ARGS
+		command = ['gcc'] + args.user_supplied_compiler_args + gcc_args
 		if args.debug:
 			print("compiling with gcc for extra checking", file=sys.stderr)
-		execute_compiler(command, '', args, rename_functions=False)	
+		execute_compiler(command, '', args, rename_functions=False, checking_only=True)	
 
 	sys.exit(0)
 
@@ -243,7 +254,7 @@ def update_source(sanitizer, sanitizer_n, wrapper_source, tar_source,  args, cla
 	return wrapper_source, sanitizer_args
 
 
-def execute_compiler(base_command, compiler_stdin, args, rename_functions=True, print_stdout=True, debug_wrapper_file="tmp_dcc_sanitizer1.c"):
+def execute_compiler(base_command, compiler_stdin, args, rename_functions=True, print_stdout=True, debug_wrapper_file="tmp_dcc_sanitizer1.c", checking_only=False):
 	command = list(base_command)
 	if compiler_stdin:
 		if rename_functions and not args.unsafe_system_includes:
@@ -285,14 +296,32 @@ def execute_compiler(base_command, compiler_stdin, args, rename_functions=True,
 	process = subprocess.run(command, input=input, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
 	stdout = codecs.decode(process.stdout, 'utf8', errors='replace')
 
+	if checking_only:
+		# we are running gcc as an extra checking phase
+		# and options don't match the gcc version so give up silently
+		if 'command line' in stdout:
+			if args.debug:
+				print(stdout, end='', file=sys.stderr)
+			return ''
+
+		# checking is run afterwe have already successfully generated an executable with clang
+		# so if we can get an error, unlink the executable
+		if process.returncode:
+			try:
+				os.unlink(args.object_filename)
+			except OSError as e:
+				if args.debug:
+					print(e)
+
+
 	# avoid a confusing mess of linker errors
 	if "undefined reference to `main" in stdout:
 		print("Error: your program does not contain a main function - a C program must contain a main function", file=sys.stderr)
 		sys.exit(1)
 
 	# workaround for  https://github.com/android-ndk/ndk/issues/184
 	# when not triggered earlier    
-	if "undefined reference to `__mul" in stdout:
+	if "undefined reference to `__mul" in stdout and not checking_only:
 		command = [c for c in command if not c in ['-fsanitize=undefined', '-fno-sanitize-recover=undefined,integer']]
 		if args.debug:
 			print("undefined reference to `__mulodi4'", file=sys.stderr)
@@ -303,12 +332,12 @@ def execute_compiler(base_command, compiler_stdin, args, rename_functions=True,
 	# a user call to a renamed unistd.h function appears to be undefined
 	# so recompile without renames
 
-	if rename_functions and "undefined reference to `__renamed_" in stdout:
+	if rename_functions and "undefined reference to `__renamed_" in stdout and not checking_only:
 		if args.debug:
 			print("undefined reference to `__renamed_' recompiling without -D renames", file=sys.stderr)
 		return execute_compiler(base_command, compiler_stdin, args, rename_functions=False, print_stdout=print_stdout, debug_wrapper_file=debug_wrapper_file)
 
-	if process.stdout and print_stdout:
+	if stdout and print_stdout:
 		if args.explanations:
 			explain_compiler_output(stdout, args)
 		else:
@@ -328,6 +357,7 @@ class Args(object):
 	shared_libasan = None
 	stack_use_after_return = None
 	incremental_compilation = False
+	treat_warnings_as_errors = False
 	leak_check = False
 	suppressions_file = os.devnull
 	user_supplied_compiler_args = []
@@ -343,6 +373,7 @@ class Args(object):
 	object_files_being_linked = False
 	libraries_being_linked = False
 	threads_used = False
+	object_filename = "a.out"
 	system_includes_used = set()
 	ifdef_instead_of_wrap = sys.platform == "darwin" # ld reportedly doesn't have wrap on Darwin
 
@@ -445,13 +476,17 @@ def parse_clang_arg(arg, next_arg, args):
 		args.incremental_compilation = True
 	elif arg.startswith('-l'):
 		args.libraries_being_linked = True
+	elif arg == '-Werror':
+		args.treat_warnings_as_errors = True
 	elif arg == '-pthreads':
 		args.threads_used = True
-	elif arg == '-o' and next_arg:
-		object_filename = next_arg
-		if object_filename.endswith('.c') and os.path.exists(object_filename):
-			print("%s: will not overwrite %s with machine code" % (os.path.basename(sys.argv[0]), object_filename), file=sys.stderr)
-			sys.exit(1)
+	elif arg.startswith('-o'):
+		object_filename = arg[2:] or next_arg
+		if object_filename:
+			args.object_filename = object_filename
+			if object_filename.endswith('.c') and os.path.exists(object_filename):
+				print("%s: will not overwrite %s with machine code" % (os.path.basename(sys.argv[0]), object_filename), file=sys.stderr)
+				sys.exit(1)
 	else:
 		process_possible_source_file(arg, args)
 

compiler_explanations.py

@@ -79,12 +79,14 @@ def get_short_explanation(self, message, colorize_output):
 		if colorize_output:
 			color = colors.color
 		else:
-			color = lambda text, color_name: text
+			color = lambda text, *args, **kwargs: text
 
 		parameters = dict((name, getattr(message, name)) for name in dir(message) if not name.startswith('__'))
 		parameters['match'] = match
 		parameters['color'] = color
-		parameters['emphasize'] = lambda text: color(text, 'red')
+		parameters['emphasize'] = lambda text: color(text, style='bold')
+		parameters['danger'] = lambda text: color(text, color='red', style='bold')
+		parameters['info'] = lambda text: color(text, 'cyan', style='bold')
 		return eval('f"' + self.explanation.replace('\n', '\\n') +'"',  globals(), parameters)
 
 explanations = [
@@ -450,14 +452,138 @@ def get_short_explanation(self, message, colorize_output):
 
 		regex = r"indirection requires pointer operand \('(.*)' invalid\)",
 
-		explanation = """ you are trying to use '{emphasize(underlined_word)}' as a pointer.
+		explanation = """You are trying to use '{emphasize(underlined_word)}' as a pointer.
   You can not do this because '{emphasize(underlined_word)}' is of type {emphasize(match.group(1))}.
 """,
 
 		reproduce = """
 int main(int argc, char *argv[]) {
 	return *argc;
 }
+""",
+	),
+
+	Explanation(
+		label = 'duplicated-cond',
+
+		regex = r"duplicated .*\bif\b.* condition",
+
+		explanation = """You have repeated the same condition in a chain of if statements.
+Only the first if statement using the condition can be executed.
+The others can never be executed.
+
+""",
+
+		reproduce = """
+int main(int argc, char *argv[]) {
+	if (argc == 1)
+		return 42;
+	else if (argc == 1)
+    	return 43;
+ 	else
+		return 44;
+}
+""",
+	),
+
+	Explanation(
+		label = 'duplicated-branches',
+
+		regex = r"condition has identical branches",
+
+		explanation = """Your if statement has identical then and else parts.
+It is pointless to have an if statement which executes the same code
+when its condition is true and also when its condition is false.
+
+""",
+
+		reproduce = """
+int main(int argc, char *argv[]) {
+	if (argc == 1)
+		return 42;
+	else 
+    	return 42;
+}
+""",
+	),
+
+	Explanation(
+		label = 'logical-or-always-true',
+
+		regex = r"logical .?\bor\b.* is always true",
+
+		explanation = """Your '{emphasize('||')}' expression is always true, no matter what value variables have.
+Perhaps you meant to use '{emphasize('&&')}' ?
+
+""",
+
+		reproduce = """
+int main(int argc, char *argv[]) {
+	if (argc > 1 || argc < 3)
+		return 42;
+	else 
+    	return 43;
+}
+""",
+	),
+
+	Explanation(
+		label = 'logical-and-always-false',
+
+		regex = r"logical .?\band\b.* is always false",
+
+		explanation = """Your '{emphasize('&&')}' expression is always false, no matter what value variables have.
+Perhaps you meant to use '{emphasize('||')}' ?
+
+""",
+
+		reproduce = """
+int main(int argc, char *argv[]) {
+	if (argc > 1 && argc < 1)
+		return 42;
+	else 
+    	return 43;
+}
+""",
+	),
+
+	Explanation(
+		label = 'logical-equal-expressions',
+
+		regex = r"logical .?((and|or)).? of equal expressions",
+
+		explanation = """Your have used '{emphasize(highlighted_word)}' with same lefthand and righthand operands.
+If this what you meant, it can be simplified: {emphasize('x ' + highlighted_word + ' x')} can be replaced with just {emphasize('x')}.
+
+""",
+
+		reproduce = """
+int main(int argc, char *argv[]) {
+	if (argc > 1 ||argc > 1)
+		return 42;
+	else 
+    	return 43;
+}
+""",
+	),
+
+	Explanation(
+		label = 'shadow-local-variable',
+
+		regex = r"declaration shadows a local variable",
+
+		explanation = """Your already have a variable named '{emphasize(highlighted_word)}'.
+It is confusing to have a second overlapping declaration of the same variable name.
+
+""",
+
+		reproduce = """
+int main(int argc, char *argv[]) {
+	{
+		int argc = 42;
+		return argc;
+	}
+}
 """,
 	),
 ]

tests/expected_output/clang-6.0-x86_64-pc-linux-gnu/duplicated-branches.txt

@@ -0,0 +1,10 @@
+tests/extracted_compile_time_errors/duplicated-branches.c: In function ‘main’:
+tests/extracted_compile_time_errors/duplicated-branches.c:3:5: warning: this condition has identical branches [-Wduplicated-branches]
+  if (argc == 1)
+     ^
+dcc explanation: Your if statement has identical then and else parts.
+It is pointless to have an if statement which executes the same code
+when the condition is true and when it is false.
+
+
+

tests/expected_output/clang-6.0-x86_64-pc-linux-gnu/duplicated-cond.txt

@@ -0,0 +1,13 @@
+tests/extracted_compile_time_errors/duplicated-cond.c: In function ‘main’:
+tests/extracted_compile_time_errors/duplicated-cond.c:5:16: warning: duplicated ‘if’ condition [-Wduplicated-cond]
+  else if (argc == 1)
+           ~~~~~^~~~
+tests/extracted_compile_time_errors/duplicated-cond.c:3:11: note: previously used here
+  if (argc == 1)
+      ~~~~~^~~~
+dcc explanation: You have repeated the same condition in a chain of if statements.
+Only the first if statement using the condition can be executed.
+The others can never be executed.
+
+
+

tests/expected_output/clang-6.0-x86_64-pc-linux-gnu/logical-and-always-false.txt

@@ -0,0 +1,9 @@
+tests/extracted_compile_time_errors/logical-and-always-false.c: In function ‘main’:
+tests/extracted_compile_time_errors/logical-and-always-false.c:3:15: warning: logical ‘and’ of mutually exclusive tests is always false [-Wlogical-op]
+  if (argc > 1 && argc < 1)
+               ^~
+dcc explanation: Your '&&' expression is always false, no matter what value variables have.
+Perhaps you meant to use '||' ?
+
+
+

tests/expected_output/clang-6.0-x86_64-pc-linux-gnu/logical-equal-expressions.txt

@@ -0,0 +1,9 @@
+tests/extracted_compile_time_errors/logical-equal-expressions.c: In function ‘main’:
+tests/extracted_compile_time_errors/logical-equal-expressions.c:3:15: warning: logical ‘or’ of equal expressions [-Wlogical-op]
+  if (argc > 1 ||argc > 1)
+               ^~
+dcc explanation: Your have used '||' with same lefthand and righthand operands.
+If this what you meant, it can be simplified: x || x can be replaced with just x.
+
+
+

tests/expected_output/clang-6.0-x86_64-pc-linux-gnu/logical-or-always-true.txt

@@ -0,0 +1,9 @@
+tests/extracted_compile_time_errors/logical-or-always-true.c: In function ‘main’:
+tests/extracted_compile_time_errors/logical-or-always-true.c:3:15: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
+  if (argc > 1 || argc < 3)
+               ^~
+dcc explanation: Your '||' expression is always true, no matter what value variables have.
+Perhaps you meant to use '&&' ?
+
+
+

tests/expected_output/clang-6.0-x86_64-pc-linux-gnu/shadow-local-variable.txt

@@ -0,0 +1,11 @@
+tests/extracted_compile_time_errors/shadow-local-variable.c:4:7: warning: declaration shadows a local variable [-Wshadow]
+                int argc = 42;
+                    ^
+tests/extracted_compile_time_errors/shadow-local-variable.c:2:14: note: previous declaration is here
+int main(int argc, char *argv[]) {
+             ^
+dcc explanation: Your already have a variable named 'argc'.
+It is confusing to have a second overlapping declaration of the same variable name.
+
+
+

tests/expected_output/clang-7.0-x86_64-pc-linux-gnu/duplicated-branches.txt

@@ -0,0 +1,10 @@
+tests/extracted_compile_time_errors/duplicated-branches.c: In function ‘main’:
+tests/extracted_compile_time_errors/duplicated-branches.c:3:5: warning: this condition has identical branches [-Wduplicated-branches]
+  if (argc == 1)
+     ^
+dcc explanation: Your if statement has identical then and else parts.
+It is pointless to have an if statement which executes the same code
+when the condition is true and when it is false.
+
+
+

tests/expected_output/clang-7.0-x86_64-pc-linux-gnu/duplicated-cond.txt

@@ -0,0 +1,13 @@
+tests/extracted_compile_time_errors/duplicated-cond.c: In function ‘main’:
+tests/extracted_compile_time_errors/duplicated-cond.c:5:16: warning: duplicated ‘if’ condition [-Wduplicated-cond]
+  else if (argc == 1)
+           ~~~~~^~~~
+tests/extracted_compile_time_errors/duplicated-cond.c:3:11: note: previously used here
+  if (argc == 1)
+      ~~~~~^~~~
+dcc explanation: You have repeated the same condition in a chain of if statements.
+Only the first if statement using the condition can be executed.
+The others can never be executed.
+
+
+