Re: [PATCH] sanitizer: do not ICE for pointer cmp/sub

[Jeff Law via Gcc-patches]

On 12/10/20 1:31 AM, Martin Liška wrote:
> Hello.
>
> In C FE we have troubles to instrument top-level pointer comparison
> (and subtraction):
>
> /home/marxin/Programming/gcc/gcc/testsuite/c-c++-common/asan/pr98204.c:5:1:
> internal compiler error: in pointer_diff, at c/c-typeck.c:3954
>     5 | static long i=((char*)&(v.c)-(char*)&v);
>       | ^~~~~~
>
> and
>
>  gcc
> /home/marxin/Programming/gcc/gcc/testsuite/c-c++-common/asan/pr98204.c
> -c -fsanitize=address,pointer-compare
> /home/marxin/Programming/gcc/gcc/testsuite/c-c++-common/asan/pr98204.c:6:16:
> error: initializer element is not constant
>     6 | static long i2=((char*)&(v.c)<(char*)&v);
>       |                ^
>
> The patch fixes that by not instrumenting that when current_function_decl
> is NULL_TREE.
>
> On the contrary, C++ is fine with that and does the emission in ctor:
>
> $ +
> /home/marxin/Programming/gcc/gcc/testsuite/c-c++-common/asan/pr98204.c
> -c -fsanitize=address,pointer-compare,pointer-subtract
> -fdump-tree-gimple=/dev/stdout
> ...
> void __static_initialization_and_destruction_0 (int __initialize_p,
> int __priority)
> {
>   if (__initialize_p == 1) goto <D.2830>; else goto <D.2831>;
>   <D.2830>:
>   __builtin___asan_before_dynamic_init
> ("/home/marxin/Programming/gcc/gcc/testsuite/c-c++-common/asan/pr98204.c");
>   if (__priority == 65535) goto <D.2832>; else goto <D.2833>;
>   <D.2832>:
>   __builtin___sanitizer_ptr_sub (&v.c, &v);
>   i = 0;
>   __builtin___sanitizer_ptr_cmp (&v.c, &v);
>
> Are we able to do something similar for C FE, or are we fine with the
> suggested patch?
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> gcc/c/ChangeLog:
>
>     PR sanitizer/98204
>     * c-typeck.c (pointer_diff): Do not emit a top-level
>     sanitization.
>     (build_binary_op): Likewise.
>
> gcc/testsuite/ChangeLog:
>
>     PR sanitizer/98204
>     * c-c++-common/asan/pr98204.c: New test.
It would be better if we had consistency across C and C++ for this.  But
to make this work for C wouldn't we have to change the static
initialization to a runtime initialization?  Which in turn means we'd
need to set up some kind of ctor?  WHich in turn means that we'd bring
the whole issue of static ctor initialization order to C?  Ewwwww.

I'd be real tempted to just live with the inconsistency and go with your
patch.

jeff