Nim 2.0 requires explicit gcsafe annotations when using `without` errors

[elcritch]

Nim 2.0 is out! Naturally I tried compiling Codex with it. Questionable itself works fine, but it causes a bunch of gcsafe issues in various codex methods.

It seems due to the global error variable used in the error binding templates. Nim 2.0 appears to be detecting the usage of a global and requires annotating methods which use without clauses with gcsafe.

Rather than annotating methods with gcsafe we probably want to annotate Questionable generate code to be marked gcsafe. Ideally we could figure out a way to remove the global errorVariable piece. Though that seems tricky given the way the error variables are bound.

[markspanbroek]

Can you help me reproduce this? I tried compiling Codex with Nim 2.0.0 (NIM_COMMIT=v2.0.0 make), and after adding a a bunch of {.gcsafe.}, {.effectsOf.} and removing shallowCopy in several of Codex's dependencies, it compiled. I didn't need to touch questionable.

(Here's a list of all the changes: https://gist.github.com/markspanbroek/f0f7c6c29b42188f1d50807890530ba5)

[elcritch]

The extra required {.gcsafe.} annotations seem to be mainly due to the questionable's use of a global. There might be others causes too, but I did some testing with commenting out without in a few of those places and it fixed the gcsafe issue.

Really we shouldn't need to annotate proc's that use without with {.gcsafe.}. Though yah, we could just go that route it doesn't seem proper.

[markspanbroek]

Really we shouldn't need to annotate proc's that use without with {.gcsafe.}

I agree, I just couldn't find a place where I had to do that :smile:

I added {.gcsafe.} in a bunch of places, but without wasn't used in any of them.

[elcritch]

Ah, gotcha! Yah I recall it taking a couple of changes due to it occurring in a method. I'll have to try and replicate it tomorrow to find an example.

[markspanbroek]

Hmm, been reading through the manual, and it seems to suggest that our use of the global should be ok:

"We call a proc p GC safe when it doesn't access any global variable that contains GC'ed memory (string, seq, ref or a closure) either directly or indirectly through a call to a GC unsafe proc."

We're storing a pointer in the global errorVariable, so accessing it should be GC safe.

[elcritch]

"We call a proc p GC safe when it doesn't access any global variable that contains GC'ed memory (string, seq, ref or a closure) either directly or indirectly through a call to a GC unsafe proc."

Hmmm, yah that's a good point. I mean it's sorta side-stepping the GC there by taking a pointer.

I was trying to reproduce my original tests earlier and haven't had success yet. There's cases where commenting out a block of code means a method isn't called anymore and so it doesn't require the gcsafe annotation. Basically it's just not compiled. I'm starting to wonder if that may have been what I was seeing and it happened to be a without block. :/

Also, I did find that the gcsafe requirements could be satisfied by making the base method async whenever one of it's child ones are async. As in:

type
  BlockStore = ref object of RootObj
  CacheStore = ref object of BlockStore

method hasBlock*(self: BlockStore, cid: Cid): Future[?!bool] {.base.} = # <--------
  raiseAssert("Not implemented!")

method hasBlock*(self: CacheStore, cid: Cid): Future[?!bool] {.async.} =
  discard "some impl"

The hasBlock for BlockStore requires gcsafe but seems to be due mixed async status. That's a guess but doing:

method hasBlock*(self: BlockStore, cid: Cid): Future[?!bool] {.async, base.} = ...

Appears to skip the need for gcsafe.

[elcritch]

I'm also getting this, but only in nimsuggest.

lol, now I'm sorta baffled.

[dryajov]

Hmm, been reading through the manual, and it seems to suggest that our use of the global should be ok:

"We call a proc p GC safe when it doesn't access any global variable that contains GC'ed memory (string, seq, ref or a closure) either directly or indirectly through a call to a GC unsafe proc."

We're storing a pointer in the global errorVariable, so accessing it should be GC safe.

Does the pointer points to GCed memory? If thats the case, then the compiler is probably right to complain.

But I do remember that there were lots of false positives with that, so gcsafe was used almost by default on most procs, that should have changed in latest release, but maybe some of it still lingers around. I wouldn't discard the compiler being an issue here as well.

Perhaps, @zah can provide some more background around this?

[dryajov]

Also, I did find that the gcsafe requirements could be satisfied by making the base method async whenever one of it's child ones are async. As in:

The async annotation will expand the method into the async "state machine" that relies on a closure iterator, the signature of the method/proc should not change, but it's definitely rewriting the proc underneath and changing its semantics.

This doesn't happen with the standalone Feature[...] return type. So the fact that you are getting slightly different behavior with the async annotation is not surprising.

However, since the method doesn't contain any await calls, the async annotation should not be needed.

[elcritch]

I'm going to close this issue as it appears unrelated to the problems I was seeing. Thanks for checking it out though @markspanbroek and @dryajov.