Skip to content

Document more remarks about the diagnostics CLI tools #22278

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

Merged
merged 3 commits into from
Jan 11, 2021

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Jan 8, 2021

Summary

Checks rest of the checkboxes from dotnet/diagnostics#1737

  • Tools targeting x86 need to be started using the x86 host.
  • Tools need to share a TMPDIR environment variable value with the target application.
  • Tools need run as same user as the target application to access diagnostic pipes

Part of dotnet/diagnostics#515.

@sywhang sywhang requested review from sdmaclea and a team as code owners January 8, 2021 22:57
@sywhang sywhang requested a review from josalem January 8, 2021 22:57
Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

We may want to add a note that says failing to do meet these requirements can result in <insert the error message> since the error message doesn't necessarily imply the problem in this case.

@@ -29,6 +29,9 @@ There are two ways to download and install `dotnet-counters`:
| macOS | [x64](https://aka.ms/dotnet-counters/osx-x64) |
| Linux | [x64](https://aka.ms/dotnet-counters/linux-x64) \| [arm](https://aka.ms/dotnet-counters/linux-arm) \| [arm64](https://aka.ms/dotnet-counters/linux-arm64) \| [musl-x64](https://aka.ms/dotnet-counters/linux-musl-x64) \| [musl-arm64](https://aka.ms/dotnet-counters/linux-musl-arm64) |

> [!NOTE]
> To use `dotnet-counters` on an x86 app, you need a corresponding x86 version of the tool.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear how to do this. It might be nice to have more detailed instructions somewhere. Having two versions of the tool on Windows is a bit difficult. I was using the same tool but executing it with the x86 runtime by pointing dotnet at dotnet-dump.dll. It is a bit awkward experience.

This also affects arm32 when analyzing cross dumps on Windows. So you currently need a 32bit dotnet tool process to debug a 32 bit target process or 32 bit dump.

So I guess you would see a similar problem on a arm64 host running a arm32 process. Although I can't say I have seen that problem. So it might not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback Steve.

AFAIK the easiest/pain-free way for the customers to do this would be to just use the single-file versions of these apps which is why I added the remark here (the most natural thing to do after reading this comment would be to just click the x86 link above).

Copy link
Contributor

Choose a reason for hiding this comment

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

Its OK to leave as is, I didn't really get my bearings. You could consider something like this ...

Suggested change
> To use `dotnet-counters` on an x86 app, you need a corresponding x86 version of the tool.
> To use `dotnet-counters` on an win-x86 app, you need a corresponding [win-x86](https://aka.ms/dotnet-counters/win-x86) direct download version of the tool.

Copy link
Contributor

@sdmaclea sdmaclea left a comment

Choose a reason for hiding this comment

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

LGTM May want to consider comment regarding x86 and 32-bit processes.

@sywhang sywhang merged commit 2e7e86c into dotnet:master Jan 11, 2021
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.

3 participants