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

Deepcopy in arc crashes #15405

Closed
cooldome opened this issue Sep 24, 2020 · 12 comments
Closed

Deepcopy in arc crashes #15405

cooldome opened this issue Sep 24, 2020 · 12 comments

Comments

@cooldome
Copy link
Member

cooldome commented Sep 24, 2020

Deepcopy recently appeared for arc runtime, first test results.
Test case:

import lexbase, streams
const test = "<A><B>value</B></A>"
var stream = newStringStream(test)
var lex: BaseLexer
open(lex, stream)
var lex2 = deepCopy(lex)

Crashes on the nil pointer dereference with stacktrace:

C:\Nim\t8.nim(24)        t8
C:\Nim\lib\system.nim(2881) deepCopy
C:\Nim\lib\system\deepcopy.nim(192) genericDeepCopy
C:\Nim\lib\system\deepcopy.nim(130) genericDeepCopyAux
C:\Nim\lib\system\deepcopy.nim(72) genericDeepCopyAux
C:\Nim\lib\system\deepcopy.nim(68) genericDeepCopyAux
C:\Nim\lib\system\deepcopy.nim(168) genericDeepCopyAux

I have done a bit of investigation.: .typeInfoV1 member is nil for the NTIv2 struct of the StringStreamObj object.

typeInfoV1 is not nil for StreamObj which is inheritance base for StringStreamObj. AFAIK, genTypeInfoV1 codegen is done only for type BaseLexer that includes reference to StreamObj. Because StringStreamObj is not directly referenced, genTypeInfoV1 is not executed for it.

@cooldome
Copy link
Member Author

I don't see how you can get all child objects of the given base object to fix it.

@Araq
Copy link
Member

Araq commented Sep 24, 2020

The compiler has no such data-structure that would allow for that. :-)

@cooldome
Copy link
Member Author

Any idea how it can be fixed? running genTypeInfoV1 for all inheritable objects?

@Araq
Copy link
Member

Araq commented Sep 24, 2020

Seems like genTypeInfoV2 is missing a simple loop over the parent objects.

@cooldome
Copy link
Member Author

cooldome commented Sep 24, 2020

IMO, in arc mode genTypeInfoV1 is executed only from genDeepCopy. At least this is what I see.
StringStreamObj is not reachable from genDeepCopy.

Unless we start calling genTypeInfoV1 from genTypeInfoV2 I don't see how to reach StringStreamObj.
Are you ok with such fix?

cooldome added a commit that referenced this issue Sep 24, 2020
@Araq
Copy link
Member

Araq commented Sep 25, 2020

That means we always get the V1 type info bloat. Thinking about a better way...

@Araq
Copy link
Member

Araq commented Sep 25, 2020

There is a good workaround which IMO we should officially document:

template enableDeepCopy(x) =
  discard getTypeInfo(x)

import parsexml, streams
const test_xml_str = "<A><B>value</B></A>"
var stream = newStringStream(test_xml_str)
enableDeepCopy(stream)
var xml: XmlParser
open(xml, stream, "test")
var xml2 = deepCopy(xml)

@cooldome
Copy link
Member Author

cooldome commented Sep 25, 2020

Workaround doesn't work if you don't have accesss to internals of the objects. Imagine you have only XmlParser and its content is opaque and contains non exported refs to objects.
This won't work:

var xml: XmlParser
enableDeepCopy(xml)

You need to do:

enableDeepCopy(xml.f1)  # but f1 is not exported and its type is not exported either
enableDeepCopy(xml.f2)  # but f2 is not exported and its type is not exported either

Also xnk f1 can contain ref to base type not the actual type you want to be enabled.

@Araq
Copy link
Member

Araq commented Sep 25, 2020

The question is: Does the workaround work for you?

@cooldome
Copy link
Member Author

It helps me to progress in arc adoption. It helps with the deecopies in the parsers, there is one more deepcopy that will be hard to workaround like that but my code crashes before this step, investigating.

@SFR0815
Copy link

SFR0815 commented Oct 1, 2020

I have come across an issue as follows:

  1. element.nim
type
  Element* = ref object of RootObj
    name: string
    getString_Impl: proc(element: Element): string
    setString_Impl: proc(element: Element, value: string)

proc setElementDefinitions*(
            element: Element,
            getString_Impl: proc(element: Element): string,
            setString_Impl: proc(element: Element, value: string)) =
  element.getString_Impl = getString_Impl
  element.setString_Impl = setString_Impl

proc getString*(element: Element): string =
  return element.getString_Impl(element)
proc `value =`*(element: Element, value: string) =
  element.setString_Impl(element, value)

proc `name`*(element: Element): string =
  return element.name
proc `name =`*(element: Element, name: string) =
  element.name = name
  1. spec_element.nim:
ype
  Element* = ref object of RootObj
    name: string
    getString_Impl: proc(element: Element): string
    setString_Impl: proc(element: Element, value: string)

proc setElementDefinitions*(
            element: Element,
            getString_Impl: proc(element: Element): string,
            setString_Impl: proc(element: Element, value: string)) =
  element.getString_Impl = getString_Impl
  element.setString_Impl = setString_Impl

proc getString*(element: Element): string =
  return element.getString_Impl(element)
proc `value =`*(element: Element, value: string) =
  element.setString_Impl(element, value)

proc `name`*(element: Element): string =
  return element.name
proc `name =`*(element: Element, name: string) =
  element.name = name
  1. main.nim:
import ./spec_element

let element_1: Element = newStringElement("element_1")

let element_2 = element_1.deepCopy()

echo element_2.name

Works without --gc:arc and crashes with.

If you put all together into one module is works with AND without --gc:arc:

type
  Element = ref object of RootObj
    name: string
    getString_Impl: proc(element: Element): string
    setString_Impl: proc(element: Element, value: string)
  StringElement = ref object of Element
    value: string

proc setElementDefinitions(
            element: Element,
            getString_Impl: proc(element: Element): string,
            setString_Impl: proc(element: Element, value: string)) =
  element.getString_Impl = getString_Impl
  element.setString_Impl = setString_Impl

proc getString(element: Element): string =
  return element.getString_Impl(element)
proc `value =`*(element: Element, value: string) =
  element.setString_Impl(element, value)

proc `name`(element: Element): string =
  return element.name
proc `name =`(element: Element, name: string) =
  element.name = name

proc getString_Impl(element: Element): string =
  return element.StringElement().value
proc setString_Impl(element: Element, value: string) =
  element.StringElement().value = value

proc newStringElement(name: string = ""): StringElement =
  result.new()
  result.setElementDefinitions(
              getString_Impl = getString_Impl,
              setString_Impl = setString_Impl)
  `name =`(result, name)

let element_1 = newStringElement("element_1")

let element_2 = element_1.deepCopy()

echo element_2.name

@SFR0815
Copy link

SFR0815 commented Oct 1, 2020

Sorry,

messed up #2 above. Should be as follows:

  1. spec_element.nim:
import ./element

export element

type
  StringElement* = ref object of Element
    value: string

proc getString_Impl(element: Element): string =
  return element.StringElement().value
proc setString_Impl(element: Element, value: string) =
  element.StringElement().value = value

proc newStringElement*(name: string = ""): StringElement =
  result.new()
  result.setElementDefinitions(
              getString_Impl = getString_Impl,
              setString_Impl = setString_Impl)
  `name =`(result, name)

@Araq Araq closed this as completed in 531ed2d Oct 1, 2020
mildred pushed a commit to mildred/Nim that referenced this issue Jan 11, 2021
* fix nim-lang#15405
* fix tests
* deepcopy for ARC has to be enabled via --deepcopy:on

Co-authored-by: Araq <rumpf_a@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants