On 11/7/25 00:46, Andrew Pinski wrote:
On Thu, Nov 6, 2025 at 6:22 PM Andrew MacLeod <[email protected]> wrote:
I can add a check in the inferred range manager for atomic loads to
resolve this PR.
The IL sequence tends to look like:
_1 = &this_2(D)->b;
__atomic_load_8 (_1, 0);
Just want to make sure I get this right since memory operations are not
my strong suit.
The first argument to the atomic load is non-null (so _1), as well as
the base of the RHS of the address expression that defines _1 are
non-zero?. (this_2)
The attached patch scavenges a little code from
fold_using_range::range_of_address that I think works... but perhaps
there is a more efficient way to do this? Is this likely to work OK and
be safe? or are there additional checks I need to be doing?
And I suppose there is an entire range of atomic operations this applies
to? certainly atomic_store should qualify...
Bootstraps on x86_64-pc-linux-gnu with no regressions, but I'm not sure
that is a really extensive test for this..
Isn't the other route on fixing this is by adding the nonnull
attribute to these builtins? I tried looking to see if anyone had
mentioned why these builtins didn't have the nonnull attribute on them
but I didn't find anything.
The nonnull attribute support for builtins has been there since 2002
and atomic builtins were added afterwards (towards 2011).
DEF_ATTR_TREE_LIST (ATTR_NONNULL_1_NOTHROW_LEAF_LIST, ATTR_NONNULL, \
ATTR_LIST_1, ATTR_NOTHROW_LEAF_LIST)
DEF_ATTR_TREE_LIST (ATTR_NONNULL_1_LEAF_LIST , ATTR_NONNULL, \
ATTR_LIST_1, ATTR_LEAF_LIST )
#define ATTR_NONNULL_1_NOTHROWCALL_LEAF_LIST (flag_non_call_exceptions ? \
ATTR_NONNULL_1_LEAF_LIST : ATTR_NONNULL_1_NOTHROW_LEAF_LIST)
And then use ATTR_NONNULL_1_NOTHROWCALL_LEAF_LIST instead of
ATTR_NOTHROWCALL_LEAF_LIST in DEF_SYNC_BUILTIN for the builtins in
sync-builtins.def that are non null for the 1st argument.
I can only think that nobody has looked into the code generation
enough here to make a big difference. This would also handle isolate
paths too without any extra coding so I suspect special casing these
builtins everywhere is not a route which we want just for nonnullness.
Thanks,
Andrew
_1 = &this_2(D)->b;
__atomic_load_8 (_1, 0);
That would tag _1 as non_null, but that won't tag 'this_2' as
non-zero. I think the goal is to treat the use of _1 in the
__atomic_load as if it was dereferenced...
ie
struct d {
long a;
long b;
};
void call ();
void call2 ();
void fake (long *) __attribute__((nonnull));
void q (d *j)
{
long *p = &j->b;
fake (p);
if (j)
call ();
}
Replicates a similar thing. we know p is non-null, but never figure
out that j is. I guess we do not know that p is dereferenced in the
call to fake().
If however we add a derefernce:
void t (d *j)
{
long *p = &j->b;
fake (p);
if (*p)
call2 ()
if (j)
call ();
}
we *DO* know that j is non-null due to the
_1 = MEM[(long int *)j_4(D) + 8B];
That is introduced by the de-reference, and the 'if (j)' is removed.
So I think the goal is to treat the argument to the atomic calls as if
they are memory de-references, not just non-null? I don't suppose we
have anything like that already available :-P
Which means I probably need to do the same thing that the
non_null_loadstore () callback does at the very least.
Does any of this make sense?
Andrew
