Community
Participate
Working Groups
Standard source directory structure of an MBS project is <workspace-loc>/<project-name>/src/<file-name>. When debugging such a project and entering "break ./src/<file-name>:n" in the GDB console I get two breakpoints in the Breakpoint view. The reason is the synchronization mechanism introduced in https://bugs.eclipse.org/bugs/show_bug.cgi?id=392512 doesn't convert the debugger path when comparing the created platform breakpoint's file name with the target breakpoint's file name.
Pushed a fix to Gerrit: https://git.eclipse.org/r/#/c/9114/ Marc, please review it. The code is quite complicated but I couldn't come up with anything better.
I'm trying to understand the problem of this bug. I'm testing on Linux and here is what I see before the patch: 1- launch a session 2- in the gdb console type b ./src/DSFTestApp.cpp:197 3- GDB does not find the file and creates a PENDING breakpoint 4- the breakpoint synchronizer installs another bp using the absolute path. after the patch, step 4 does not happen. That means that the breakpoint is shown in the breakpoint view but is not installed on the target and therefore will not stop the target. I wonder if it wouldn't be more user-friendly to leave things as is? Eclipse forces the installation of a second breakpoint, which will actually hit, which is probably what the user intended. Or is there some other behavior I didn't notice? Note that removing the ./ prefix makes things work. I wonder why GDB behaves like that? 566,050 [MI] 28-interpreter-exec console "b src/DSFTestApp.cpp:197" 566,052 [MI] ~"Breakpoint 2 at 0x8048c0a: file ../src/DSFTestApp.cpp, line 197.\n" 566,053 [MI] =breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x08048c0a",func="main(int, char**)",file="../src/DSFTestApp.cpp",fullname="/home/lmckhou/runtime-TestDSF/DSFTestApp/src/DSFTestApp.cpp",line="197",times="0",original-location="src/DSFTestApp.cpp:197"} 566,053 [MI] 28^done
(In reply to comment #2) > I'm trying to understand the problem of this bug. > > I'm testing on Linux and here is what I see before the patch: > 1- launch a session > 2- in the gdb console type > b ./src/DSFTestApp.cpp:197 > 3- GDB does not find the file and creates a PENDING breakpoint > 4- the breakpoint synchronizer installs another bp using the absolute path. > > after the patch, step 4 does not happen. > That means that the breakpoint is shown in the breakpoint view but is not > installed on the target and therefore will not stop the target. > > I wonder if it wouldn't be more user-friendly to leave things as is? > Eclipse forces the installation of a second breakpoint, which will actually > hit, which is probably what the user intended. > > Or is there some other behavior I didn't notice? > My intention is to associate the breakpoints installed on the target with platform breakpoints. In this case we have a target breakpoint that has no platform breakpoint associated with it. I think it would be confusing for those who use the console to set breakpoints. In general, the logic in my previous implementation is wrong. It ignores the source lookup when trying to find a platform breakpoint for a target breakpoint. > Note that removing the ./ prefix makes things work. I wonder why GDB > behaves like that? > > 566,050 [MI] 28-interpreter-exec console "b src/DSFTestApp.cpp:197" > 566,052 [MI] ~"Breakpoint 2 at 0x8048c0a: file ../src/DSFTestApp.cpp, line > 197.\n" > 566,053 [MI] > =breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep", > enabled="y",addr="0x08048c0a",func="main(int, > char**)",file="../src/DSFTestApp.cpp",fullname="/home/lmckhou/runtime- > TestDSF/DSFTestApp/src/DSFTestApp.cpp",line="197",times="0",original- > location="src/DSFTestApp.cpp:197"} > 566,053 [MI] 28^done I don't know. Setting a breakpoint at ../src/DSFTestApp.cpp would work even if the work directory is the project directory.
(In reply to comment #3) > (In reply to comment #2) > > I'm trying to understand the problem of this bug. > > > > I'm testing on Linux and here is what I see before the patch: > > 1- launch a session > > 2- in the gdb console type > > b ./src/DSFTestApp.cpp:197 > > 3- GDB does not find the file and creates a PENDING breakpoint > > 4- the breakpoint synchronizer installs another bp using the absolute path. > > > > after the patch, step 4 does not happen. > > That means that the breakpoint is shown in the breakpoint view but is not > > installed on the target and therefore will not stop the target. > > > > I wonder if it wouldn't be more user-friendly to leave things as is? > > Eclipse forces the installation of a second breakpoint, which will actually > > hit, which is probably what the user intended. > > > > Or is there some other behavior I didn't notice? > > > > My intention is to associate the breakpoints installed on the target with > platform breakpoints. In this case we have a target breakpoint that has no > platform breakpoint associated with it. I think it would be confusing for > those who use the console to set breakpoints. Ok, except that we have a similar situation when creating a second breakpoint at the same line. If I create a platform bp at line 20 and then create one using the gdb console at the same line 20, that second target bp does not seem to get associated with the platform bp, or at least is hit an error. Should I open a new bug? > In general, the logic in my previous implementation is wrong. It ignores the > source lookup when trying to find a platform breakpoint for a target > breakpoint. You're right. The code looks good. Cosmetics comments in Gerrit.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > I'm trying to understand the problem of this bug. > > > > > > I'm testing on Linux and here is what I see before the patch: > > > 1- launch a session > > > 2- in the gdb console type > > > b ./src/DSFTestApp.cpp:197 > > > 3- GDB does not find the file and creates a PENDING breakpoint > > > 4- the breakpoint synchronizer installs another bp using the absolute path. > > > > > > after the patch, step 4 does not happen. > > > That means that the breakpoint is shown in the breakpoint view but is not > > > installed on the target and therefore will not stop the target. > > > > > > I wonder if it wouldn't be more user-friendly to leave things as is? > > > Eclipse forces the installation of a second breakpoint, which will actually > > > hit, which is probably what the user intended. > > > > > > Or is there some other behavior I didn't notice? > > > > > > > My intention is to associate the breakpoints installed on the target with > > platform breakpoints. In this case we have a target breakpoint that has no > > platform breakpoint associated with it. I think it would be confusing for > > those who use the console to set breakpoints. > > Ok, except that we have a similar situation when creating a second > breakpoint at the same line. If I create a platform bp at line 20 and then > create one using the gdb console at the same line 20, that second target bp > does not seem to get associated with the platform bp, or at least is hit an > error. Should I open a new bug? > Yes, please. We need to figure out how to handle this case. > > In general, the logic in my previous implementation is wrong. It ignores the > > source lookup when trying to find a platform breakpoint for a target > > breakpoint. > > You're right. The code looks good. Cosmetics comments in Gerrit. Thanks!
(In reply to comment #5) > > Ok, except that we have a similar situation when creating a second > > breakpoint at the same line. If I create a platform bp at line 20 and then > > create one using the gdb console at the same line 20, that second target bp > > does not seem to get associated with the platform bp, or at least is hit an > > error. Should I open a new bug? > > > > Yes, please. We need to figure out how to handle this case. I've opened Bug 397460 and posted your proposed fix to Gerrit.
Marc, I changed the code as you suggested (btw, nice trick!) and pushed the new version of the patch to Gerrit. Can you please review that part of the code? I modified your code to handle the case when an error appears in the source lookup and would appreciate your opinion. Thanks!
Checked in. Thanks Marc.
This patch doesn't work when the source lookup contains a path mapping. Moreover it breaks the normal behavior. I suggest reverting it. Marc, what do you think?
(In reply to comment #9) > This patch doesn't work when the source lookup contains a path mapping. > Moreover it breaks the normal behavior. I suggest reverting it. > Marc, what do you think? We're using the SourceLookup service in the bp synchronizer, which should handle path mappings. Why aren't things working?
(In reply to comment #10) > (In reply to comment #9) > > This patch doesn't work when the source lookup contains a path mapping. > > Moreover it breaks the normal behavior. I suggest reverting it. > > Marc, what do you think? > > We're using the SourceLookup service in the bp synchronizer, which should > handle path mappings. Why aren't things working? The usage of the source lookup in the patch is wrong. It's just a coincidence that it works for this case. The main problem is inconsistency between the GDB source lookup and the CDT source lookup. When a breakpoint is set at ./src/DSFTestApp.cpp GDB doesn't find the source file but the CDT does. MIBreakpointsSynchronizer creates a platform breakpoint using the full path (/workspace/project/src/DSFTestApp.cpp). When the platform breakpoint path is passed to MIBreakpointsSynchronizer.getTargetBreakpoint() is already converted to the debugger path by MIBreakpointsManager (in this case it is the same /workspace/project/src/DSFTestApp.cpp path which is important). findTargetLineBreakpoint() converts the path of all existing target breakpoints to the platform paths and compares them with the debugger path. As a result ./src/DSFTestApp.cpp gets converted to /workspace/project/src/DSFTestApp.cpp and "found". So, a duplicate target breakpoint is not created. But if there is a path mapping in the source lookup, the mismatch between the platform and debugger path prevents MIBreakpointsSynchronizer to find the corresponding target breakpoint. It is broken for all breakpoints including non-pending. It's all very complicated, no wonder I missed it when I was implementing the patch.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > This patch doesn't work when the source lookup contains a path mapping. > > > Moreover it breaks the normal behavior. I suggest reverting it. > > > Marc, what do you think? > > > > We're using the SourceLookup service in the bp synchronizer, which should > > handle path mappings. Why aren't things working? > > The usage of the source lookup in the patch is wrong. It's just a > coincidence that it works for this case. The main problem is inconsistency > between the GDB source lookup and the CDT source lookup. > When a breakpoint is set at ./src/DSFTestApp.cpp GDB doesn't find the source > file but the CDT does. MIBreakpointsSynchronizer creates a platform > breakpoint using the full path (/workspace/project/src/DSFTestApp.cpp). When > the platform breakpoint path is passed to > MIBreakpointsSynchronizer.getTargetBreakpoint() is already converted to the > debugger path by MIBreakpointsManager (in this case it is the same > /workspace/project/src/DSFTestApp.cpp path which is important). > findTargetLineBreakpoint() converts the path of all existing target > breakpoints to the platform paths and compares them with the debugger path. > As a result ./src/DSFTestApp.cpp gets converted to > /workspace/project/src/DSFTestApp.cpp and "found". So, a duplicate target > breakpoint is not created. > But if there is a path mapping in the source lookup, the mismatch between > the platform and debugger path prevents MIBreakpointsSynchronizer to find > the corresponding target breakpoint. It is broken for all breakpoints > including non-pending. > It's all very complicated, no wonder I missed it when I was implementing the > patch. Thanks for the explanation! Yes, that didn't jump out at us at all :) Is it correct to say that the patch put in for this bug does improve the situation a little? In that case, maybe it would be better to keep things as is, and properly fix this bug for the SR1 release?
(In reply to comment #12) > Is it correct to say that the patch put in for this bug does improve the > situation a little? In that case, maybe it would be better to keep things > as is, and properly fix this bug for the SR1 release? It does improve this particular situation which only applies to breakpoints set using correct relative paths that are not recognized by GDB. But it breaks much more common case for all line breakpoints set from the console that can be affected by a path mapping defined in the source lookup. I would rather keep this bug open and revert the patch.
(In reply to comment #13) > (In reply to comment #12) > > Is it correct to say that the patch put in for this bug does improve the > > situation a little? In that case, maybe it would be better to keep things > > as is, and properly fix this bug for the SR1 release? > > It does improve this particular situation which only applies to breakpoints > set using correct relative paths that are not recognized by GDB. But it > breaks much more common case for all line breakpoints set from the console > that can be affected by a path mapping defined in the source lookup. > I would rather keep this bug open and revert the patch. Ok, makes sense
Reverting the commit required some manual intervention so I'm pushing it to gerrit: https://git.eclipse.org/r/13528 Since RC3 hasn't been built yet, I'll commit it now. I have checked that the JUnit tests still work. I'd like Mikhail to confirm that it fixes the path mapping issue since I haven't been able to reproduce it myself.
(In reply to comment #15) > Reverting the commit required some manual intervention so I'm pushing it to > gerrit: > https://git.eclipse.org/r/13528 > > Since RC3 hasn't been built yet, I'll commit it now. I have checked that > the JUnit tests still work. I'd like Mikhail to confirm that it fixes the > path mapping issue since I haven't been able to reproduce it myself. I pushed a second patchset to gerrit. It is simpler than the first one and keeps the new API. I was thinking that this new API will be required when trying to fix this bug again, so it would be better to keep it. I've committed this second patch set to master. Mikhail, can you please review and also confirm it fixes the problem?
(In reply to comment #16) > (In reply to comment #15) > > Reverting the commit required some manual intervention so I'm pushing it to > > gerrit: > > https://git.eclipse.org/r/13528 > > > > Since RC3 hasn't been built yet, I'll commit it now. I have checked that > > the JUnit tests still work. I'd like Mikhail to confirm that it fixes the > > path mapping issue since I haven't been able to reproduce it myself. > > I pushed a second patchset to gerrit. It is simpler than the first one and > keeps the new API. I was thinking that this new API will be required when > trying to fix this bug again, so it would be better to keep it. > > I've committed this second patch set to master. > > Mikhail, can you please review and also confirm it fixes the problem? I tried the latest master on Windows with MinGW and Cygwin. The latter's source lookup contains the path mapping that maps Cygwin paths to Windows paths. In both cases I was able to set console breakpoints using the file name which is good enough for this release. Regarding this particular bug, I think we need to try making the CDT source lookup consistent with GDB.
This bug was assigned and targeted at a now released milestone (or Future or Next that isn't being used by CDT). As that milestone has now passed, the milestone field has been cleared. If this bug has been fixed, please set the milestone to the version it was fixed in and mark the bug as resolved.
