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 VARIANT code and add tests #3197

Merged
merged 1 commit into from Jul 7, 2020
Merged

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented May 3, 2020

Proposed Changes

  • Fix porting bug: VARENUM vtType = vt & ~VARENUM.TYPEMASK; check should have been VARENUM vtType = vt & VARENUM.TYPEMASK;
  • Structify VARIANT by moving finalizer into disposer
  • Fix overflow exceptions in conversion to unsigned numbers
  • Implement conversion to float/double
Microsoft Reviewers: Open in CodeFlow

@hughbe hughbe requested a review from a team as a code owner May 3, 2020 19:02
@ghost ghost assigned hughbe May 3, 2020
@codecov
Copy link

codecov bot commented May 3, 2020

Codecov Report

Merging #3197 into master will decrease coverage by 28.72576%.
The diff coverage is 0.00000%.

@@                 Coverage Diff                  @@
##              master       #3197          +/-   ##
====================================================
- Coverage   61.67065%   32.94489%   -28.72576%     
====================================================
  Files           1287         865         -422     
  Lines         451933      254088      -197845     
  Branches       39511       36748        -2763     
====================================================
- Hits          278710       83709      -195001     
+ Misses        167822      165544        -2278     
+ Partials        5401        4835         -566     
Flag Coverage Δ
#Debug 32.94489% <0.00000%> (-28.72577%) ⬇️
#production 32.94489% <0.00000%> (+0.01955%) ⬆️
#test ?

Copy link
Contributor

@weltkante weltkante left a comment

Choose a reason for hiding this comment

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

This code looks broken, do we want to fix it or should I open an issue?

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label May 4, 2020
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 4, 2020
@hughbe
Copy link
Contributor Author

hughbe commented May 4, 2020

Woohoo - so much to go through wow!

So I've updated the PR. Unfortunately had to rebase because the changes didn't make much sense due to code movement.

Basically the changes I made were

  • Implement Clear as VariantClear. I had to guard this with an if condition, as it looks like VT_CLSID | VT_BYREF caused the native code VariantClear to try to delete a managed pointer, which caused all sorts of problems. Looking at the implementation in IDA, I found
    image
    I suspect this is a sideffect or some sort of oversight in the previous if check in the native code?

  • Ban VT_VARIANT on its own and implement VT_VARIANT | VT_BYREF in terms of VariantCopy

  • Ban VT_CLSID on its own

  • Fix the implementation of VT_DECIMAL and VT_DECIMAL | VT_BYREF by essentially pointer casting &this to DECIMAL* as we do in the native code. I tested this by calling PInvokes such as VarDecFromI8 and VarDecFromR8.

  • Ban VT_ARRAY and VT_VECTOR as type modifiers. I may come back and implement this at some point but keeping it for now as its unused in its current use case.

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label May 7, 2020
@RussKie RussKie requested a review from JeremyKuhne May 7, 2020 04:48
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 7, 2020
@hughbe hughbe force-pushed the VARIANT_fixes branch 2 times, most recently from e95e7ed to 23f9461 Compare May 7, 2020 09:17
@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #3197 into master will decrease coverage by 28.89385%.
The diff coverage is 0.00000%.

@@                 Coverage Diff                  @@
##              master       #3197          +/-   ##
====================================================
- Coverage   61.68011%   32.78626%   -28.89385%     
====================================================
  Files           1288         866         -422     
  Lines         451935      254079      -197856     
  Branches       39511       36750        -2761     
====================================================
- Hits          278754       83303      -195451     
+ Misses        167786      165980        -1806     
+ Partials        5395        4796         -599     
Flag Coverage Δ
#Debug 32.78626% <0.00000%> (-28.89386%) ⬇️
#production 32.78626% <0.00000%> (-0.15614%) ⬇️
#test ?

@dotnet dotnet deleted a comment from codecov bot May 8, 2020
@RussKie RussKie dismissed their stale review May 8, 2020 03:40

Looks good to me 👍

@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label May 8, 2020
@hughbe
Copy link
Contributor Author

hughbe commented May 8, 2020

Let me know if there’s anything to action :)

@RussKie
Copy link
Member

RussKie commented May 12, 2020

Waiting for @JeremyKuhne's approval.
@weltkante is there anything outstanding from your review?

weltkante
weltkante previously approved these changes May 12, 2020
@weltkante
Copy link
Contributor

weltkante commented May 12, 2020

is there anything outstanding from your review?

all important points are covered

Its still unclear what an "OLE property set" is and whether the code should support it or not (currently it does support some but not all cases, which makes little sense) - but that shouldn't hold up the PR as it was the same before. My thoughts are OLE property sets are that its something you shouldn't be supporting here and the corresponding cases could be removed, but I don't oppose leaving them as they are if there is no documentation to be found.

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks for trying to get this into better shape! I've left comments for things I'd like to see changed/addressed.

Taking this farther we need to better handle / differentiate when we're working with PROPVARIANT, VARIANT, or TYPEDESC. The headers have a decent table that shows what is used in which context. We want to make sure we're cleaning / copying appropriately based on what we have for content.

It would be good to give significant thought to how we might help prevent inadvertent leaking of native memory. I'd also like to spend time building confidence that we don't have situations where data hangs off the end of the VARIANT.

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label May 12, 2020
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label May 13, 2020
@RussKie
Copy link
Member

RussKie commented Jun 18, 2020

Overall looks good to me (at the level I understand this area).

@JeremyKuhne please review again when you have time.

@hughbe hughbe force-pushed the VARIANT_fixes branch 7 times, most recently from f702d0e to ef7409d Compare June 19, 2020 11:13
@RussKie
Copy link
Member

RussKie commented Jun 25, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JeremyKuhne JeremyKuhne merged commit fd49490 into dotnet:master Jul 7, 2020
@ghost ghost added this to the 5.0 Preview8 milestone Jul 7, 2020
@hughbe hughbe deleted the VARIANT_fixes branch July 8, 2020 12:34
monojenkins pushed a commit to monojenkins/mono that referenced this pull request Jul 9, 2020
In dotnet/winforms#3197 we were investigating what happens when you try to create an an array from SAFEARRAY with more than 32 dimensions.

Found out that this throws `TypeLoadException`.

Thought I'd come along and write some tests to validate this constraint in dotnet/runtime.

Note: in In dotnet/winforms#3197 we wanted to `stackalloc` an `int` array to navigate through an n-dimensional array instead of manually allocating `int[]` which costs memory. Obviously we could only do this if `Array.Rank` is not something large such that `stackalloc` would cause `StackOverflowExceptions`. Is it safe to assume that we will not change `MAX_RANK`?

Updated the comment in https://github.com/dotnet/runtime/blob/80d8920a54b0d73eb8423fe705583a0fd7ad1723/src/coreclr/src/vm/clsload.cpp#L3327-L3331

I think we do need this check!

Note that I think mono will hit some assertions and (possibly?) crash: https://github.com/dotnet/runtime/blob/1cfccd2a7937117341b256847e613dd710a70adb/src/mono/mono/metadata/class-init.c#L920

/cc @akoeplinger for mono if it is relevant, @weltkante @JeremyKuhne @AaronRobinsonMSFT
monojenkins pushed a commit to monojenkins/mono that referenced this pull request Jul 9, 2020
In dotnet/winforms#3197 we were investigating what happens when you try to create an an array from SAFEARRAY with more than 32 dimensions.

Found out that this throws `TypeLoadException`.

Thought I'd come along and write some tests to validate this constraint in dotnet/runtime.

Note: in In dotnet/winforms#3197 we wanted to `stackalloc` an `int` array to navigate through an n-dimensional array instead of manually allocating `int[]` which costs memory. Obviously we could only do this if `Array.Rank` is not something large such that `stackalloc` would cause `StackOverflowExceptions`. Is it safe to assume that we will not change `MAX_RANK`?

Updated the comment in https://github.com/dotnet/runtime/blob/80d8920a54b0d73eb8423fe705583a0fd7ad1723/src/coreclr/src/vm/clsload.cpp#L3327-L3331

I think we do need this check!

Note that I think mono will hit some assertions and (possibly?) crash: https://github.com/dotnet/runtime/blob/1cfccd2a7937117341b256847e613dd710a70adb/src/mono/mono/metadata/class-init.c#L920

/cc @akoeplinger for mono if it is relevant, @weltkante @JeremyKuhne @AaronRobinsonMSFT
monojenkins pushed a commit to monojenkins/mono that referenced this pull request Jul 9, 2020
In dotnet/winforms#3197 we were investigating what happens when you try to create an an array from SAFEARRAY with more than 32 dimensions.

Found out that this throws `TypeLoadException`.

Thought I'd come along and write some tests to validate this constraint in dotnet/runtime.

Note: in In dotnet/winforms#3197 we wanted to `stackalloc` an `int` array to navigate through an n-dimensional array instead of manually allocating `int[]` which costs memory. Obviously we could only do this if `Array.Rank` is not something large such that `stackalloc` would cause `StackOverflowExceptions`. Is it safe to assume that we will not change `MAX_RANK`?

Updated the comment in https://github.com/dotnet/runtime/blob/80d8920a54b0d73eb8423fe705583a0fd7ad1723/src/coreclr/src/vm/clsload.cpp#L3327-L3331

I think we do need this check!

Note that I think mono will hit some assertions and (possibly?) crash: https://github.com/dotnet/runtime/blob/1cfccd2a7937117341b256847e613dd710a70adb/src/mono/mono/metadata/class-init.c#L920

/cc @akoeplinger for mono if it is relevant, @weltkante @JeremyKuhne @AaronRobinsonMSFT
monojenkins pushed a commit to monojenkins/mono that referenced this pull request Jul 13, 2020
In dotnet/winforms#3197 we were investigating what happens when you try to create an an array from SAFEARRAY with more than 32 dimensions.

Found out that this throws `TypeLoadException`.

Thought I'd come along and write some tests to validate this constraint in dotnet/runtime.

Note: in In dotnet/winforms#3197 we wanted to `stackalloc` an `int` array to navigate through an n-dimensional array instead of manually allocating `int[]` which costs memory. Obviously we could only do this if `Array.Rank` is not something large such that `stackalloc` would cause `StackOverflowExceptions`. Is it safe to assume that we will not change `MAX_RANK`?

Updated the comment in https://github.com/dotnet/runtime/blob/80d8920a54b0d73eb8423fe705583a0fd7ad1723/src/coreclr/src/vm/clsload.cpp#L3327-L3331

I think we do need this check!

Note that I think mono will hit some assertions and (possibly?) crash: https://github.com/dotnet/runtime/blob/1cfccd2a7937117341b256847e613dd710a70adb/src/mono/mono/metadata/class-init.c#L920

/cc @akoeplinger for mono if it is relevant, @weltkante @JeremyKuhne @AaronRobinsonMSFT
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Several tests aren't locale agnostic Relates to #3121

@ghost ghost locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants