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

const array indexing zeroes data in object variants at runtime #8015

Closed
mratsim opened this issue Jun 11, 2018 · 9 comments · Fixed by #16782
Closed

const array indexing zeroes data in object variants at runtime #8015

mratsim opened this issue Jun 11, 2018 · 9 comments · Fixed by #16782
Labels

Comments

@mratsim
Copy link
Collaborator

mratsim commented Jun 11, 2018

Follow-up on #8007.

As a workaround to Tables losing compile-time object variant fields at runtime, I used arrays.

However after assigning a const array of object variant to a non-const variable, fields containing a proc pointer become nil.

Test case:

type
  CostKind = enum
    Fixed,
    Dynamic

  Cost = object
    case kind*: CostKind
    of Fixed:
      cost*: int
    of Dynamic:
      handler*: proc(): int {.nimcall.}

proc foo1(): int {.nimcall.} =
  100

proc foo2(): int {.nimcall.} =
  200

# Change to `let` and it doesn't crash
const costTable = [
  0: Cost(kind: Fixed, cost: 999),
  1: Cost(kind: Dynamic, handler: foo1),
  2: Cost(kind: Dynamic, handler: foo2)
]

##########################

echo cast[ByteAddress](costTable[1].handler) # A random address for example - 4497725824
echo cast[ByteAddress](costTable[2].handler) # 4497726128

echo costTable[0]           # (kind: Fixed, cost: 999)
echo costTable[1].handler() # 100
echo costTable[2].handler() # 200

##########################
# Now trying to carry the table as an object field

type
  Wrapper = object
    table: array[3, Cost]

proc procNewWrapper(): Wrapper =
  result.table = costTable


# Alternatively, change to `const` and it doesn't crash
let viaProc = procNewWrapper()

echo cast[ByteAddress](viaProc.table[1].handler) # Address is 0
echo cast[ByteAddress](viaProc.table[2].handler)

echo viaProc.table[0]           # (kind: Fixed, cost: 999)
echo viaProc.table[1].handler() # SIGSEGV
echo viaProc.table[2].handler()

No issue if:

  • everything is done at compile-time
  • everything is done at runtime
  • we don't use object variant

Example without object variant

type
  Cost = proc(): int {.nimcall.}

proc foo1(): int {.nimcall.} =
  100

proc foo2(): int {.nimcall.} =
  200

# Change to `let` and it doesn't crash
const costTable = [
  0: foo1,
  1: foo1,
  2: foo2
]

##########################

echo cast[ByteAddress](costTable[1]()) # A random address for example - 4497725824
echo cast[ByteAddress](costTable[2]()) # 4497726128

echo costTable[1]() # 100
echo costTable[2]() # 200

##########################
# Now trying to carry the table as an object field

type
  Wrapper = ref object of RootObj # changing to object doesn't change anything here as well
    table: array[3, Cost]

proc procNewWrapper(): Wrapper =
  new result
  result.table = costTable

let viaProc = procNewWrapper()

echo cast[ByteAddress](viaProc.table[1]) # Address is 0
echo cast[ByteAddress](viaProc.table[2])

echo viaProc.table[1]() # SIGSEGV
echo viaProc.table[2]()
mratsim added a commit to status-im/nimbus-eth1 that referenced this issue Jun 11, 2018
mratsim added a commit to status-im/nimbus-eth1 that referenced this issue Jun 12, 2018
* Decoupling op logic and gas - introduce gasometer, rework opcode declaration

* Remove gas constants for gas opcode computation

* Remove gas constants for precompiled contracts

* make vm_types compile

* Make opcode, call and computation compile

* Distinguish between dynamic and complex gas costs, fix arithmetic

* Fix context and sha3

* update memory and storage ops

* Log opcode uses memory expansion code

* update/stub system_ops with gas costs

* Make test compile. Deactivate stub test_vm

* all tests compiles, opcode fails due to nim-lang/Nim#8007 (const object variant in tables reset at runtime)

* Create an enum without holes - workaround: nim-lang/Nim#8007

* Use arrays instead of tables for GasCosts, remove some unused imports - passing all basic tests!

* Make test_vm_json compile

* Fix test_vm_json - workaround nim-lang/Nim#8015

* fix memory expansion cost bug

* Remove leftover special handling from before GckMemExpansion

* cleanup outdated comment, better align =

* Fix sha3 gas cost not taking memory expansion into account

* Improve gas error reporting of test_vm_json

* Fix gas computation regression due to mem expansion

* mass replace for memExpansion->RequestedMemSize was too eager

* fix log gas cost (no tests :/)

* missed a static FeeSchedule

* static as expression is fickle
@dom96 dom96 added VM see also `const` label Object Variants labels Jun 12, 2018
@jangko
Copy link
Contributor

jangko commented Aug 3, 2018

this is the generated code for costTable

NIM_CONST tyArray_WhU07q1GSgwZ2Yc60JIUlg costTable_sBUTwJc6CNeo5483rNsQhg = {{((tyEnum_CostKind_GGuYJIMY9aVFBKVS7j4CxtA) 0), ((NI) 999), NIM_NIL},
{((tyEnum_CostKind_GGuYJIMY9aVFBKVS7j4CxtA) 1), ((NI) 0), foo1_X2aQK4bCESQjARXX8182jQ},
{((tyEnum_CostKind_GGuYJIMY9aVFBKVS7j4CxtA) 1), ((NI) 0), foo2_X2aQK4bCESQjARXX8182jQ_2}}
;

((NI) 999), NIM_NIL} wrong!
((NI) 0), foo1_X2aQK4bCESQjARXX8182jQ} wrong!
((NI) 0), foo2_X2aQK4bCESQjARXX8182jQ_2} wrong!

why this code below not crash? because Nim compiler use literal value here, not taken from costTable

echo costTable[0]           # (kind: Fixed, cost: 999)
echo costTable[1].handler() # 100
echo costTable[2].handler() # 200

although the generated C code is wrong, the bug could be located somewhere else, not necessarily in codegen. But it is a starting point if someone decide to take a look.

@mratsim mratsim changed the title const array losing proc field data in object variants at runtime const array losing field data in object variants at runtime Apr 8, 2019
@mratsim
Copy link
Collaborator Author

mratsim commented Apr 8, 2019

As reported in #9021 and https://forum.nim-lang.org/t/4773 for enums, all kind of fields lose data not only proc fields.

@Araq
Copy link
Member

Araq commented May 10, 2019

Const case objects never were supposed to compile, there is logic in the compiler to detect and reject that but as we can see it's not triggered.

@narimiran
Copy link
Member

Current outputs (Nim devel 1.1.1) for the two examples in the original post:

94137662452323
94137662452460
(kind: Fixed, cost: 0)
100
200
94137662452323
94137662452460
(kind: Fixed, cost: 999)
100
200
100
200
100
200
94504683509470
94504683509607
100
200

(Notice cost: 0!)

@timotheecour
Copy link
Member

seems related: #13081

@Clyybber Clyybber closed this as completed Jun 8, 2020
@Clyybber Clyybber reopened this Jun 8, 2020
@mratsim
Copy link
Collaborator Author

mratsim commented Jun 10, 2020

Just so people know, in many cases you can do const foo = static(yourProcOrCompileTimeVal) as a workaround to cross the compile-time/runtime boundary

@timotheecour
Copy link
Member

even though other object variant bugs were recently fixed, this bug remains

minimal repro:

when true:
  import unittest
  type Foo = object
    case b: bool
    of false: v1: int
    of true: v2: int
  const t = [Foo(b: false, v1: 1), Foo(b: true, v2: 2)]
  check $t == "[(b: false, v1: 1), (b: true, v2: 2)]"
  check $t[0] == "(b: false, v1: 1)" # fails

fails as of 64016dd 1.5.1

echo costTable[0] # (kind: Fixed, cost: 999)

@mratsim I tried a few versions and never saw that, only (kind: Fixed, cost: 0)
(I'm on OSX)

Which git hash did you try it on? (this is why we need to show git hash in issue reports, always);

echo viaProc.table[1].handler() # SIGSEGV

doesn't crash anymore

@timotheecour timotheecour changed the title const array losing field data in object variants at runtime const array indexing zeroes data in object variants at runtime Oct 23, 2020
@narimiran
Copy link
Member

Both original examples and the example in the @timotheecour's comment above mine seem to work fine on the latest devel. Can you confirm?

@timotheecour
Copy link
Member

Can you confirm?

indeed => #16782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants