Skip to content

Use DOTNET_ variables in tests#76997

Merged
BruceForstall merged 8 commits intodotnet:mainfrom
am11:feature/tests/setup
Oct 19, 2022
Merged

Use DOTNET_ variables in tests#76997
BruceForstall merged 8 commits intodotnet:mainfrom
am11:feature/tests/setup

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
@ghost ghost 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

AaronRobinsonMSFT commented Oct 13, 2022

@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?

runtime/src/coreclr/tools/superpmi/superpmi/jithost.cpp

Lines 10 to 46 in 6556178

// 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.

BruceForstall
BruceForstall suggested changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@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
Closed
2 tasks
@am11 am11 closed this Oct 14, 2022
@am11 am11 reopened this Oct 14, 2022
@danmoseley
Copy link
Member

danmoseley commented Oct 14, 2022

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

@build-analysis build-analysis bot mentioned this pull request Oct 14, 2022
Closed
BruceForstall
BruceForstall approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@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
Contributor

BruceForstall commented Oct 14, 2022

@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. :)

BruceForstall
BruceForstall approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@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
@am11 am11 mentioned this pull request Oct 20, 2022
Merged
@markples markples mentioned this pull request Nov 2, 2022
Merged
@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.

Reviewers

@AaronRobinsonMSFT AaronRobinsonMSFT Awaiting requested review from AaronRobinsonMSFT

1 more reviewer

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Infrastructure community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@am11 @AaronRobinsonMSFT @danmoseley @BruceForstall @ViktorHofer