On Wed, Jul 10, 2019 at 11:16 PM Eric Botcazou <ebotca...@adacore.com> wrote:
>
> Hi,
>
> the returns are a little problematic for coverage info because they are
> commonalized in GIMPLE, i.e. turned into gotos to a representative return.
> This means that you need to clear the location of the representative return,
> see lower_gimple_return:
>
> /* Remove the line number from the representative return statement.
> It now fills in for many such returns. Failure to remove this
> will result in incorrect results for coverage analysis. */
> gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);
>
> otherwise the coverage info is blatantly wrong. But this in turn means that
> such representative returns are vulneable to inheriting random source location
> information in the debug info generated by the compiler.
>
> I have attached an Ada testcase that demonstrates the problem at -O0:
>
> .loc 1 14 10 discriminator 2
> movl $0, %r13d
> .L12:
> movl %r13d, %eax
> jmp .L23
>
> .L12 is what's left at the RTL level of a representative return in GIMPLE and
> it inherits the location directive, which gives wrong coverage info (that's
> not visible with gcov because the instrumentation done by -fprofile-arcs
> papers over the issue, but for example with callgrind).
>
> The proposed fix is attached: it sets the current location to the function's
> end locus when expanding such a representative return to RTL. That's a bit on
> the kludgy side, but doing something in GIMPLE, e.g. in lower_gimple_return,
> leads to various regressions in terms of quality of diagnostics.
>
> Tested on x86_64-suse-linux, both GCC and GDB, OK for mainline?
Hmm. So for
int baz;
int foo()
{
int i;
goto skip;
done:
return i;
skip:
i = 1;
if (baz)
return baz;
/* ... */
goto done;
} /* (*) */
we'd assign (*) to the return? It might be better to record
(in struct function?) the location of any actually existing
return location and use that? In fact, similar kludgy would
be to simply not clear the GIMPLE_RETURN location
but kludge it up right away?
Can you explain how diagnostics regress when doing that?
Does coverage somehow treat the function end locus specially
so you chose that? Will it show the alternate returns as
covered at all? I presume stuff like cross-jumping or
GIMPLE tail-merging also wrecks coverage info in similar
ways (well, not by injecting random locations but by
not covering taken paths).
Thanks,
Richard.
>
>
> 2019-07-10 Eric Botcazou <ebotca...@adacore.com>
>
> * cfgexpand.c (expand_gimple_stmt_1) <GIMPLE_RETURN>: If the statement
> doesn't have a location, set the current location to the function's
> end.
>
> --
> Eric Botcazou
