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

Use DOTNET_ variables in tests #76997

Merged
merged 8 commits into from
Oct 19, 2022
Merged

Conversation

am11
Copy link
Member

@am11 am11 commented Oct 13, 2022

Renamed COMPlus_ to DOTNET_ under src/tests, except for configuration tests.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 13, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Renamed COMPlus_ to DOTNET_ under src/tests, except for configuration tests.

Author: am11
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@am11 am11 added area-Infrastructure and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Renamed COMPlus_ to DOTNET_ under src/tests, except for configuration tests.

Author: am11
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Renamed COMPlus_ to DOTNET_ under src/tests, except for configuration tests.

Author: am11
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: -

@am11
Copy link
Member Author

am11 commented Oct 13, 2022

@ViktorHofer I used neutral infrastructure label as src/tests has tests for both runtimes, and DOTNET_ / COMPlus_ variables are handled by both with separate implementations (DOTNET_ is checked first and COMPlus is a legacy fallback, hence this PR).

@AaronRobinsonMSFT
Copy link
Member

@am11 This is great. One concern I have is in superpmi - see below. I never updated this path because I wasn't sure all the scenarios and just haven't gotten back to it. @BruceForstall Can you provide some details on when the following is used? Does the JIT team use superpmi with the test suite that is being updated here?

// Look for 'key' as an environment variable named COMPlus_<key>. The returned value
// is nullptr if it is not found, or a string if found. If not nullptr, the returned
// value must be freed with jitInstance.freeLongLivedArray(value).
WCHAR* GetCOMPlusVariable(const WCHAR* key, JitInstance& jitInstance)
{
static const WCHAR Prefix[] = W("COMPlus_");
static const size_t PrefixLen = (sizeof(Prefix) / sizeof(Prefix[0])) - 1;
// Prepend "COMPlus_" to the provided key
size_t keyLen = wcslen(key);
size_t keyBufferLen = keyLen + PrefixLen + 1;
WCHAR* keyBuffer =
reinterpret_cast<WCHAR*>(jitInstance.allocateArray(sizeof(WCHAR) * keyBufferLen));
wcscpy_s(keyBuffer, keyBufferLen, Prefix);
wcscpy_s(&keyBuffer[PrefixLen], keyLen + 1, key);
// Look up the environment variable
DWORD valueLen = GetEnvironmentVariableW(keyBuffer, nullptr, 0);
if (valueLen == 0)
{
jitInstance.freeArray(keyBuffer);
return nullptr;
}
// Note this value must live as long as the jit instance does.
WCHAR* value = reinterpret_cast<WCHAR*>(jitInstance.allocateLongLivedArray(sizeof(WCHAR) * valueLen));
DWORD newValueLen = GetEnvironmentVariableW(keyBuffer, value, valueLen);
jitInstance.freeArray(keyBuffer);
if (valueLen < newValueLen)
{
jitInstance.freeLongLivedArray(value);
return nullptr;
}
return value;
}

@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Renamed COMPlus_ to DOTNET_ under src/tests, except for configuration tests.

Author: am11
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

@am11
Copy link
Member Author

am11 commented Oct 13, 2022

@AaronRobinsonMSFT, those are defined in superpmi.py which I haven't changed.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

It's nice to move everything to our current recommended format.

I worry about how we can properly test this. We could easily lose coverage and not notice it.

In addition, there are other places that set/use COMPlus variables, e.g., src\coreclr\scripts, https://github.com/dotnet/jitutils

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 13, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 13, 2022
@build-analysis build-analysis bot mentioned this pull request Oct 14, 2022
2 tasks
@am11 am11 closed this Oct 14, 2022
@am11 am11 reopened this Oct 14, 2022
@danmoseley
Copy link
Member

Dotnet/diagnostics also has a bunch, but the docs in some cases may apply also to older runtimes.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

@BruceForstall
Copy link
Member

@AaronRobinsonMSFT Do you want to review?

@am11
Copy link
Member Author

am11 commented Oct 14, 2022

@BruceForstall, #77025 is merged, I have pushed superpmi scripts change here. Also, there were few redundant exports which I've cleaned up. Please take another look. :)

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Additional changes LGTM; thanks for doing that.

@BruceForstall BruceForstall merged commit 51e3afb into dotnet:main Oct 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants