Improvements for Breakpoint Logic

[asimgunes]

Hi Eclipse GDB Adapter Team,

Here in this update, we tried to improve the breakpoint logic of the cdt-gdb-adapter to provide capability to dynamically control the breakpoint options in runtime.

The idea came from a problem we encounter during the insert breakpoint operation, finally we want to introduce a new feature where breakpoint options could be dynamically handled by derived classes of GDBBackend (especially type of breakpoint [hardware/software] for our case).

Through this update, we introduce a new method getBreakpointOptions to GDBBackend class. With this new functionality, any derived class of GDBBackend could override the breakpoint options.

We believe this will improve user experience in embedded systems by managing the limited breakpoint capability more efficiently. Besides, any error thrown in the getBreakpointOptions if no more breakpoints are available in the device will automatically disable the breakpoint in VSCode user interface; and by already implemented nature of the cdt-gdb-adapter if one of the breakpoints are removed, the next available breakpoint is enabled automatically.

There are also some refactoring performed during the operation. Perhaps, not all the refactorings are must to have, but we tried to handle the namings of functions/methods and object structure to be semantically coherent after the update.

Please review this update in your suitable time and let us know if you have feedbacks. We will be happy if you consider this update and merge to the project main stream.

Kind regards Asim Gunes

[asimgunes]

This change looks fine to me - I have sent an email to cdt-cloud-dev (link should work soon) to cover the API changing.

What is needed here though are some tests of this new API. e.g. something that tries to send a hardware breakpoint but getBreakpointOptions changes it to sw (or vice versa).

Please let me know if you need advice on how to write such a test.

PS Please also check your contributor agreement status as the ECA check failed.

Hi @jonahgraham,

I have also added new integration tests for getBreakpointOptions method, However, one of my tests are falling into 'timeout' time to time. When I trigger the test alone they are working; when I run all the tests, in some conditions it is falling into timeout error on afterEach operation where it is trying to stop the debugClient.

I couldn't realise any particular reason or pattern for this error, perhaps it is going to cause problem on ci-tests. The error is thrown from src/integration-tests/dynamicBreakpointOptions.spec.ts @line:134. Do you have any suggestion to control this?

Additionally, I add some scripts to the package.json and I feel like the scripts are growing and could cause maintenance overhead in the future. I can remove the additional scripts for simplicity (I thought other cases are still checking all the combinations). Or, I can leave it like this (explicitly defined) to make sure tests are performed in the future if other script conditions are changed. So, please check this part too, I am happy to send an additional commit if there is a change request comes for the script definitions.

Kind regards Asim

[asimgunes]

Hi @jonahgraham, I have also added new integration tests for getBreakpointOptions method, However, one of my tests are falling into 'timeout' time to time. When I trigger the test alone they are working; when I run all the tests, in some conditions it is falling into timeout error on afterEach operation where it is trying to stop the debugClient. I couldn't realise any particular reason or pattern for this error, perhaps it is going to cause problem on ci-tests. The error is thrown from src/integration-tests/dynamicBreakpointOptions.spec.ts @line:134. Do you have any suggestion to control this?

If we see it on the test machine I may be a little concerned, but looking at your code I am not sure of what it could be. Sometimes operations timeouts are simply too short (which is why .mocharc-windows-ci.json exists)

Additionally, I add some scripts to the package.json and I feel like the scripts are growing and could cause maintenance overhead in the future. I can remove the additional scripts for simplicity (I thought other cases are still checking all the combinations). Or, I can leave it like this (explicitly defined) to make sure tests are performed in the future if other script conditions are changed. So, please check this part too, I am happy to send an additional commit if there is a change request comes for the script definitions.

It would be great to know how to better handle these scripts. It is unwieldy at the moment. As you can probably see the issue of how to run the same tests under different conditions is quite important and I am certainly interested if there is a better way to do that.

I don't think the new scripts you have added fall under that purpose, so I would remove the extra script entries.

I rollback the script changes.

[jonahgraham]

@asimgunes the tests failed on the GitHub actions. Can you review and let me know what is expected here.

PS. Until a PR is merged the GitHub actions won't run automatically. You can enable github actions in your fork and see test results without having to wait for me to hit run on them.

[jonahgraham]

@asimgunes Please also see https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/issues/277 - it doesn't explain the test failures, but does explain some of the orphaned processes.

[asimgunes]

Hi @jonahgraham,

I just want to share my observations: setting hardware breakpoints are causing timeout in Windows but working fine in Linux tests, I recently add "skip" in my test which is testing hardware breakpoint and it is working fine without this test.

In other test definitions, I observe some conditional skip's on Windows operating system and I wonder if I need to skip hardware breakpoint test in Windows or am I missing something in my implementation.

I also see your ticket #277, maybe there could be a relation between hardware breakpoints, the testing machine and windows.

[jonahgraham]

Yes - can't set hardware breakpoints on Windows - what I don't know is why that causes other tests to fail by having it on for one.

You should indeed to a conditional skip if os.platform() === 'win32' for tests that need to use hardware breakpoints.

When running the --test-hw-breakpoint-on part of the test matrix, the windows tests are simply skipped:

https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/419e439a2fc0a9d678551c554215596306a7fc9a/src/integration-tests/utils.ts#L244-L247

277 doesn't relate to this problem as it preexisted before hardware breakpoints were introduced.

[asimgunes]

Hi @jonahgraham,

I skipped my hardware breakpoint test for windows and removed the logger from the implementation. Tests completed successfully on my side, I suppose it is ready to go for your review.

Kind regards.

[jonahgraham]

It looks like your formatting is oscillating between yarn format output and something else. I will rebase your change to have the formatting change applied as a separate commit, it looks like my plan of adding yarn format-check to the CI never happened :(

[jonahgraham]

my plan of adding yarn format-check to the CI never happened :(

It is now on its way in #278