Skip to content

Gdb Manual Remote launch targets for CoreBuild projects #1222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ewaterlander
Copy link
Contributor

Support the Gdb Remote TCP and Serial launch targets for CoreBuild projects. A new non-public LaunchConfiguration type is added, that is working with the same launch descriptor type as CoreBuild Local projects. Also a new LaunchConfigurationProvider that supports the Gdb Remote TCP and Serial targets is added.

It makes use of the launch delegate
org.eclipse.cdt.dsf.gdb.launching.GdbTargetedLaunchDelegate, which is also used for standard GDB (DSF) Manual Remote Debugging launch configurations for TCP and Serial connections.

For the user is appears as if there is a single CoreBuild launch configuration that adapts dynamically to the launch target, while in reality there is a separate launch configuration for each target.

Fixes #1168

@ewaterlander
Copy link
Contributor Author

Hi @betamaxbandit and @Kummallinen
would you like to review this?

CoreBuildGdbManualRemoteLaunchConfigProvider is at the moment a modified copy of the provider for Local. It needs further cleanup which I would like to discuss about.

With this change we will have working Gdb Remote launch targets for CMake and CoreBuild Makefile projects.


@Override
public boolean launchConfigurationChanged(ILaunchConfiguration configuration) throws CoreException {
// TODO Auto-generated method stub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D: remove comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


@Override
public void launchTargetRemoved(ILaunchTarget target) throws CoreException {
// TODO Auto-generated method stub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O: if you're sure there is nothing to do then say it in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the method.

* E.g. org.eclipse.cdt.debug.core.launch.CoreBuildGenericLaunchConfigProvider.
*/

public class CoreBuildGdbManualRemoteLaunchConfigProvider extends AbstractLaunchConfigProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O: hmm, my experience of writing launch config providers for Renesas is to use ProjectPerTargetLaunchConfigProvider.
With that approach, it is expected that any values in the launch target (eg hostname, port) are copied from the launch target into the launch configuration during populateLaunchConfiguration.

Additionally we have a unique launch configuration per launch target, so in createLaunchConfiguration the name also contains the launch target ID, like this:

	 /*
	 * Suffix the launch configuration with the launch target name, so its obvious to the user which one they are
	 * using.
	 */
	String name = launchManager.generateLaunchConfigurationName(descriptor.getName()//
			+ " " + target.getId());

When the user changes the launch target, the corresponding launch configuration is available when clicking the launchbar launch config gear. And any previously set values are shown.

Without this, I don't see the purpose of the launch target.

In this scenario, Renesas launch targets specify a target device. So maybe this approach is not appropriate when we're talking about a TCP/Serial connection.

This way of thinking for me may be so ingrained now that it's difficult for me to see any other way. Can you share your vision for this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @betamaxbandit

I updated my code. The launch config name is now unique and target attributes are populated into the configuration.

I did not know of ProjectPerTargetLaunchConfigProvider. It is not used in CDT, except in one test.
I will look into it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it is not critical to use ProjectPerTargetLaunchConfigProvider.
However, it does provide a getLaunch and getLaunchDescriptor method which might be useful.
Also another use of using ProjectPerTargetLaunchConfigProvider is it can be tested for using instanceof. I think this can be useful in launch classes to distinguish between a corebuild and non-corebuild launch.

Support the Gdb Remote TCP and Serial launch targets for CoreBuild
projects.  A new non-public LaunchConfiguration type is added, that is
working with the same launch descriptor type as CoreBuild Local
projects. Also a new LaunchConfigurationProvider that supports the Gdb
Remote TCP and Serial targets is added.

It makes use of the launch delegate
org.eclipse.cdt.dsf.gdb.launching.GdbTargetedLaunchDelegate, which is
also used for standard GDB (DSF) Manual Remote Debugging launch
configurations for TCP and Serial connections.

For the user is appears as if there is a single CoreBuild launch
configuration that adapts dynamically to the launch target, while in
reality there is a separate launch configuration for each target.

Also cleanup up a bit CoreBuildGenericLaunchConfigProvider and
fixed a bug in launchTargetRemoved().

Fixes eclipse-cdt#1168
Copy link
Contributor

@betamaxbandit betamaxbandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ewaterlander ,
this looks good to me. The only minor is the extension point comment.
Some of the checks are failing - I haven't investigated those.
The other comments I've made need some more thought and probably in a future commit.

It would be good to have a manual test for this, so that at least the typical expected user flow is known and documented.

@@ -123,4 +140,13 @@
</enablement>
</configProvider>
</extension>
<extension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O: this extension point could be merged with the one above.

return workingCopy.doSave();
}

private String getTargetConfigKey(ILaunchTarget target) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this method and the one in CoreBuildGenericLaunchConfigProvider

* E.g. org.eclipse.cdt.debug.core.launch.CoreBuildGenericLaunchConfigProvider.
*/

public class CoreBuildGdbManualRemoteLaunchConfigProvider extends AbstractLaunchConfigProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it is not critical to use ProjectPerTargetLaunchConfigProvider.
However, it does provide a getLaunch and getLaunchDescriptor method which might be useful.
Also another use of using ProjectPerTargetLaunchConfigProvider is it can be tested for using instanceof. I think this can be useful in launch classes to distinguish between a corebuild and non-corebuild launch.

Comment on lines +138 to +152
String targetTypeId = target.getTypeId();
if (targetTypeId.equals(GDBRemoteTCPLaunchTargetProvider.TYPE_ID)) {
String host = target.getAttribute(IGDBLaunchConfigurationConstants.ATTR_HOST, EMPTY);
String port = target.getAttribute(IGDBLaunchConfigurationConstants.ATTR_PORT, EMPTY);
workingCopy.setAttribute(IGDBLaunchConfigurationConstants.ATTR_REMOTE_TCP, true);
workingCopy.setAttribute(IGDBLaunchConfigurationConstants.ATTR_HOST, host);
workingCopy.setAttribute(IGDBLaunchConfigurationConstants.ATTR_PORT, port);
}
if (targetTypeId.equals(GDBRemoteSerialLaunchTargetProvider.TYPE_ID)) {
String serialPort = target.getAttribute(IGDBLaunchConfigurationConstants.ATTR_DEV, EMPTY);
String baudRate = target.getAttribute(IGDBLaunchConfigurationConstants.ATTR_DEV_SPEED, EMPTY);
workingCopy.setAttribute(IGDBLaunchConfigurationConstants.ATTR_REMOTE_TCP, false);
workingCopy.setAttribute(IGDBLaunchConfigurationConstants.ATTR_DEV, serialPort);
workingCopy.setAttribute(IGDBLaunchConfigurationConstants.ATTR_DEV_SPEED, baudRate);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see you are copying the values from the launch target to the launch config here.

If the user subsequently changes the values in the launch target, they are not be updated in the launch configuration. More thought is needed to handle this, maybe in another commit...

Perhaps there should be a new ILaunchConfigurationProvider2 added, which has launchTargetChanged. I'm just thinking aloud here. Maybe this is called in 2 places:

  • When the launch config is opened again.
  • During a launch, which allows the launch config to be synchronised with the launch target.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also related to this comment is another issue.

image

The Type control and other controls for TCP and Serial should be disabled when using a targetted launch, because now these values can be specified in 2 places; the launch target and the launch config. Which one takes precedence? I think the launch target values should. Therefore the user should not be aloud to change them in the launch config.

Maybe to assist the user when the controls are greyed out, there should be a helpful hint to change them in the launch target, in a tooltip or message somewhere

When using a non-targetted launch (ie the regular Debug Configurations dialog with delegate C/C++ Remote Applications using "GDB (DSF) Manual Remote Debugging Launcher"), the controls should be enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gdb Remote launch targets for CoreBuild projects
2 participants