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

ECMA is not clear about conv.ovf.i1.un behavior. #53189

Closed
sandreenko opened this issue May 24, 2021 · 8 comments · Fixed by #56450
Closed

ECMA is not clear about conv.ovf.i1.un behavior. #53189

sandreenko opened this issue May 24, 2021 · 8 comments · Fixed by #56450
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI documentation Documentation bug or enhancement, does not impact product or test code
Milestone

Comments

@sandreenko
Copy link
Contributor

Digging into #52828 I think there are more issues to fix, according to ECMA-335 such IL

      ldc.r4       -1.0
      conv.ovf.i1.un

should throw,
when

      ldc.r4       -1.0
      conv.ovf.i1

should not.

Nowadays they both don't (with or without the changes from #52828 )

@sandreenko sandreenko added bug area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels May 24, 2021
@sandreenko sandreenko added this to the 6.0.0 milestone May 24, 2021
@sandreenko sandreenko self-assigned this May 24, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 24, 2021
@SingleAccretion
Copy link
Contributor

Digging into #52828 I think there are more issues to fix, according to ECMA-335 such IL

  ldc.r4       -1.0
  conv.ovf.i1.un

should throw

Hmm, I am not seeing it 🤔. Could you point me to the part of the specification which implies this?

@sandreenko
Copy link
Contributor Author

sandreenko commented May 25, 2021

Hmm, I am not seeing it 🤔. Could you point me to the part of the specification which implies this?

I could be wrong, cc @echesakovMSFT , @AndyAyersMS, and @dotnet/jit-contrib to help me here.

we start with:

image
and it says:
image

so I understand it as:
ldc.r4 -1.0 -> push -1.0F on the stack
conv.ovf.i1.un -> pop value from the stack and treat as unsigned int, meaning -1.0F is truncated to -1 and then treated as MAX_UNSIGNED (0xFFFFFFFF), then we try to cast MAX_UNSIGNED to i1 but it does not fit -> overflow exception.

@sandreenko
Copy link
Contributor Author

Looks like

goto CONV_UN;

and
case CEE_CONV_U8:

should be goto CONV;
but changing this causes Process terminated. Infinite recursion during resource lookup within System.Private.CoreLib., interesting.

the dumps for the method that causes it.
badmethods.dump.txt
goodmethods.dump.txt

I don't think I will have time to work on it this week, feel free to grab it if you are interested and like to read ecma.

@sandreenko sandreenko removed their assignment May 26, 2021
@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Jun 3, 2021
@AndyAyersMS
Copy link
Member

@sandreenko presumably this is not a regression, so not something we need to address urgently?

@sandreenko
Copy link
Contributor Author

@sandreenko presumably this is not a regression, so not something we need to address urgently?

I think we need to address it in 6.0 because the behavior has changed since 5.0, so now there are 3 behaviours:

  1. what we had in 5.0;
  2. what we have now;
  3. what ECMA says we should have;

and if somebody hits an issue with 2. that did not repro with 1. it would be hard to justify why it is expected.

@SingleAccretion
Copy link
Contributor

Some points to consider:

1. As far as I am aware this opcode is never produced by Roslyn for a floating-point operand.
2. Behavior (per the specification, not implementation) of other instructions suffixed with .un:

  • Relops, shr.un: not relevant for our purposes.
  • add.ovf.un, sub.ovf.un, mul.ovf.un - don't support* floating point operands (see Table III.7: Overflow Arithmetic Operations).
  • rem.un, div.un: don't support* floating point operands (see floating point operands).
  • conv.r.un: The conv.r.un operation takes an integer off the stack, interprets it as unsigned, and replaces it with an F type floating-point number to represent the integer.

* "don't support" - operands required to be of correct type for Correctness.

I believe the most relevant bit of text is in III.3.29, unsigned data conversion with overflow detection, which is the only place I've been able to find that specifies what should the .un conversions do when the type on the stack is a floating-point one:

Convert the value on top of the stack to the type specified in the opcode, and leave that converted 
value on the top of the stack. If the value cannot be represented, an exception is thrown. The item 
on the top of the stack is treated as an unsigned value before the conversion. 
Conversions from floating-point numbers to integral values truncate the number toward zero. 
Note that integer values of less than 4 bytes are extended to int32 (not native int) on the 
evaluation stack.

Of course, Table III.8: Conversion Operations is also very relevant.

Below is my reconstruction based on it and the above rules for the sequence in question.

  1. ldc.r4 -1 - type on the stack is F.
  2. conv.ovf.i1.un
    • Type on the stack is F, target type is int8 => instruction is legal, action is Truncate to zero.
    • "Treat the item in top of the stack as unsigned" - I have not been able to find what should this mean for Fs (that's the real question here I suppose). However, Conversions from floating-point numbers to integral values truncate the number toward zero seems to indicate that this is a no-op for Fs.
    • Truncation to zero occurs, now the value is of type int32 and is equal to -1.
    • Bounds check occurs and passes, the final value is -1.

I suppose that'd be my argument for why the current behavior is correct. However, it seems to me that the specification is quite terse on this, if not incomplete. It would be awesome to have it be specified what "treating as unsigned" means for Fs. There is text here and there (e. g. in III.1.1.1 Numeric data types, Unsigned integers) that strongly implies "being treated as unsigned" is only valid for integers, but it is not clear what implications does this have for the conversion operations.

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Jun 8, 2021

Curiously enough, and as another data point, Mono interpreter considers conv.ovf.i1.un to be:

  1. Pop the floating point value.
  2. if (val < 0 || val > MAXINT8 || isnan(val)) throw.
  3. Cast to int8 and push on the stack.

So, there the semantic for "treat as unsigned" seems to be "consider < 0 invalid" (added in mono/mono#18813).

@sandreenko
Copy link
Contributor Author

Thanks for the great analysis, @SingleAccretion. The idea that maybe we should edit the spec seems attractive because it is for sure can state the contract clearer for such case.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 28, 2021
@sandreenko sandreenko added documentation Documentation bug or enhancement, does not impact product or test code and removed bug labels Aug 2, 2021
@sandreenko sandreenko changed the title Another bug with unsigned/signed cast ECMA is not clear about conv.ovf.i1.un behaviour. Aug 2, 2021
@sandreenko sandreenko changed the title ECMA is not clear about conv.ovf.i1.un behaviour. ECMA is not clear about conv.ovf.i1.un behavior. Aug 2, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants