| Message ID | 201103281100.57051.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
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 >
> 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. Could you elaborate? UNKNOWN_LOCATION isn't used for INSN_LOCATOR at all thanks for the curr_insn_locator hunk. > 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. Huh? What is the line setting input_location for? > 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). Well, RTL locators are tested against zero all over the place so it's pretty clear what zero means. I don't see any overloading here.
On Mon, Mar 28, 2011 at 1:08 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> 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. > > Could you elaborate? UNKNOWN_LOCATION isn't used for INSN_LOCATOR at all > thanks for the curr_insn_locator hunk. int curr_insn_locator (void) { - if (curr_rtl_loc == -1) + if (curr_rtl_loc == -1 || curr_location == UNKNOWN_LOCATION) return 0; Oh, I didn't realize curr_location is of type location_t. I'm not very familiar with these bits. Btw, insn_locators_alloc initializes it with -1 while it should probably initialize it with UNKNOWN_LCOATION as well. >> 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. > > Huh? What is the line setting input_location for? Oh, it's contionally set ... not enough coffee. >> 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). > > Well, RTL locators are tested against zero all over the place so it's pretty > clear what zero means. I don't see any overloading here. So it looks like the patch is perfect after all. And ok. Thanks, Richard. > -- > Eric Botcazou >
> Oh, I didn't realize curr_location is of type location_t. I'm not very > familiar with these bits. Btw, insn_locators_alloc initializes it with -1 > while it should probably initialize it with UNKNOWN_LCOATION as well. Indeed, will adjust them afterwards. > So it looks like the patch is perfect after all. And ok. Thanks.
Index: cfgexpand.c =================================================================== --- cfgexpand.c (revision 171572) +++ cfgexpand.c (working copy) @@ -1745,11 +1745,8 @@ expand_gimple_cond (basic_block bb, gimp last2 = last = get_last_insn (); extract_true_false_edges_from_block (bb, &true_edge, &false_edge); - if (gimple_has_location (stmt)) - { - set_curr_insn_source_location (gimple_location (stmt)); - set_curr_insn_block (gimple_block (stmt)); - } + set_curr_insn_source_location (gimple_location (stmt)); + set_curr_insn_block (gimple_block (stmt)); /* These flags have no purpose in RTL land. */ true_edge->flags &= ~EDGE_TRUE_VALUE; @@ -1896,6 +1893,10 @@ static void expand_gimple_stmt_1 (gimple stmt) { tree op0; + + set_curr_insn_source_location (gimple_location (stmt)); + set_curr_insn_block (gimple_block (stmt)); + switch (gimple_code (stmt)) { case GIMPLE_GOTO: @@ -2052,32 +2053,21 @@ expand_gimple_stmt_1 (gimple stmt) static rtx expand_gimple_stmt (gimple stmt) { - int lp_nr = 0; - rtx last = NULL; location_t saved_location = input_location; + rtx last = get_last_insn (); + int lp_nr; - last = get_last_insn (); - - /* If this is an expression of some kind and it has an associated line - number, then emit the line number before expanding the expression. - - We need to save and restore the file and line information so that - errors discovered during expansion are emitted with the right - information. It would be better of the diagnostic routines - used the file/line information embedded in the tree nodes rather - than globals. */ gcc_assert (cfun); + /* We need to save and restore the current source location so that errors + discovered during expansion are emitted with the right location. But + it would be better if the diagnostic routines used the source location + embedded in the tree nodes rather than globals. */ if (gimple_has_location (stmt)) - { - input_location = gimple_location (stmt); - set_curr_insn_source_location (input_location); - - /* Record where the insns produced belong. */ - set_curr_insn_block (gimple_block (stmt)); - } + input_location = gimple_location (stmt); expand_gimple_stmt_1 (stmt); + /* Free any temporaries used to evaluate this statement. */ free_temp_slots (); Index: cfglayout.c =================================================================== --- cfglayout.c (revision 171572) +++ cfglayout.c (working copy) @@ -323,7 +323,7 @@ get_curr_insn_block (void) int curr_insn_locator (void) { - if (curr_rtl_loc == -1) + if (curr_rtl_loc == -1 || curr_location == UNKNOWN_LOCATION) return 0; if (last_block != curr_block) {
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? 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.