On Fri, Oct 13, 2017 at 02:53:50PM +0200, Martin Liška wrote: > @@ -3826,6 +3827,19 @@ pointer_diff (location_t loc, tree op0, tree op1) > pedwarn (loc, OPT_Wpointer_arith, > "pointer to a function used in subtraction"); > > + if (sanitize_flags_p (SANITIZE_POINTER_SUBTRACT)) > + { > + gcc_assert (current_function_decl != NULL_TREE); > + > + op0 = save_expr (op0); > + op1 = save_expr (op1); > + > + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT); > + *instrument_expr > + = build_call_expr_loc (loc, tt, 2, c_fully_fold (op0, false, NULL), > + c_fully_fold (op1, false, NULL)); > + }
Why the c_fully_fold? Can't that be deferred until it actually is folded all together later? > + ret = pointer_diff (location, op0, op1, &instrument_expr); > goto return_build_binary_op; > } > /* Handle pointer minus int. Just like pointer plus int. */ > @@ -11707,6 +11721,18 @@ build_binary_op (location_t location, enum tree_code > code, > pedwarn (location, 0, > "comparison of distinct pointer types lacks a cast"); > } > + > + if (sanitize_flags_p (SANITIZE_POINTER_COMPARE)) > + { > + gcc_assert (current_function_decl != NULL_TREE); > + > + op0 = save_expr (op0); > + op1 = save_expr (op1); > + > + tree tt = builtin_decl_explicit (BUILT_IN_ASAN_POINTER_COMPARE); > + instrument_expr > + = build_call_expr_loc (location, tt, 2, op0, op1); > + } Is this the right spot for this? I mean then you don't handle ptr >= 0 or ptr > 0 and similar or ptr >= 0x12345678. I know we have warnings or pedwarns for those, still I think it would be better to handle the above e.g. before if ((TREE_CODE (TREE_TYPE (orig_op0)) == BOOLEAN_TYPE || truth_value_p (TREE_CODE (orig_op0))) ^ (TREE_CODE (TREE_TYPE (orig_op1)) == BOOLEAN_TYPE || truth_value_p (TREE_CODE (orig_op1)))) maybe_warn_bool_compare (location, code, orig_op0, orig_op1); by testing if ((code0 == POINTER_TYPE || code1 == POINTER_TYPE) && sanitize_flags_p (SANITIZE_POINTER_COMPARE)) What about the C++ FE? Or is pointer comparison well defined there? What about pointer subtraction? My memory is fuzzy. > +// { dg-options "-fsanitize=pointer-compare -O0" } Please use -fsanitize=address,pointer-compare etc. in the testcases, so that it is an example to users who don't know we have implicit -fsanitize=address for these tests. > + if (offset <= 2048) { > + if (__asan_region_is_poisoned (left, offset) == 0) I think the LLVM coding conventions don't want a space before ( above. > + // check whether left is a stack memory pointer > + if (GetStackVariableBeginning(left, &shadow_offset1)) { > + if (GetStackVariableBeginning(right - 1, &shadow_offset2) > + && shadow_offset1 == shadow_offset2) Nor && at the beginning of the line (they want it at the end of previous one). > + return; > + else > + goto do_error; > + } If you have goto do_error; for all cases, then you don't need to indent further stuff into else ... further and further. > + // check whether left is a heap memory address > + else { > + HeapAddressDescription hdesc1, hdesc2; > + if (GetHeapAddressInformation(left, 0, &hdesc1) > + && hdesc1.chunk_access.access_type == kAccessTypeInside) { > + if (GetHeapAddressInformation(right, 0, &hdesc2) > + && hdesc2.chunk_access.access_type == kAccessTypeInside > + && (hdesc1.chunk_access.chunk_begin > + == hdesc2.chunk_access.chunk_begin)) > + return; So, here one is a heap object and the other is not. That should be an do_error, right? > + } else { > + // check whether left is an address of a global variable > + GlobalAddressDescription gdesc1, gdesc2; > + if (GetGlobalAddressInformation(left, 0, &gdesc1)) { > + if (GetGlobalAddressInformation(right - 1, 0, &gdesc2) > + && gdesc1.PointsInsideASameVariable (gdesc2)) > + return; > + } else { > + // TODO Either goto do_error; here too, or do the if (offset <= 16384) case here. Guess upstream wouldn't like it with a TODO spot. Jakub