On 2/14/20 9:05 PM, Bernd Edlinger wrote: > On 2/11/20 2:57 PM, Andrew Burgess wrote: >> * Bernd Edlinger [2020-02-05 17:55:37 +0000]: >> >>> On 2/5/20 12:37 PM, Andrew Burgess wrote: >>> >>> did you mean, when we don't land in the middle of a line ? >> >> No, in this case I did write the correct thing. >> >> So GDB had/has some code designed to "improve" the handling of looping >> constructs. I think the idea is to handle cases like this: >> >> 0x100 LN=5 >> 0x104 <-\ >> 0x108 | >> 0x10c | LN=6 >> 0x110 | >> 0x114 --/ >> >> So when line 6 branches back to the middle of line 5, we don't stop >> (because we are in the middle of line 5), but we do want to stop at >> the start of line 6, so we "reset" the starting point back to line 5. >> >> This is done by calling set_step_info in process_event_stop_test, in >> infrun.c. >> >> What we actually did though was whenever we were at an address that >> didn't exactly match a SAL (in the example above, marked LN=5, LN=6) >> we called set_step_info. This worked fine, at address 0x104 we reset >> back to LN=5, same at address 0x108. >> >> However, in the new world things can look different, for example, like >> this: >> >> 0x100 LN=5,STMT=1 >> 0x104 >> 0x108 LN=6,STMT=0 >> 0x10c LN=6,STMT=1 >> 0x110 >> 0x114 >> >> In this world, when we move forward to 0x100 we don't have a matching >> SAL, so we get the SAL for address 0x100. We can then safely call >> set_step_info like we did before, that's fine. But when we step to >> 0x108 we do now have a matching SAL, but we don't want to stop yet as >> this address is 'STMT=0'. However, if we call set_step_info then GDB >> is going to think we are stepping away from LN=6, and so will refuse >> to stop at address 0x10c. >> >> The proposal in this commit is that if we land at an address which >> doesn't specifically match a SAL, 0x104, 0x110, 0x114 in the above >> example, then we do call set_step_info, but otherwise we don't. >> >> There are going to be situations where the behaviour of GDB changes >> after this patch, but I don't think we can avoid that. What we had >> previously was just a heuristic. I think enabling this new >> information in GDB will allow us to do better overall, so I think we >> should make this change and fix any issues as they show up. >> > > I agree with that, thanks for this explanation. > > However, I think I found a small problem in this patch. > You can see it with the next-inline test case. > When you use just step the execution does not stop > between the inline functions, because not calling > > current behaviour is this: > > Breakpoint 1, main () at next-inline.cc:63 > 63 get_alias_set (&xx); > (gdb) s > get_alias_set (t=0x404040 ) at next-inline.cc:50 > 50 if (t != NULL > (gdb) s > 51 && TREE_TYPE (t).z != 1 > (gdb) s > 0x0000000000401169 in tree_check (i=0, t=0x404040 ) at next-inline.h:34 > 34 if (t->x != i) > (gdb) s > 0x000000000040117d in tree_check (i=0, t=0x404040 ) at next-inline.h:34 > 34 if (t->x != i) > (gdb) s > 0x000000000040118f in tree_check (i=0, t=0x404040 ) at next-inline.h:34 > 34 if (t->x != i) > (gdb) s > main () at next-inline.cc:64 > 64 return 0; > (gdb) > > I debugged a bit because I was not sure why that happens, and it looks > like set_step_info does also set the current inline frame, but you > only want to suppress the line number not the currently executing > inline function. > With this small patch the step works as expected. > Hmm, I think this got stuck, so I just worked a follow-up patch out for you. I also noticed that the test case cannot work with gcc before gcc-8, since the is_stmt feature is not implemented earlier, at least with gcc-4.8 and gcc-5.4 the test case fails. I thought it would be good to detect that by adding the -gstatement-frontiers option, so that the gcc driver complains when it is not able to generate sufficient debug info for that test. Thoughts? Bernd.