Skip to content

Conversation

jl0pd
Copy link
Contributor

@jl0pd jl0pd commented Jul 30, 2022

According to III.3.47 stack must be empty before localloc instruction

@ghost ghost added area-Tools-ILVerification Issues related to ilverify tool and IL verification in general community-contribution Indicates that the PR has been added by a community member labels Jul 30, 2022
@dnfadmin
Copy link

dnfadmin commented Jul 30, 2022

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Jul 30, 2022

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

Issue Details

According to III.3.47 stack must be empty before localloc instruction

Author: jl0pd
Assignees: -
Labels:

area-ILVerification

Milestone: -

jl0pd and others added 2 commits July 30, 2022 20:44
{
}

.class public auto ansi beforefieldinit LocAllocTests
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a positive test as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added test for stack underflow & positive test

@jkotas
Copy link
Member

jkotas commented Jul 30, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Jul 31, 2022

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Jul 31, 2022

@trylek @jkoritzinsky The tests at https://github.com/dotnet/runtime/tree/main/src/tests/ilverify are not running as part of /azp run runtime-coreclr outerloop. The ilverify tests should be failing on this PR. It looks like a problem with the xunit test conversion. Could you please take a look? I am not able to tell how it is meant to work.

@trylek
Copy link
Member

trylek commented Jul 31, 2022

I have taken a first look. I believe the problem is not a direct consequence of test merging even though it may have been affected by some of the related script refactorings. In particular, the ILVerificationTests.csproj project that runs the various ilproj tests under ilverify/ILTests is somewhat non-standard in the sense that it resides directly under the first directory level ilverify; all other tests reside under at least two subdirectory levels and this logic is reflected in the legacy XUnit wrappers that are also generated for each of the two-level directory subtrees. The msbuild script logic around identifying the right set of second-order subdirectories in src/tests/build.proj is quite hacky (and grossly inefficient, for that matter) and it's possible / probable that previously it honored this special case of just one directory level by treating it as a second-level directory and this special treatment somehow disappeared in the multiple refactorings that have been made to the script (mostly by me, of course, I'm not trying to shift the blame to anyone else). I'll look into fixing it.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

I have verified that the tests are passing locally.

@jkotas jkotas merged commit 65af9d0 into dotnet:main Aug 1, 2022
@jkotas
Copy link
Member

jkotas commented Aug 1, 2022

Thank you!

@jl0pd
Copy link
Contributor Author

jl0pd commented Aug 1, 2022

Glad I could help!

@jl0pd jl0pd deleted the ilverify-localloc branch August 1, 2022 09:47
trylek added a commit to trylek/runtime that referenced this pull request Aug 1, 2022
As JanK noticed in dotnet#73109, outerloop tests don't run the
ilverify/ILVerificationTests.csproj test project. This is because
the project is somewhat atypical in residing just one directory
level beneath the test binary root folder; all other tests reside
under at least two directory levels and the legacy XUnit wrapper
generator reflects it so that each wrapper corresponds to
a two-level directory subtree.

The pre-existing logic for determining active two-level test
directories was quite hacky and perf-costly; I probably accidentally
removed the support for single-subfolder tests in one of my
refactorings of the src/tests/build.proj script. This change fixes
it and makes the construction of TestDirectories much more efficient
as we no longer construct a Carthesian product of all tests
and all two-level folder directories.

Thanks

Tomas
trylek added a commit that referenced this pull request Aug 2, 2022
As JanK noticed in #73109, outerloop tests don't run the
ilverify/ILVerificationTests.csproj test project. This is because
the project is somewhat atypical in residing just one directory
level beneath the test binary root folder; all other tests reside
under at least two directory levels and the legacy XUnit wrapper
generator reflects it so that each wrapper corresponds to
a two-level directory subtree.

The pre-existing logic for determining active two-level test
directories was quite hacky and perf-costly; I probably accidentally
removed the support for single-subfolder tests in one of my
refactorings of the src/tests/build.proj script. This change fixes
it and makes the construction of TestDirectories much more efficient
as we no longer construct a Carthesian product of all tests
and all two-level folder directories.

Thanks

Tomas
@ghost ghost locked as resolved and limited conversation to collaborators Aug 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Tools-ILVerification Issues related to ilverify tool and IL verification in general 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