setBreakpoints and setFunctionBreakpoints don't respect spec

[jonahgraham]

I am reviewing the setFunctionBreakpoints and setBreakpoints a little more closely after running into a problem integrating the adapter into Eclipse with LSP4E.

There are a few issues with breakpoint handling that needs addressing, I am using this one issue to track them.

• breakpoints simultaneously in multiple files
• breakpoints simultaneously in multiple files on Windows
• comparing of filename is problematic (see conversation in Fix breakpoints being set in multiple files #156)
• "bad" breakpoints are causing error response instead of breakpoints response with unverified breakpoints (see Breakpoint.verified and Breakpoint.message)
• the order of Breakpoints in the responses' body does not follow "The array elements are in the same order as the elements of the 'breakpoints'"
• breakpoints that are inserted on relocated lines do not have the relocated line reported back in the response
• breakpoints requested on lines that GDB relocates causes subsequent setBreakpoints to reinsert the breakpoint instead of reusing the already relocated breakpoint. This is a particular problem when hit counting is being used
• breakpoints with multiple locations (identified with <MULTIPLE> in addr field from gdb and sub breakpoints, e.g. 1.1 and 1.2 as ids) causes issues as adapter does not identify them as working together and in some cases tries to delete the multiple values.
• MI parser does not parse return values with <MULTIPLE> properly on -break-insert - it does handle it properly on -break-list because GDB has a different format on -break-insert result. The bkpt= is not duplicated on the insert case.
• Some behaviour is conditional on GDB version - e.g. < 8.2 does not report relocated breakpoint new line number - therefore tests need to be dependent on GDB version and we need to start running multiple versions

[jonahgraham]

See PR #154 - I have started adding (now failing) tests for each of the issues already. Only "breakpoints with multiple locations" issue does not cause a test case failure - but the MI log shows a problem.

[jonahgraham]

Only "breakpoints with multiple locations" issue does not cause a test case failure - but the MI log shows a problem.

Latest update to PR #154 makes this fail too because instead of silently logging log-output-stream (&) messages, I changed it to be an output event on the 'log' stream. This allows the test to verify that the stream does not have a warning.

[jonahgraham]

I added "breakpoints simultaneously in multiple files". This may be a regression, not too sure, but I have added a new test for a simple case.

[jonahgraham]

This may be a regression

@thegecko this is a regression as a result of #136

[thegecko]

OK, I imagine the problem will be around here, although I can't immediately see why:

https://github.com/eclipse-cdt/cdt-gdb-adapter/blob/master/src/GDBDebugSession.ts#L280

If you've added a failing test I can look tomorrow if you get stuck.

[thegecko]

Oh, I think I see it. I assume setBreakPointsRequest() is called once per file? In which case, each subsequent call will create a delete list containing all breakpoints not in the current file.

I think the fix is to filter out all breakpoints except the ones for the file being dealt with. e.g.:

https://github.com/eclipse-cdt/cdt-gdb-adapter/blob/master/src/GDBDebugSession.ts#L261

  const file = args.source.path as string;
  const gdbbps = result.BreakpointTable.body.filter((gdbbp) => {
    // Only deal with breakpoints in the current file
    if (gdbbp.fullname !== file) {
      return false;
    }
    // Ignore function breakpoints
    if (this.functionBreakpoints.indexOf(gdbbp.number) > -1) {
      return false;
    }
    return true;
  });

and

https://github.com/eclipse-cdt/cdt-gdb-adapter/blob/master/src/GDBDebugSession.ts#L280

  return !!(gdbbp.line && parseInt(gdbbp.line, 10) === vsbp.line
    && vsbpCond === gdbbpCond);

[jonahgraham]

Thanks @thegecko for the fast turnaround on fixing this. The other changes needed to address the other parts of the issue here may change the code again. I am working on the other fixes, so let me know if you are looking at any of that.

[thegecko]

let me know if you are looking at any of that

No plans, but I'd be keen to see these fixes published as soon as possible :)

[jonahgraham]

The fixes are individually fairly straightforward - fixes on the way soon, hopefully today or tomorrow.

[jonahgraham]

The cod that compares file names is still "dodgy" - see conversation in #156

[thegecko]

🐟

[jonahgraham]

🐟

I want to fix my typo - but then no one will get the joke.

[jonahgraham]

Everything is done in the list - except making test version dependent. The ones requiring >=8.2 are simply skipped all the time. I am going to roll this into the greater discussion of #159