Re: [PATCH] Update source location for PRE inserted stmt

2012-11-26 Thread Dehao Chen
On Sun, Nov 25, 2012 at 11:37 AM, Xinliang David Li davi...@google.com wrote: On Sun, Nov 25, 2012 at 4:40 AM, Richard Biener richard.guent...@gmail.com wrote: On Thu, Nov 15, 2012 at 5:46 PM, Eric Botcazou ebotca...@adacore.com wrote: But UNKNOWN_LOCATION is effectively wrong as well. If

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-25 Thread Richard Biener
On Thu, Nov 15, 2012 at 5:46 PM, Eric Botcazou ebotca...@adacore.com wrote: But UNKNOWN_LOCATION is effectively wrong as well. If other optimizations move the statements above the inserted instruction, then the new instruction ends up inheriting whatever location happens to be in the previous

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-15 Thread Diego Novillo
On 2012-11-05 06:54 , Eric Botcazou wrote: Those compiler generated statements do not have source origins, but they need to have debug location information attached so that information collected for them can be correlated with program constructs (such as CFG). One of the natural way is to use

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-15 Thread Eric Botcazou
But UNKNOWN_LOCATION is effectively wrong as well. If other optimizations move the statements above the inserted instruction, then the new instruction ends up inheriting whatever location happens to be in the previous statement. That's correct, UNKNOWN_LOCATION isn't a panacea either,

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-15 Thread Dehao Chen
On Thu, Nov 15, 2012 at 8:46 AM, Eric Botcazou ebotca...@adacore.com wrote: But UNKNOWN_LOCATION is effectively wrong as well. If other optimizations move the statements above the inserted instruction, then the new instruction ends up inheriting whatever location happens to be in the

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-15 Thread Eric Botcazou
The randomness here means that if we set UNKNOWN_LOCATION to insn, it can get source location anywhere. Even with some small code layout changes, the location for that insn could change. I would hope that in the future, we add an assertion when emitting instruction to enforce that

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-15 Thread Xinliang David Li
I probably made too general statement in this topic. However for the PRE case, I believe the choice of not using UNKNOWN location is still better. thanks, David On Thu, Nov 15, 2012 at 9:23 AM, Eric Botcazou ebotca...@adacore.com wrote: The randomness here means that if we set UNKNOWN_LOCATION

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-15 Thread Dehao Chen
Yeah, at least for the unittest I provided, the coverage info will be wrong without the patch. Thanks, Dehao On Thu, Nov 15, 2012 at 10:30 AM, Xinliang David Li davi...@google.com wrote: I probably made too general statement in this topic. However for the PRE case, I believe the choice of not

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-05 Thread Eric Botcazou
Those compiler generated statements do not have source origins, but they need to have debug location information attached so that information collected for them can be correlated with program constructs (such as CFG). One of the natural way is to use the source location associated with the

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-05 Thread Dehao Chen
No, there is nothing natural (and this can even be wrong). The statements must have the source location corresponding to the source construct they are generated for, which may be totally different from that of the insertion point. Yes, that can generate jumpiness in GDB, but this is far

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-05 Thread Xinliang David Li
For the example I listed, the new statement is generated for source construct at program point (2). However unlike simple code motion, (2) is not going away after PRE. How would setting the location of the new statement at the insertion point break coverage? Besides, the new statement won't create

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-04 Thread Richard Biener
On Wed, Oct 31, 2012 at 8:02 PM, Xinliang David Li davi...@google.com wrote: Dehao's patch will make the debugging of the following code (-g -O2) less jumpy. After the testing of x 0, it should go to line 'a++'. Without the fix, when stepping through 'abc', the lines covered are 6, 4, 11,

Re: [PATCH] Update source location for PRE inserted stmt

2012-11-04 Thread Dehao Chen
2. __attribute__((noinline)) int abc (int *a) 3. { 4. int ret = 0; 5. 6. if (x 0) 7.ret += *a; 8. else 9.a++; 10. 11. ret += *a; 12. return ret; 13 } In terms of jumpiness, without the patch, the jump sequence is: 2 - 13 - 4 - 11 - 13 With the patch, the jump sequence is: 2- 9 - 4

Re: [PATCH] Update source location for PRE inserted stmt

2012-10-31 Thread Richard Biener
On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li davi...@google.com wrote: It will make the location info for the newly synthesized stmt more deterministic, I think. Maybe, but it will increase the jumpiness in the debugger without actually being accurate, no? For example if the partially

Re: [PATCH] Update source location for PRE inserted stmt

2012-10-31 Thread Dehao Chen
On Wed, Oct 31, 2012 at 2:34 AM, Richard Biener richard.guent...@gmail.com wrote: On Wed, Oct 31, 2012 at 12:57 AM, Xinliang David Li davi...@google.com wrote: It will make the location info for the newly synthesized stmt more deterministic, I think. Maybe, but it will increase the

Re: [PATCH] Update source location for PRE inserted stmt

2012-10-31 Thread Xinliang David Li
Dehao's patch will make the debugging of the following code (-g -O2) less jumpy. After the testing of x 0, it should go to line 'a++'. Without the fix, when stepping through 'abc', the lines covered are 6, 4, 11, 13. With the fix it should be 6, 9, 11, 13 -- much better. David 1. int x;

[PATCH] Update source location for PRE inserted stmt

2012-10-30 Thread Dehao Chen
Hi, This patch aims to improve debugging of optimized code. It ensures that PRE inserted statements have the same source location as the statement at the insertion point, instead of UNKNOWN_LOCATION. Bootstrapped on x86_64, and passed gcc regression tests and gdb regression tests. Is it okay

Re: [PATCH] Update source location for PRE inserted stmt

2012-10-30 Thread Steven Bosscher
On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote: This patch aims to improve debugging of optimized code. It ensures that PRE inserted statements have the same source location as the statement at the insertion point, instead of UNKNOWN_LOCATION. Wrong patch attached. However, is it really

Re: [PATCH] Update source location for PRE inserted stmt

2012-10-30 Thread Dehao Chen
Sorry, new patch attached... On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher stevenb@gmail.com wrote: On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote: This patch aims to improve debugging of optimized code. It ensures that PRE inserted statements have the same source location as the

Re: [PATCH] Update source location for PRE inserted stmt

2012-10-30 Thread Xinliang David Li
It will make the location info for the newly synthesized stmt more deterministic, I think. David On Tue, Oct 30, 2012 at 4:38 PM, Steven Bosscher stevenb@gmail.com wrote: On Wed, Oct 31, 2012 at 12:00 AM, Dehao Chen wrote: This patch aims to improve debugging of optimized code. It ensures