Skip to content
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

Fix WAMR IDE Debugger Ignoring Launch Config #2155

Conversation

MrSarius
Copy link
Contributor

@MrSarius MrSarius commented Apr 26, 2023

The DebugConfigurationProvider was overwriting configurations provided in launch.json. In particular, this for example prevented from specifying a custom port for the debugger.

Example launch.json

{
    "configurations": [
        {
            "type": "wamr-debug",
            "request": "attach",
            "name": "Attach Debugger",
            "stopOnEntry": true,
            "attachCommands": [
                "process connect -p wasm connect://127.0.0.1:1237"
            ]
        }
    ]
}

MrSarius and others added 2 commits April 26, 2023 13:47
- debugger was ignoring configurations provided in launch.json
- updated package version to current WAMR version

public setDebugConfig(hostPath: string, port: number): void {
this.port = port;
this.hostPath = hostPath;
/* linux and windows has different debug configuration */
if (os.platform() === 'win32' || os.platform() === 'darwin') {
Copy link
Contributor

Choose a reason for hiding this comment

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

The os.platform() === 'linux' branch has been removed, does it work on linux without that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

['initCommands']: ['platform select remote-linux'] was the only attributes that was added to the configuration-object in that branch. The rest was redundant.
So if if os.platform() === 'win32' || os.platform() === 'darwin' is true, we add the respective additional configuration to the default.

@TianlongLiang
Copy link
Contributor

Hi, there, this pr is a great idea and I appreciate your enthusiasm to contribute to our project. I reviewed your code and tested it. There may be some small extra discussions and modifications needed.

Firstly, modifying the port number in the configuration alone is not sufficient, we need to also find a way to pass the port number to debug.sh inside docker image to let iwasm use that port too. In order to do that, we can modify the shell, let it take an extra argument, and in provideTasks(), pass that port number along. But I am not sure what's the best way to get port number from configuration in that function, any ideas?

Secondly, when I was debugging the extension to use the debug feature, sometimes I didn't see resolveDebugConfiguration() get called(I am not sure whether a specific operation order resulted in this). When it does, It will leave this.wasmDebugConfig undefined, maybe we should go back to the old way of explicitly using setter and only use promise in resolveDebugConfiguration(), what do you think?

@MrSarius
Copy link
Contributor Author

MrSarius commented Apr 28, 2023

Firstly, modifying the port number in the configuration alone is not sufficient, ...

True, I was mainly using the debugger outside the container.
Is there anything wrong with simply passing port and host as program parameters to the script?

Secondly, when I was debugging the extension to use the debug feature, sometimes I didn't see resolveDebugConfiguration() get called../.

resolveDebugConfiguration() is not always called? Is this a VS code bug? Shouldn't be like that.
But it definitely makes sense to set this.wasmDebugConfig explicitly anyways.

@TianlongLiang
Copy link
Contributor

I'm sorry for not getting back to you sooner. I was off for labor day.

True, I was mainly using the debugger outside the container. Is there anything wrong with simply passing port and host as program parameters to the script?

I feel like we might be talking about different docker containers. Let's make sure we are on the same page. I am not talking about the dev container but the two containers that wamr-ide used. When we click some button in the extension, for example, the build button, wasm-toolchain docker image will be run to provide build environment. Likewise, when clicking the run/debug button, wasm-debug-server docker image will be run. So when clicking debug button, if we want to use a non-default port we specified in the launch config, we have to let the iwasm inside wasm-debug-server use that same port.

resolveDebugConfiguration() is not always called? Is this a VS code bug? Shouldn't be like that. But it definitely makes sense to set this.wasmDebugConfig explicitly anyways.

I am not sure whether this is caused by a specific VS Code version or my debug setting(or maybe the npm installed package is not debug version). Anyway, I can't set a breakpoint inside that function and observe it got called when debugging, and it will make this.wasmDebugConfig undefined.

@MrSarius
Copy link
Contributor Author

MrSarius commented May 5, 2023

Honestly, I would not change anything in the debug script. My change was more to use the debugger outside the IDE if you start your own debug server but still want to use the VS code debugger. In this case, it was simply very unintuitive that the available configuration parameters were simply ignored.

We could of course cut the port out of the attachedComments and pass it to the  boot_debugger_server.sh  script as a third parameter:

docker run --rm -it --name=wasm-debug-server-ctr \
           -v "$(pwd)":/mnt \
           -p $3:1234 \
           wasm-debug-server:$2 \
           /bin/bash -c "./debug.sh $1"

However, this would only make sense if someone tried to use the IDE while port 1234 is already in use on their machine. What do you think?

@TianlongLiang
Copy link
Contributor

I was debugging the extension directly, like in this document and simply click the debug button.

My change was more to use the debugger outside the IDE if you start your own debug server but still want to use the VS code debugger.

It sounds like a different testing way compared to my testing method. Would you mind telling me the process of how to test with this method so that I can have a try?

@MrSarius
Copy link
Contributor Author

MrSarius commented May 6, 2023

I am talking about debugging my wasm code, not the extension.

I start my own debugging server on a port other than 1234. (The code was compiled with debug symbols.)
iwasm -g=127.0.0.1:1237 main.wasm

Then I open VS Code with the extension installed and configure my launch.json accordingly. I press F5 and run the debugger with my settings.

{
    "configurations": [
        {
            "type": "wamr-debug",
            "request": "attach",
            "name": "Attach Debugger",
            "stopOnEntry": true,
            "attachCommands": [
                "process connect -p wasm connect://127.0.0.1:1237"
            ]
        }
    ]
}

Of course my implementation expects resolveDebugConfiguration() to be called. Still need to fix that.

@MrSarius MrSarius force-pushed the benjuri/fix-wamr-ide-debugger-ignoring-launch-config branch from 697936b to 257ce36 Compare May 6, 2023 11:07
@TianlongLiang
Copy link
Contributor

Thanks! Now I truly understand what this PR is about. Frankly speaking, I wasn't using VS Code and writing launch.json for debugging as much, it's really great that this time our discussion gets me learning and fills some of the voids.

- apparently vs code sometimes does not call resolveDebugConfiguration()
@MrSarius MrSarius force-pushed the benjuri/fix-wamr-ide-debugger-ignoring-launch-config branch from f7e3c0d to 23bba90 Compare May 9, 2023 12:57
@MrSarius
Copy link
Contributor Author

MrSarius commented May 9, 2023

My PR should be ready now. The wasmDebugConfig is now never undefined, although I couldn't recreate the case where resolveDebugConfiguration() was not called by VS Code.

If anyone else is testing this PR: The April update of VS Code breaks the debugging protocol. Therefore please use version 1.77.

@TianlongLiang
Copy link
Contributor

LGTM

@wenyongh wenyongh merged commit ff0752b into bytecodealliance:main May 10, 2023
1 check passed
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
The `DebugConfigurationProvider` was overwriting configurations provided
in `launch.json`. In particular, this for example prevented from specifying a
custom port for the debugger.

Example `launch.json`
```
{
    "configurations": [
        {
            "type": "wamr-debug",
            "request": "attach",
            "name": "Attach Debugger",
            "stopOnEntry": true,
            "attachCommands": [
                "process connect -p wasm connect://127.0.0.1:1237"
            ]
        }
    ]
}
```

Co-authored-by: Ben Riegel <benjuri@amazon.com>
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.

None yet

4 participants