Hi Jakub,

thanks for the further suggestions; updated patch attached.

On 11.09.23 15:34, Jakub Jelinek wrote:
> On Mon, Sep 11, 2023 at 03:21:54PM +0200, Tobias Burnus wrote:
>> +      if (TREE_STATIC (var))
>> +    {
>> +      if (allocator == NULL_TREE && allocator_loc == UNKNOWN_LOCATION)
>> +        error_at (loc, "%<allocator%> clause required for "
>> +                       "static variable %qD", var);
>> +      else if (allocator
>> +               && (tree_int_cst_sgn (allocator) != 1
>> +                   || tree_to_shwi (allocator) > 8))
> Has anything checked that in this case allocator is actually INTEGER_CST
> which fits into shwi?  Otherwise tree_to_shwi will ICE.
> Consider say allocator argument of
> 329857234985743598347598437598347594835743895743wb
> or (((unsigned __int128) 0x123456789abcdef0) << 64)
> Either tree_fits_shwi_p (allocator) check would do it, or perhaps
> else if (allocator
>        && TREE_CODE (allocator) == INTEGER_CST
>        && wi::to_widest (allocator) > 0
>        && wi::to_widest (allocator) <= 8)
> ?
I have now used the latter. Using _BitInt and __int128 fails because the
type-check fails. I have not added them to the testsuite as not all
targets support the two. Using a casted -1 did ICE before I used to
wi::to_widest.
>> +          error_at (OMP_CLAUSE_LOCATION (nl),
>> +                    "allocator variable %qD must be declared before %qD",
>> +                    allocator, var);
...
>> +                 error_at (EXPR_LOCATION (*l),
>> +                           "allocator variable %qD, used in the "
>> +                           "%<allocate%> directive for %qD, must not be "
>> +                           "modified between declaration of %qD and its "
>> +                           "%<allocate%> directive",
>> +                           allocator, var, var);
> BTW, it doesn't necessarily have to be just the simple case which you catch
> here, namely that allocator is a VAR_DECL defined after var in current
> scope.
...
> I bet we can't catch everything, but perhaps e.g. doing the first
> diagnostics from within walk_tree might be better.

Done now. What's not caught is, e.g., a value change by calling a
function which modifies its parameter:

omp_allocator_t a = ...; int v; foo(a); #pragma omp allocate(v) allocator(a)

as the current check is only whether 'a' is declared before 'v' or
whether 'a' is assigned to between v's declaration and the pragma.

Any additional comments or suggestions?

Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955