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
