Argh! Paul OKed the patch already.
Here are my comments anyway.
On 15/09/2012 11:46, Tobias Burnus wrote:
> Dear all,
>
> this patch fixes some of the warning showing up for gcc/fortran at
> http://scan.coverity.com/. Coverity sells static C/C++/C#/Java code
> analyzers and offer scanning to open-source projects. The result looks
> like
> http://www.coverity.com/images/products/static-analysis/screenshot-1-large.png
>
>
> * [Out of bounds] array.c's gfc_match_array_ref: I am not sure whether
> that code is unreachable, but in any case it is out of bounds. It does
> not seem to trigger for our test cases; maybe removing it and adding an
> assert is sufficient?
>
> diff --git a/gcc/fortran/array.c b/gcc/fortran/array.c
> index 44ec72e..1611c3b 100644
> --- a/gcc/fortran/array.c
> +++ b/gcc/fortran/array.c
> @@ -253,7 +253,7 @@ coarray:
> gfc_error ("Invalid form of coarray reference at %C");
> return MATCH_ERROR;
> }
> - else if (ar->dimen_type[ar->codimen + ar->dimen] == DIMEN_STAR)
> + else if (ar->dimen_type[ar->codimen + ar->dimen - 1] == DIMEN_STAR)
> {
> gfc_error ("Unexpected '*' for codimension %d of %d at %C",
> ar->codimen + 1, corank);
That one is not completely obvious.
Your change is bogus in the case ar->dimen==ar->codimen==0 at least.
The loop starts with ar->codimen = 0, and when the closing bracked is
matched, there is an extra ar->codimen++.
So in the scope of the loop, ar->codimen is the maximal coarray index,
not the codimension. I have a feeling that there is no out of bound
here. Does coverity give more details?
>
> * [Uninitialized memory] trans-io.c's gfc_trans_transfer: Here, the
> warning is about accessing uninitialized loop elements in
> gfc_cleanup_loop. The reason is that those aren't initialized for
> expr->rank == 0; I thought that the code should be unreachable due to
> the (se.ss != 0) condition, but an assert fails for instance for
> graphite/pr36286.f90. On the other hand, for se.ss != NULL && expr->rank
> == 0, handling lower code part is wrong as it only deals with "loop".
> Probably, one needs to use "if (se.ss == NULL || expr->rank == 0)",
> which I now did.
> diff --git a/gcc/fortran/trans-io.c b/gcc/fortran/trans-io.c
> index 34db6fd..4ad961f 100644
> --- a/gcc/fortran/trans-io.c
> +++ b/gcc/fortran/trans-io.c
> @@ -2310,7 +2313,7 @@ gfc_trans_transfer (gfc_code * code)
> gfc_add_block_to_block (&body, &se.pre);
> gfc_add_block_to_block (&body, &se.post);
>
> - if (se.ss == NULL)
> + if (se.ss == NULL || expr->rank == 0)
> tmp = gfc_finish_block (&body);
> else
> {
I'm not sure about that one.
I tried adding a `gcc_assert (expr->rank != 0);' in the `else' branch
and it passed for me on pr36286.f90 (but I don't have graphite enabled).
All the other changes look good to me (OK for them).
>
> TODO:
>
> There are also real issues like the following in symbol.c's
> gfc_undo_symbols:
>
> gfc_free_namelist (old->namelist_tail);
> old->namelist_tail->next = NULL;
>
> where one first frees the memory and then sets a component to NULL.
> Locally, I was wondering whether the two lines should be swapped as
> free_namelist also frees ns->next. Alternatively, the "->next" part is
> wrong, which is more in line with the next line of code:
> p->namelist_tail = old->namelist_tail;
> But I think one needs to study the code more closely to know what the
> code is supposed to do.
>
According to the code in match.c, `namelist_tail' seems to be the last
element in the chain so that new elements can be added to
`namelist_tail->next'.
Then `gfc_undo_symbol' should probably have:
gfc_free_namelist (old->namelist_tail->next);
old->namelist_tail->next = NULL;
Thanks
Mikael
