Re: [patch] Fix bogus source locations with optimization

[Richard Guenther <richard dot guenther at gmail dot com>]

On Mon, Mar 28, 2011 at 11:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> when optimization is enabled, especially -O2 and above, you can have lines in
> the assembly file with really bogus source location info. ?The scenario is as
> follows: an optimization pass at the Tree level (typically PRE) creates a new
> statement and inserts it at some place in the dominator tree, creating a new
> basic block; this statement is (rightfully) given "unknown" location. ?When
> RTL is being expanded, this statement inherits the current source location,
> which is the location of the last statement of the previous basic block. ?Then
> basic block reordering kicks in and the statement is moved to another place,
> still carrying the inherited source location down to the assembly file.
>
> The proposed fix is to allow RTL statements to have "unknown" location ("zero"
> locator) like Tree statements. ?They of course will be assigned the current
> source location but only in the assembly file, thus becoming sort of silent.
>
> Tested on {i586,x86_64}-suse-linux, OK for the mainline?

This overloads UNKNOWN_LOCATION for both insn_locator and
source_location, I don't think this is the best idea.  It'll eventually
break when compiling with C++ anyway.

The expand_gimple_stmt change will cause late diagnostic to
use an unknown location instead of one from a previously expanded
stmt.  Probably not ideal but matches what GIMPLE diagnostics
would have done earlier.

In general I think the patch is a good thing, but the UNKNOWN_LOCATION
overloading needs to be resolved.  Maybe just add UINKNOWN_LOCATOR?
The special value also should be documented somewhere (no idea where
core insn-locator functionality resides).

Thanks,
Richard.

>
> 2011-03-28 ?Eric Botcazou ?<ebotcazou@adacore.com>
>
> ? ? ? ?* cfgexpand.c (expand_gimple_cond): Always set the source location and
> ? ? ? ?block before expanding the statement.
> ? ? ? ?(expand_gimple_stmt_1): Likewise. ?Set them here...
> ? ? ? ?(expand_gimple_stmt): ...and not here. ?Tidy.
> ? ? ? ?* cfglayout.c (curr_insn_locator): Return 0 if the current location is
> ? ? ? ?unknown.
>
>
> --
> Eric Botcazou
>