https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92116
Thomas Schwinge <tschwinge at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Keywords|openacc |
Component|libgomp |tree-optimization
Assignee|jules at gcc dot gnu.org |msebor at gcc dot
gnu.org
--- Comment #2 from Thomas Schwinge <tschwinge at gcc dot gnu.org> ---
Martin Sebor, regarding this:
(In reply to myself from comment #0)
> As reported in
> <http://mid.mail-archive.com/58cdb016-4c82-a271-cbc5-1ede344fdad3@gmail.com>:
>
> | PS I tried compiling GCC with [a new] patch. It fails in libgomp
> | with:
> |
> | libgomp/oacc-mem.c: In function ‘gomp_acc_remove_pointer’:
> | cc1: warning: invalid use of a null pointer [-Wnonnull]
> |
> | so clearly it's missing location information. With
> | -Wnull-dereference enabled we get more detail:
> |
> | libgomp/oacc-mem.c: In function ‘gomp_acc_remove_pointer’:
> | libgomp/oacc-mem.c:1013:31: warning: potential null pointer dereference
> [-Wnull-dereference]
> | 1013 | for (size_t i = 0; i < t->list_count; i++)
> | | ~^~~~~~~~~~~~
> | libgomp/oacc-mem.c:1012:19: warning: potential null pointer dereference
> [-Wnull-dereference]
> | 1012 | t->refcount = minrefs;
> | | ~~~~~~~~~~~~^~~~~~~~~
> | libgomp/oacc-mem.c:1013:31: warning: potential null pointer dereference
> [-Wnull-dereference]
> | 1013 | for (size_t i = 0; i < t->list_count; i++)
> | | ~^~~~~~~~~~~~
> | libgomp/oacc-mem.c:1012:19: warning: potential null pointer dereference
> [-Wnull-dereference]
> | 1012 | t->refcount = minrefs;
> | | ~~~~~~~~~~~~^~~~~~~~~
> | cc1: warning: invalid use of a null pointer [-Wnonnull]
> |
> | I didn't spend too long examining the code but it seems like
> | the warnings might actually be justified. When the first loop
> | terminates with t being null
It's actually not possible to terminate this loop with 't = NULL'. ;-)
As I understand this, what your compiler analysis pass has difficulties to
figure out (from the code as written in 'gomp_acc_remove_pointer'?) is that
here:
> | the subsequent dereferences are
> | invalid:
> |
> | if (t->refcount == minrefs)
> | {
> | /* This is the last reference, so pull the descriptor off the
> | chain. This prevents gomp_unmap_vars via gomp_unmap_tgt from
> | freeing the device memory. */
> | struct target_mem_desc *tp;
> | for (tp = NULL, t = acc_dev->openacc.data_environ; t != NULL;
> | tp = t, t = t->prev)
> | {
> | if (n->tgt == t)
> | {
> | if (tp)
> | tp->prev = t->prev;
> | else
> | acc_dev->openacc.data_environ = t->prev;
> | break;
> | }
> | }
> | }
... we are always going to eventually find 'n->tgt == t': we start this loop
with 't == n->tgt' (see further above), and what this code here is doing is
just to locate 'n->tgt' in 'data_environ', which is guaranteed to contain it
(yes, there should be some 'assert' or similar for that...), and then un-link
it from 'data_environ'. Then we 'break', and thus after the loop we again have
the original 't = n->tgt', and so here:
> | /* Set refcount to 1 to allow gomp_unmap_vars to unmap it. */
> | n->refcount = 1;
> | t->refcount = minrefs;
> | for (size_t i = 0; i < t->list_count; i++)
..., there will never be 't == NULL'.
Certainly this code is a bit "strange" -- but maybe that's something your
analysis has to consider for "real-world code"? I'm assigning this PR to you
in case you'd like to track this, otherwise please set RESOLVED, INVALID or
something like that.
..., and that said, we shall be removing this code from
'gomp_acc_remove_pointer' any moment now... ;-)
