gcc warnings when compiling isl

[Michael Kruse]

A bug report was filed for Polly regarding warnings when compiling isl:
https://llvm.org/PR51945

Also reproducible without Polly/rpmbuild:

$ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0

$ ./configure CPPFLAGS=-Wall
$ make
home/meinersbur/src/isl/pip.c: In function ‘move_parameters’:
/home/meinersbur/src/isl/pip.c:115:13: warning: suggest parentheses
around comparison in operand of ‘|’ [-Wparentheses]
115 | if (nparam < 0 | nparam_bset < 0)
| ~~~~~~~^~~
/home/meinersbur/src/isl/pip.c: In function ‘main’:
/home/meinersbur/src/isl/pip.c:75:10: warning: ‘context_copy’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
75 | return isl_basic_set_free(bset);
| ^~~~~~~~~~~~~~~~~~~~~~~~
/home/meinersbur/src/isl/pip.c:339:48: note: ‘context_copy’ was declared here
339 | struct isl_basic_set *context, *bset, *copy, *context_copy;
| ^~~~~~~~~~~~
/home/meinersbur/src/isl/pip.c:339:41: warning: ‘copy’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
339 | struct isl_basic_set *context, *bset, *copy, *context_copy;
| ^~~~
/home/meinersbur/src/isl/bound.c: In function ‘main’:
/home/meinersbur/src/isl/bound.c:241:27: warning: ‘copy’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
241 | isl_pw_qpolynomial_fold *copy;
| ^~~~
In file included from /home/meinersbur/src/isl/basis_reduction_tab.c:112:
/home/meinersbur/src/isl/basis_reduction_templ.c: In function
‘isl_tab_compute_reduced_basis’:
/home/meinersbur/src/isl/basis_reduction_templ.c:203:6: warning: ‘row’
may be used uninitialized in this function [-Wmaybe-uninitialized]
203 | save_alpha(lp, row-i, i, alpha_buffer[j]);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/meinersbur/src/isl/isl_sample.c: In function ‘basic_set_sample’:
/home/meinersbur/src/isl/isl_sample.c:1156:18: warning: variable ‘ctx’
set but not used [-Wunused-but-set-variable]
1156 | struct isl_ctx *ctx;
| ^~~
/home/meinersbur/src/isl/isl_schedule_tree.c: In function
‘isl_schedule_tree_plain_is_equal’:
/home/meinersbur/src/isl/isl_stride.c: In function ‘set_stride’:
/home/meinersbur/src/isl/isl_ast_build.c: In function
‘isl_ast_build_insert_dim’:
/home/meinersbur/src/isl/isl_sample.c: In function ‘isl_set_sample_point’:
/home/meinersbur/src/isl/isl_space.c: In function ‘name_ok’:
/home/meinersbur/src/isl/isl_schedule_tree.c:683:5: warning: ‘equal’
may be used uninitialized in this function [-Wmaybe-uninitialized]
683 | if (equal < 0 || !equal)
| ^
/home/meinersbur/src/isl/isl_stride.c:146:6: warning: variable ‘pos’
set but not used [-Wunused-but-set-variable]
146 | int pos;
| ^~~
/home/meinersbur/src/isl/isl_ast_build.c:1556:10: warning: ‘space’ may
be used uninitialized in this function [-Wmaybe-uninitialized]
1556 | space = isl_space_map_from_set(space);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/meinersbur/src/isl/isl_sample.c:1334:9: warning: ‘pnt’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
1334 | return pnt;
| ^~~
/home/meinersbur/src/isl/isl_space.c:479:7: warning: variable ‘dummy’
set but not used [-Wunused-but-set-variable]
479 | long dummy;
| ^~~~~
/home/meinersbur/src/isl/isl_aff.c: In function ‘isl_aff_drop_dims’:
/home/meinersbur/src/isl/isl_schedule_tree.c: In function ‘initial_domain’:
/home/meinersbur/src/isl/isl_map_simplify.c: In function
‘isl_basic_map_drop_redundant_divs_ineq’:
/home/meinersbur/src/isl/isl_ast_build.c:1646:13: note: ‘space’ was
declared here
1646 | isl_space *space, *ma_space;
| ^~~~~
/home/meinersbur/src/isl/isl_scheduler.c: In function ‘compute_schedule_wcc’:
/home/meinersbur/src/isl/isl_aff.c:2593:11: warning: variable ‘ctx’
set but not used [-Wunused-but-set-variable]
2593 | isl_ctx *ctx;
| ^~~
/home/meinersbur/src/isl/isl_schedule_tree.c:1942:17: warning:
‘domain’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
1942 | isl_union_set *domain;
| ^~~~~~
/home/meinersbur/src/isl/isl_map_simplify.c:4759:5: warning:
‘last_neg’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
4759 | if (ineq1 > ineq2) {
| ^
/home/meinersbur/src/isl/isl_map.c: In function
‘isl_map_partial_lexopt_aligned_pw_multi_aff’:
/home/meinersbur/src/isl/isl_scheduler.c:6089:43: warning: ‘best_dist’
may be used uninitialized in this function [-Wmaybe-uninitialized]
6089 | if (best_weight == weight && best_dist <= dist)
| ~~~~~~~~~~^~~~~~~
/home/meinersbur/src/isl/isl_aff.c: In function ‘isl_aff_insert_dims’:
/home/meinersbur/src/isl/isl_schedule_tree.c: In function
‘subtree_schedule_extend’:
/home/meinersbur/src/isl/isl_map_simplify.c:5031:17: note: ‘last_neg’
was declared here
5031 | int last_pos, last_neg;
| ^~~~~~~~
/home/meinersbur/src/isl/isl_map.c:7288:11: warning: ‘todo’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
7288 | todo = isl_set_intersect(todo, *empty);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/meinersbur/src/isl/isl_scheduler.c:6064:31: note: ‘best_dist’
was declared here
6064 | int i, best = graph->n_edge, best_dist, best_weight;
| ^~~~~~~~~
/home/meinersbur/src/isl/isl_aff.c:2672:11: warning: variable ‘ctx’
set but not used [-Wunused-but-set-variable]
2672 | isl_ctx *ctx;
| ^~~
/home/meinersbur/src/isl/isl_schedule_tree.c:1836:17: warning: ‘umap’
may be used uninitialized in this function [-Wmaybe-uninitialized]
1836 | isl_union_map *umap;
| ^~~~
/home/meinersbur/src/isl/isl_map_simplify.c:4745:2: warning:
‘last_pos’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
4745 | isl_basic_map_inequality_to_equality(bmap, ineq);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/meinersbur/src/isl/isl_scheduler.c:6089:20: warning:
‘best_weight’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
6089 | if (best_weight == weight && best_dist <= dist)
| ~~~~~~~~~~~~^~~~~~~~~
/home/meinersbur/src/isl/isl_schedule_tree.c: In function
‘isl_schedule_tree_get_subtree_schedule_union_map’:
/home/meinersbur/src/isl/isl_map_simplify.c:5031:7: note: ‘last_pos’
was declared here
5031 | int last_pos, last_neg;
| ^~~~~~~~
/home/meinersbur/src/isl/isl_scheduler.c:6064:42: note: ‘best_weight’
was declared here
6064 | int i, best = graph->n_edge, best_dist, best_weight;
| ^~~~~~~~~~~
/home/meinersbur/src/isl/isl_schedule_tree.c:2026:9: warning: ‘umap’
may be used uninitialized in this function [-Wmaybe-uninitialized]
2026 | return subtree_schedule_extend(tree, umap);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

$ ./isl_cat --version
isl-0.24-217-gbd68de91-GMP

Michael

--
Tardyzentrismus verboten!

[Sven Verdoolaege]

On Thu, Sep 23, 2021 at 04:43:11PM -0500, Michael Kruse wrote:
> A bug report was filed for Polly regarding warnings when compiling isl:
> https://llvm.org/PR51945

Thanks. I'll have a look soon.
At least some of them look like real issues.

skimo

[Sven Verdoolaege]

Actually, it's mostly harmless.
There's one typo (| instead of ||) and a couple of unused variables.
I'll send out patches for those
The rest looks spurious.

On Thu, Sep 23, 2021 at 04:43:11PM -0500, Michael Kruse wrote:

> /home/meinersbur/src/isl/pip.c: In function ‘main’:
> /home/meinersbur/src/isl/pip.c:75:10: warning: ‘context_copy’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
> 75 | return isl_basic_set_free(bset);
> | ^~~~~~~~~~~~~~~~~~~~~~~~
> /home/meinersbur/src/isl/pip.c:339:48: note: ‘context_copy’ was declared here
> 339 | struct isl_basic_set *context, *bset, *copy, *context_copy;
> | ^~~~~~~~~~~~
> /home/meinersbur/src/isl/pip.c:339:41: warning: ‘copy’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> 339 | struct isl_basic_set *context, *bset, *copy, *context_copy;
> | ^~~~
> /home/meinersbur/src/isl/bound.c: In function ‘main’:
> /home/meinersbur/src/isl/bound.c:241:27: warning: ‘copy’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> 241 | isl_pw_qpolynomial_fold *copy;
> | ^~~~

Looks fine too me. Perhaps gcc is assuming options->verify may change
in between.

> In file included from /home/meinersbur/src/isl/basis_reduction_tab.c:112:
> /home/meinersbur/src/isl/basis_reduction_templ.c: In function
> ‘isl_tab_compute_reduced_basis’:
> /home/meinersbur/src/isl/basis_reduction_templ.c:203:6: warning: ‘row’
> may be used uninitialized in this function [-Wmaybe-uninitialized]
> 203 | save_alpha(lp, row-i, i, alpha_buffer[j]);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

AFAICT, row is always initialized.

> /home/meinersbur/src/isl/isl_schedule_tree.c:683:5: warning: ‘equal’
> may be used uninitialized in this function [-Wmaybe-uninitialized]
> 683 | if (equal < 0 || !equal)
> | ^

equal is set in all cases

> /home/meinersbur/src/isl/isl_ast_build.c:1556:10: warning: ‘space’ may
> be used uninitialized in this function [-Wmaybe-uninitialized]
> 1556 | space = isl_space_map_from_set(space);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I guess there is a similar issue with build->node here.
Does it help if "!build->node" is stored in a local variable and reused?

> /home/meinersbur/src/isl/isl_sample.c:1334:9: warning: ‘pnt’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
> 1334 | return pnt;
> | ^~~

set->n is non-negative, so it's always set.

> /home/meinersbur/src/isl/isl_schedule_tree.c:1942:17: warning:
> ‘domain’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 1942 | isl_union_set *domain;
> | ^~~~~~

Set in all relevant cases.

> /home/meinersbur/src/isl/isl_map_simplify.c:4759:5: warning:
> ‘last_neg’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 4759 | if (ineq1 > ineq2) {
> | ^

Always set.

> /home/meinersbur/src/isl/isl_map.c: In function
> ‘isl_map_partial_lexopt_aligned_pw_multi_aff’:
> /home/meinersbur/src/isl/isl_scheduler.c:6089:43: warning: ‘best_dist’
> may be used uninitialized in this function [-Wmaybe-uninitialized]
> 6089 | if (best_weight == weight && best_dist <= dist)
> | ~~~~~~~~~~^~~~~~~

Always set under given conditions.

> /home/meinersbur/src/isl/isl_map.c:7288:11: warning: ‘todo’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
> 7288 | todo = isl_set_intersect(todo, *empty);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Always set under given condition.

> /home/meinersbur/src/isl/isl_schedule_tree.c:1836:17: warning: ‘umap’
> may be used uninitialized in this function [-Wmaybe-uninitialized]
> 1836 | isl_union_map *umap;
> | ^~~~

Always set in relevant cases.

> /home/meinersbur/src/isl/isl_map_simplify.c:4745:2: warning:
> ‘last_pos’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 4745 | isl_basic_map_inequality_to_equality(bmap, ineq);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Always set.

skimo

[Michael Kruse]

Whether software should build warning-free might be a subjective
question. Maybe Tomasz Kłoczko could respond on their perspective.

Am Fr., 24. Sept. 2021 um 12:30 Uhr schrieb Sven Verdoolaege
<sven.verdo...@gmail.com>:

> > /home/meinersbur/src/isl/pip.c: In function ‘main’:
> > /home/meinersbur/src/isl/pip.c:75:10: warning: ‘context_copy’ may be
> > used uninitialized in this function [-Wmaybe-uninitialized]
> > 75 | return isl_basic_set_free(bset);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~
> > /home/meinersbur/src/isl/pip.c:339:48: note: ‘context_copy’ was declared here
> > 339 | struct isl_basic_set *context, *bset, *copy, *context_copy;
> > | ^~~~~~~~~~~~
> > /home/meinersbur/src/isl/pip.c:339:41: warning: ‘copy’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> > 339 | struct isl_basic_set *context, *bset, *copy, *context_copy;
> > | ^~~~
> > /home/meinersbur/src/isl/bound.c: In function ‘main’:
> > /home/meinersbur/src/isl/bound.c:241:27: warning: ‘copy’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> > 241 | isl_pw_qpolynomial_fold *copy;
> > | ^~~~
>
> Looks fine too me. Perhaps gcc is assuming options->verify may change
> in between.

Since options->verify is not a stack variable, the calls in-between
are indeed allowed to change it. Interestingly, clang does emit a
warning here.

> > In file included from /home/meinersbur/src/isl/basis_reduction_tab.c:112:
> > /home/meinersbur/src/isl/basis_reduction_templ.c: In function
> > ‘isl_tab_compute_reduced_basis’:
> > /home/meinersbur/src/isl/basis_reduction_templ.c:203:6: warning: ‘row’
> > may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 203 | save_alpha(lp, row-i, i, alpha_buffer[j]);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> AFAICT, row is always initialized.

row is not assigned in the `i+1 == tab->n_zero` condition. Compilers
do not try to infer logical implications.

> > /home/meinersbur/src/isl/isl_schedule_tree.c:683:5: warning: ‘equal’
> > may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 683 | if (equal < 0 || !equal)
> > | ^
>
> equal is set in all cases

gcc assumes that tree1->type can have a value not covered by one of
the enum values; since its underlying value is an `int` it
theoretically can be assigned any integer. Changing

- case isl_schedule_node_error:
+ default:

Would make the warning go away.

> > /home/meinersbur/src/isl/isl_ast_build.c:1556:10: warning: ‘space’ may
> > be used uninitialized in this function [-Wmaybe-uninitialized]
> > 1556 | space = isl_space_map_from_set(space);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> I guess there is a similar issue with build->node here.
> Does it help if "!build->node" is stored in a local variable and reused?

Yes

> > /home/meinersbur/src/isl/isl_sample.c:1334:9: warning: ‘pnt’ may be
> > used uninitialized in this function [-Wmaybe-uninitialized]
> > 1334 | return pnt;
> > | ^~~
>
> set->n is non-negative, so it's always set.

The compiler does not know that set->n is never assigned a negative value.

My suggestion is to initially assign NULL to pnt, and after the loop
check whether pnt is still NULL, and assign the void point if it is.

> > /home/meinersbur/src/isl/isl_schedule_tree.c:1942:17: warning:
> > ‘domain’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> > 1942 | isl_union_set *domain;
> > | ^~~~~~
>
> Set in all relevant cases.

A default case would make this warning go away.

> > /home/meinersbur/src/isl/isl_map_simplify.c:4759:5: warning:
> > ‘last_neg’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> > 4759 | if (ineq1 > ineq2) {
> > | ^
>
> Always set.

last_neg is only assigned in a loop with a conditional. The
conditional could never be true or the loop not iterate even once.
Maybe there is some underlying reason why this cannot happen, but not
even me can see that at a glance.

> > /home/meinersbur/src/isl/isl_map.c: In function
> > ‘isl_map_partial_lexopt_aligned_pw_multi_aff’:
> > /home/meinersbur/src/isl/isl_scheduler.c:6089:43: warning: ‘best_dist’
> > may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 6089 | if (best_weight == weight && best_dist <= dist)
> > | ~~~~~~~~~~^~~~~~~
>
> Always set under given conditions.

This maybe could be discovered by a static analyzer, but compilers
usually look only at the control flow. From that point of view, there
is a use of best_dist in the first iteration of the loop before it is
assigned.

> > /home/meinersbur/src/isl/isl_map.c:7288:11: warning: ‘todo’ may be
> > used uninitialized in this function [-Wmaybe-uninitialized]
> > 7288 | todo = isl_set_intersect(todo, *empty);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Always set under given condition.
>
> > /home/meinersbur/src/isl/isl_schedule_tree.c:1836:17: warning: ‘umap’
> > may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 1836 | isl_union_map *umap;
> > | ^~~~
>
> Always set in relevant cases.
>
> > /home/meinersbur/src/isl/isl_map_simplify.c:4745:2: warning:
> > ‘last_pos’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> > 4745 | isl_basic_map_inequality_to_equality(bmap, ineq);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Always set.

Too complex of a control flow for the compiler to see.


Michael

--
Tardyzentrismus verboten!

[Sven Verdoolaege]

On Fri, Sep 24, 2021 at 02:19:54PM -0500, Michael Kruse wrote:
> > > In file included from /home/meinersbur/src/isl/basis_reduction_tab.c:112:
> > > /home/meinersbur/src/isl/basis_reduction_templ.c: In function
> > > ‘isl_tab_compute_reduced_basis’:
> > > /home/meinersbur/src/isl/basis_reduction_templ.c:203:6: warning: ‘row’
> > > may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > 203 | save_alpha(lp, row-i, i, alpha_buffer[j]);
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > AFAICT, row is always initialized.
>
> row is not assigned in the `i+1 == tab->n_zero` condition. Compilers
> do not try to infer logical implications.

My mistake. I missed that this was a cascading if.
Still, it's not used in that case.

> > > /home/meinersbur/src/isl/isl_schedule_tree.c:683:5: warning: ‘equal’
> > > may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > 683 | if (equal < 0 || !equal)
> > > | ^
> >
> > equal is set in all cases
>
> gcc assumes that tree1->type can have a value not covered by one of
> the enum values; since its underlying value is an `int` it
> theoretically can be assigned any integer. Changing
>
> - case isl_schedule_node_error:
> + default:
>
> Would make the warning go away.

It would also stop the compiler from warning about any missing cases
in the future.

> > > /home/meinersbur/src/isl/isl_ast_build.c:1556:10: warning: ‘space’ may
> > > be used uninitialized in this function [-Wmaybe-uninitialized]
> > > 1556 | space = isl_space_map_from_set(space);
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > I guess there is a similar issue with build->node here.
> > Does it help if "!build->node" is stored in a local variable and reused?
>
> Yes

Feel free to submit a patch.

> > > /home/meinersbur/src/isl/isl_map_simplify.c:4759:5: warning:
> > > ‘last_neg’ may be used uninitialized in this function
> > > [-Wmaybe-uninitialized]
> > > 4759 | if (ineq1 > ineq2) {
> > > | ^
> >
> > Always set.
>
> last_neg is only assigned in a loop with a conditional. The
> conditional could never be true or the loop not iterate even once.
> Maybe there is some underlying reason why this cannot happen, but not
> even me can see that at a glance.

If last_neg is not set, then neg is zero and this code is not reached.

skimo