[PATCH] Reset source location for instructions moved out of its original residing basic block

[PATCH] Reset source location for instructions moved out of its original residing basic block

[Dehao Chen]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

Hi,

When debugging optimized code, it is always confusing when gdb jumped
to a place that has never been executed. This is because compiler
performs some aggressive code motion that moves an instruction outside
of its original residing basic block.

This patch tries to fix this problem by resetting the source location
when moving instructions to another BB. This can greatly improve the
debuggability of optimized code. For the attached unittest. Without
the patch, the debugger will always jump into line 14 even when the
branch at line 13 is not taken. With the patch, the problem is fixed.

Bootstrapped and passed gcc regression test.

Is it okay for trunk?

Thanks,
Dehao

gcc/ChangeLog:
2012-10-31  Dehao Chen  <dehao@google.com>

        * emit-rtl.c (reorder_insns): Reset the source location for
        instructions moved out of its original residing basic block.

gcc/testsuite/ChangeLog:
2012-10-31  Dehao Chen  <dehao@google.com>

        * gcc.dg/debug/dwarf2/code-motion.c: New testcase.

[-- Attachment #2: code-motion.patch --]
[-- Type: application/octet-stream, Size: 1232 bytes --]

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	(revision 193056)
+++ gcc/emit-rtl.c	(working copy)
@@ -4111,8 +4111,12 @@ reorder_insns (rtx from, rtx to, rtx after)
 	BB_END (bb) = to;
 
       for (x = from; x != NEXT_INSN (to); x = NEXT_INSN (x))
-	if (!BARRIER_P (x))
-	  df_insn_change_bb (x, bb);
+	{
+	  if (!BARRIER_P (x))
+	    df_insn_change_bb (x, bb);
+	  if (GET_CODE (after) == INSN && GET_CODE (x) == INSN)
+	    INSN_LOCATION (x) = INSN_LOCATION (after);
+	}
     }
 }
 
Index: gcc/testsuite/gcc.dg/debug/dwarf2/code-motion.c
===================================================================
--- gcc/testsuite/gcc.dg/debug/dwarf2/code-motion.c	(revision 0)
+++ gcc/testsuite/gcc.dg/debug/dwarf2/code-motion.c	(revision 0)
@@ -0,0 +1,20 @@
+// This test makes sure code motion will not pollute debug info.
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+// { dg-options "-O2 -g -dA" }
+extern int bar(int);
+
+int foo(int *a, int x)
+{
+  int y = 5;
+  int ret = 10;
+
+  a[0] = ret * y;
+
+  if (x > 5)
+    y = a[a[3]];
+  y = y * 2 + a[3];
+  ret += bar(y);
+
+  return ret;
+}
+// { dg-final { scan-assembler-times "code-motion.c:14" 1} }

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Eric Botcazou]

> This patch tries to fix this problem by resetting the source location
> when moving instructions to another BB. This can greatly improve the
> debuggability of optimized code. For the attached unittest. Without
> the patch, the debugger will always jump into line 14 even when the
> branch at line 13 is not taken. With the patch, the problem is fixed.

But this breaks coverage info!  You cannot change the source location of 
instructions randomly like that; the rule should be that, once a location is 
set, it cannot be changed.  At most it can be cleared.

> gcc/ChangeLog:
> 2012-10-31  Dehao Chen  <dehao@google.com>
> 
>         * emit-rtl.c (reorder_insns): Reset the source location for
>         instructions moved out of its original residing basic block.

No, that isn't acceptable.

-- 
Eric Botcazou

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Dehao Chen]

On Thu, Nov 1, 2012 at 1:52 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This patch tries to fix this problem by resetting the source location
>> when moving instructions to another BB. This can greatly improve the
>> debuggability of optimized code. For the attached unittest. Without
>> the patch, the debugger will always jump into line 14 even when the
>> branch at line 13 is not taken. With the patch, the problem is fixed.
>
> But this breaks coverage info!  You cannot change the source location of

For optimized code, there are many optimizations that can break
coverage info. Code motion is one of them. This patch actually tries
to fix the broken coverage info, as illustrated by the unittest.

> instructions randomly like that; the rule should be that, once a location is
> set, it cannot be changed.  At most it can be cleared.

If we clear the debug info for instructions moved to other BB, is it acceptable?

Thanks,
Dehao

>
>> gcc/ChangeLog:
>> 2012-10-31  Dehao Chen  <dehao@google.com>
>>
>>         * emit-rtl.c (reorder_insns): Reset the source location for
>>         instructions moved out of its original residing basic block.
>
> No, that isn't acceptable.
>
> --
> Eric Botcazou

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Eric Botcazou]

> For optimized code, there are many optimizations that can break
> coverage info. Code motion is one of them. This patch actually tries
> to fix the broken coverage info, as illustrated by the unittest.

No, you seems to be misunderstanding what coverage means.  For coverage to be 
correct, it is necessary that every insn generated for a particular source 
construct be associated with this source construct in the debug info.  If you 
associate an insn with another source construct and it is executed, then 
you'll have the other source construct marked as covered, although it might 
not be actually covered during the execution.

Code motion (as any other optimization passes) doesn't break coverage per se.
For example, there is nothing wrong with hoisting a instruction out of a loop 
and keeping its source location if it is always executed; coverage info will 
be correct after the hoisting.  In more complex cases, it might be necessary 
to clear the source location of the hoisted instruction.

> If we clear the debug info for instructions moved to other BB, is it
> acceptable?

Yes, clearing is acceptable in principle, but should be done with extreme care 
since you drop information, so reorder_insns isn't the appropriate place as 
it's too big a hammer.

FWIW, we have related patchlets in our internal tree, like:

	* loop-invariant.c (move_invariant_reg): Clear the locator of the
	invariant's insn after it has been moved.

	* tree-ssa-loop-im.c (move_computations_stmt): Clear the location and
	block of the invariant's statement after it has been moved.


As for the more general problem of jumpiness in GDB for highly optimized code, 
this should be changed in GDB if your users cannot deal with it.  The debug 
info describes the relationship between the generated code and the source code 
and, at high optimization levels, this relationship is not isomorphic at all.
It's up to the source level debugger to filter out the non-isomorphic part of 
the mapping if it deems desirable to do so.

-- 
Eric Botcazou

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Dehao Chen]

Hi, Eric,

I see your point. How about we guard these changes with a flag, say
-gless-jumpy, so that people can always choose between better coverage
and less jumpy gdb behavior (it's also important for some other
clients like AutoFDO). I will have a series of patches to follow soon
that can be guarded by this flag.

Thanks,
Dehao

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Ian Lance Taylor]

On Thu, Nov 1, 2012 at 10:00 AM, Dehao Chen <dehao@google.com> wrote:
>
> I see your point. How about we guard these changes with a flag, say
> -gless-jumpy, so that people can always choose between better coverage
> and less jumpy gdb behavior (it's also important for some other
> clients like AutoFDO). I will have a series of patches to follow soon
> that can be guarded by this flag.

This feels to me like an attempt to address the problem in the wrong
place.  It seems to me that it would be better to do one of:

* Use -Og and ensure that -Og does not move the code around.
Presumably this would lead to worse runtime performance and better
performance in the debugger.

* Add heuristics to the debugger to jump around less.

* Add a new debug facility to mark the statement as attached to a
particular source location, but moved relative to other source
locations.  Add facilities to the debugger to take that into account.

That said, I suppose I can imagine a mode like you suggest.  It
shouldn't be a -g option, it should be a -f option, like
-fdiscard-moved-insn-debug-locations or something.  That would be
along the lines of -fno-var-tracking: we generate worse debug info
upon user request.

Ian

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Xinliang David Li]

On Thu, Nov 1, 2012 at 3:57 PM, Ian Lance Taylor <iant@google.com> wrote:
> On Thu, Nov 1, 2012 at 10:00 AM, Dehao Chen <dehao@google.com> wrote:
>>
>> I see your point. How about we guard these changes with a flag, say
>> -gless-jumpy, so that people can always choose between better coverage
>> and less jumpy gdb behavior (it's also important for some other
>> clients like AutoFDO). I will have a series of patches to follow soon
>> that can be guarded by this flag.
>
> This feels to me like an attempt to address the problem in the wrong
> place.  It seems to me that it would be better to do one of:
>
> * Use -Og and ensure that -Og does not move the code around.
> Presumably this would lead to worse runtime performance and better
> performance in the debugger.
>
> * Add heuristics to the debugger to jump around less.
>
> * Add a new debug facility to mark the statement as attached to a
> particular source location, but moved relative to other source
> locations.  Add facilities to the debugger to take that into account.
>
> That said, I suppose I can imagine a mode like you suggest.  It
> shouldn't be a -g option, it should be a -f option, like
> -fdiscard-moved-insn-debug-locations or something.  That would be
> along the lines of -fno-var-tracking: we generate worse debug info
> upon user request.
>

Or have a common umbrella option to guard all changes that improves DOC.

David



> Ian

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Dehao Chen]

On Thu, Nov 1, 2012 at 4:07 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Nov 1, 2012 at 3:57 PM, Ian Lance Taylor <iant@google.com> wrote:
>> On Thu, Nov 1, 2012 at 10:00 AM, Dehao Chen <dehao@google.com> wrote:
>>>
>>> I see your point. How about we guard these changes with a flag, say
>>> -gless-jumpy, so that people can always choose between better coverage
>>> and less jumpy gdb behavior (it's also important for some other
>>> clients like AutoFDO). I will have a series of patches to follow soon
>>> that can be guarded by this flag.
>>
>> This feels to me like an attempt to address the problem in the wrong
>> place.  It seems to me that it would be better to do one of:
>>
>> * Use -Og and ensure that -Og does not move the code around.
>> Presumably this would lead to worse runtime performance and better
>> performance in the debugger.
>>
>> * Add heuristics to the debugger to jump around less.
>>
>> * Add a new debug facility to mark the statement as attached to a
>> particular source location, but moved relative to other source
>> locations.  Add facilities to the debugger to take that into account.
>>
>> That said, I suppose I can imagine a mode like you suggest.  It
>> shouldn't be a -g option, it should be a -f option, like
>> -fdiscard-moved-insn-debug-locations or something.  That would be
>> along the lines of -fno-var-tracking: we generate worse debug info
>> upon user request.

I don't see why debug info would be worse if we add something like
-fdiscard-moved-insn-debug-locations, as long as it is not turned on
by default. Any thoughts?

>>
>
> Or have a common umbrella option to guard all changes that improves DOC.

Is it like -Og approach as Ian mentioned? Yes, we can sacrifice some
performance (maybe 5%?) to get more accurate source level profile. But
if we plan to use AutoFDOed binary to collect AutoFDO profile, it'll
suffer the same debug problem again...

Thanks,
Dehao

>
> David
>
>
>
>> Ian

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Xinliang David Li]

On Thu, Nov 1, 2012 at 4:16 PM, Dehao Chen <dehao@google.com> wrote:
> On Thu, Nov 1, 2012 at 4:07 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Thu, Nov 1, 2012 at 3:57 PM, Ian Lance Taylor <iant@google.com> wrote:
>>> On Thu, Nov 1, 2012 at 10:00 AM, Dehao Chen <dehao@google.com> wrote:
>>>>
>>>> I see your point. How about we guard these changes with a flag, say
>>>> -gless-jumpy, so that people can always choose between better coverage
>>>> and less jumpy gdb behavior (it's also important for some other
>>>> clients like AutoFDO). I will have a series of patches to follow soon
>>>> that can be guarded by this flag.
>>>
>>> This feels to me like an attempt to address the problem in the wrong
>>> place.  It seems to me that it would be better to do one of:
>>>
>>> * Use -Og and ensure that -Og does not move the code around.
>>> Presumably this would lead to worse runtime performance and better
>>> performance in the debugger.
>>>
>>> * Add heuristics to the debugger to jump around less.
>>>
>>> * Add a new debug facility to mark the statement as attached to a
>>> particular source location, but moved relative to other source
>>> locations.  Add facilities to the debugger to take that into account.
>>>
>>> That said, I suppose I can imagine a mode like you suggest.  It
>>> shouldn't be a -g option, it should be a -f option, like
>>> -fdiscard-moved-insn-debug-locations or something.  That would be
>>> along the lines of -fno-var-tracking: we generate worse debug info
>>> upon user request.
>
> I don't see why debug info would be worse if we add something like
> -fdiscard-moved-insn-debug-locations, as long as it is not turned on
> by default. Any thoughts?
>
>>>
>>
>> Or have a common umbrella option to guard all changes that improves DOC.
>
> Is it like -Og approach as Ian mentioned? Yes, we can sacrifice some
> performance (maybe 5%?) to get more accurate source level profile. But
> if we plan to use AutoFDOed binary to collect AutoFDO profile, it'll
> suffer the same debug problem again...

No not the same as -Og -- DOC means debugging for fully optimized code.

David


>
> Thanks,
> Dehao
>
>>
>> David
>>
>>
>>
>>> Ian

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Alexandre Oliva]

On Nov  1, 2012, Dehao Chen <dehao@google.com> wrote:

> I see your point. How about we guard these changes with a flag, say
> -gless-jumpy

The right (DWARF-expected) approach to avoid this sortof jumpiness is to
teach GCC to emit .locs with is_stmt=1, along the lines (and
limitations) described in my GCC Summit paper on stmt frontier notes,
that proposes further extensions to deal with the foreseen limitations.

http://people.redhat.com/~aoliva/papers/sfn/gcc2010.pdf

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Reset source location for instructions moved out of its original residing basic block

[Richard Biener]

On Sun, Nov 4, 2012 at 11:26 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov  1, 2012, Dehao Chen <dehao@google.com> wrote:
>
>> I see your point. How about we guard these changes with a flag, say
>> -gless-jumpy
>
> The right (DWARF-expected) approach to avoid this sortof jumpiness is to
> teach GCC to emit .locs with is_stmt=1, along the lines (and
> limitations) described in my GCC Summit paper on stmt frontier notes,
> that proposes further extensions to deal with the foreseen limitations.

Right.  gdb could also instead of show a single line (and that changing
all the time) show all currently "active" lines (thus scan the current
function for lines that appear and compute their "liveness").  A step
would then jump to the next stmt that changes line liveness.

Richard.

> http://people.redhat.com/~aoliva/papers/sfn/gcc2010.pdf
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer