Skip to content

Conversation

@nan01ab
Copy link
Contributor

@nan01ab nan01ab commented Jan 27, 2025

Division overflow if int.MinValue / -1 and long.MinValue / -1 .

In C#:

In a checked context, this causes a System.ArithmeticException (or a subclass thereof) to be thrown. In an unchecked context, it is implementation-defined as to whether a System.ArithmeticException (or a subclass thereof) is thrown or the overflow goes unreported with the resulting value being that of the left operand.

Reference: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#12103-division-operator

There is no division overflow check in unchecked statement.
But division overflow is checked in current .Net implementation.

Division overflow may cause the result to exceed the range of int32 or int64(in unchecked statement).
This issue will be fixed later.


if (typeSymbol.Name != "Int32" && typeSymbol.Name != "Int64")
{
// -sbyte, -byte, -short, -ushort, -char -> int, -int, -uint -> long
Copy link
Member

Choose a reason for hiding this comment

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

unsigned types works with negate?

Copy link
Contributor Author

@nan01ab nan01ab Jan 27, 2025

Choose a reason for hiding this comment

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

unsigned types works with negate?

- byte, -ushort -> int
- uint -> long
- ulong -> invalid operation

Copy link
Contributor

Choose a reason for hiding this comment

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

He is right, - ushort => int

Push(minValue);
Jump(OpCode.JMPNE_L, endTarget);

AddInstruction(OpCode.THROW);
Copy link
Contributor

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

Copy link
Contributor Author

@nan01ab nan01ab Jan 28, 2025

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs.
So no exception message here.

Copy link
Contributor

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs. So no exception message here.

LOL, that is actually a problem in other places, having exception message is the correct behavior, but its ok, no need to update. .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs. So no exception message here.

LOL, that is actually a problem in other places, having exception message is the correct behavior, but its ok, no need to update. .

This and others will be fixed by later PRs.

Copy link
Member

Choose a reason for hiding this comment

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

We should fix it in a different PR

Copy link
Contributor Author

@nan01ab nan01ab Jan 30, 2025

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs. So no exception message here.

LOL, that is actually a problem in other places, having exception message is the correct behavior, but its ok, no need to update. .

Why LOL?
Many cases were written by you. 😏

@Jim8y Jim8y merged commit 46cb9dd into neo-project:master Feb 2, 2025
4 checks passed
@Jim8y Jim8y deleted the fix.div-overflow-checking branch February 2, 2025 07:20
Jim8y added a commit that referenced this pull request Feb 5, 2025
* master:
  fix: always check division overflow for int32 and int64 (#1287)
  Fix: some compiler warnings (#1288)
  use INC DEC (#1286)
  Update neo (#1285)
  Update: keep same sdk version with 'neo' (#1284)
  partially fix return in try; optimizer replaces JMP->ENDTRY with only ENDTRY (#1283)
  update to dotnet 9 (#1257)
  Added some styles (#1280)
  [`Fix`]: `checked(-x)` if x is int or long (#1281)
  Fix continue and goto (#1282)

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Types_BigInteger.cs
Jim8y pushed a commit that referenced this pull request Aug 3, 2025
Jim8y added a commit that referenced this pull request Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants