GC analyzer does not spot safepoint issue

[maleadt]

The following piece of C code, reduced from gc.c (ab was gc_cblist_notify_external_free, b was gc_cblist_notify_external_free), confuses the GC analyzer based on the definition of foobar. If it's a function declaration, void foobar() { }, the analyzer doesn't spot the potential safepoint by calling the ab function pointer from JL_NOTSAFEPOINT-annotated f:

#include <stdint.h>
#include <string.h>
#include "analyzer_annotations.h"
typedef struct {
    size_t ae
} af;
typedef struct ag *ah;
typedef struct {
    af ai
} aj;
struct ag {
    aj ak
};
ah *z;
ad, a, e;
typedef struct {
    uint64_t t
} u;
typedef struct {
    uint32_t v[8];
    int aa
} w;
u x;
w y;
typedef (*ab)();
#define b(c, d, ac)                                                            \
    for (; a;)                                                                 \
    ((c)a)()
f() JL_NOTSAFEPOINT
{
    b(ab, , );
}
h()
{
    f();
}
j()
{
    for (unsigned k = 0; k <= y.aa; k++) {
        uint32_t l = y.v[k];
        for (; l; l >>= 1)
            if (m())
                y.v[k] = 0;
        if (y.v[k])
            ;
    }
}
n()
{
    h();
}
o()
{
    size_t p = ad;
    for (int q = 0; q < p; q++)
        j();
}
r()
{
    int g = n();
    foobar();
    o();
    for (int q = 0; q < ad; q++) {
        ah s = z[q];
        if (!g)
            for (int i = 0; i < s; i++)
                for (; i < s->ak.ai.ae; i++)
                    ;
    }
    if (e)
        ;
    e = x.t;
}

$ cat reduce_decl.c
void foobar() { }

$ clang --analyze -Xanalyzer -analyzer-output=text -Xclang -load -Xclang libGCCheckerPlugin.so -Xclang -analyzer-checker=core,julia.GCChecker --analyzer-no-default-checks -include reduce_decl.c reduce.c

If you instead define only a prototype, void foobar();, the analyzer does spot the issue:

$ cat reduce_proto.c
void foobar();

$ clang --analyze -Xanalyzer -analyzer-output=text -Xclang -load -Xclang libGCCheckerPlugin.so -Xclang -analyzer-checker=core,julia.GCChecker --analyzer-no-default-checks -include reduce_proto.c reduce.c

reduce.c:31:5: warning: Calling potential safepoint from function annotated JL_NOTSAFEPOINT
    b(ab, , );
    ^
reduce.c:28:5: note: expanded from macro 'b'
    ((c)a)()
    ^
reduce.c:60:13: note: Calling 'n'
    int g = n();
            ^~~
reduce.c:50:5: note: Calling 'h'
    h();
    ^~~
reduce.c:35:5: note: Calling 'f'
    f();
    ^~~
reduce.c:31:5: note: Loop condition is true.  Entering loop body
    b(ab, , );
    ^
reduce.c:27:5: note: expanded from macro 'b'
    for (; a;)                                                                 \
    ^
reduce.c:31:5: note: Calling potential safepoint from function annotated JL_NOTSAFEPOINT
    b(ab, , );
    ^~~~~~~~~
reduce.c:28:5: note: expanded from macro 'b'
    ((c)a)()
    ^~~~~~~~
1 warning generated.

Removing seemingly trivial stuff that would never affect the analysis makes both cases detect the issue:

$ diff -u /tmp/reduce.c reduce.c                                                                                                                                                                                                     *[master] 
--- /tmp/reduce.c       2019-10-08 15:10:38.394677577 +0200
+++ reduce.c    2019-10-08 15:10:39.581379150 +0200
@@ -64,7 +64,7 @@
         ah s = z[q];
         if (!g)
             for (int i = 0; i < s; i++)
-                for (; i < s->ak.ai.ae; i++)
+                for (; i < s->ak.ai.ae; i--)
                     ;
     }
     if (e)

$ clang --analyze -Xanalyzer -analyzer-output=text -Xclang -load -Xclang libGCCheckerPlugin.so -Xclang -analyzer-checker=core,julia.GCChecker --analyzer-no-default-checks -include reduce_decl.c reduce.c

reduce.c:31:5: warning: Calling potential safepoint from function annotated JL_NOTSAFEPOINT
    b(ab, , );
    ^

Furthermore, defining foobar through -include reduce_proto.c or -include reduce_decl.c seems necessary to reproduce the issue. This wasn't the case before reduction, so it doesn't look like a simple logic issue.

Tested with LLVM 6.0.1 from BinaryBuilder on Julia master. This issue is the reason that the llvmpasses buildbot fails over at #33467. Thoughts, @Keno?

The real fix would be to (document and) annotate the callback function pointer, i.e. typedef (*ab)() JL_NOTSAFEPOINT, which silences the error.

[Keno]

So partly this is by design, because the analyzer is interprocedural, so if it has the body of foobar (and heuristically decides to look at it), it can see that it's not in fact a safepoint and doesn't emit the warning. However, over time I've started giving these error messages more, since the heuristics can just decide not to look at it, in which case a missing annotation would turn into a diagnostics, so TLDR, I think we should fix it, though it's technically working as originally intended.

[Keno]

(Meaning that whether it complains about foobar depends on whether or not it has the definition - Looks like we have a separate issue with that callback function).

[maleadt]

But it's never warning about foobar, only the callbacks get a warning (if foobar is defined).

[Keno]

But it's never warning about foobar, only the callbacks get a warning (if foobar is defined).

Right yeah, I saw that after. That is potentially ok, since the analyzer is heuristic based, so it's possible for it to stop early, but have the introduction of the foobar change the heuristics (it explores more in the vicinity of other errors). It could then determine that the callback issue is more important than the foobar issue (because it has a shorter chain of events) and report that instead.