sizeof for static arrays can yield wrong results
#22639
Comments
|
A minimal assertive repro sample would be nice to try Bisect. |
Potentially the example crashes the compiler anyway (depending on how the compilers used for bisecting are built). But here's one with assertions: import macros
type
Spectrum*[N: static int] = object
data*: array[N, float]
AngleInterpolator* = object
data*: seq[Spectrum[60]]
proc initInterpolator*(num: int): AngleInterpolator =
result = AngleInterpolator()
for i in 0 ..< num:
result.data.add Spectrum[60]()
macro genCompatibleTuple*(t: typed): untyped =
let typ = t.getType[1].getTypeImpl[2]
result = nnkTupleTy.newTree()
for i, ch in typ: # is `nnkObjectTy`
result.add nnkIdentDefs.newTree(ident(ch[0].strVal), # ch is `nnkIdentDefs`
ch[1],
newEmptyNode())
proc copyFlat[T: object | tuple](x: T) =
var tmp: genCompatibleTuple(T)
echo "TMP type: ", typeof(tmp), " of size? ", sizeof(tmp) # <- wrong, should be 480 instead of 8
doAssert sizeof(tmp) == 480
for field, val in fieldPairs(x):
# the field individually is reported correctly, but the full size is not
doAssert sizeof(val) == 480
echo field, " of field size: ", sizeof(val), " full size of type? ", sizeof(tmp)
let reflectivity = initInterpolator(1)
for el in reflectivity.data:
copyFlat(el)
doAssert sizeof(reflectivity.data[0]) == 480 # <- this is correct!
|
|
Minimal: import macros
type
Spectrum[N: static int] = object
data: array[N, float]
macro genCompatibleTuple(t: typed): untyped =
let typ = t.getType[1].getTypeImpl[2]
result = nnkTupleTy.newTree()
for i, ch in typ: # is `nnkObjectTy`
result.add nnkIdentDefs.newTree(ident(ch[0].strVal), # ch is `nnkIdentDefs`
ch[1],
newEmptyNode())
var tmp: genCompatibleTuple(Spectrum[60])Just normal generics without statics has the same problem: import macros
type
Spectrum[T] = object
data: T
macro genCompatibleTuple(t: typed): untyped =
let typ = t.getType[1].getTypeImpl[2]
result = nnkTupleTy.newTree()
for i, ch in typ: # is `nnkObjectTy`
result.add nnkIdentDefs.newTree(ident(ch[0].strVal), # ch is `nnkIdentDefs`
ch[1],
newEmptyNode())
var tmp: genCompatibleTuple(Spectrum[int])Basically the import macros
type
Spectrum[T] = object
data: T
macro genCompatibleTuple(t: typed): untyped =
let typ = t.getTypeImpl[2] # note no .getType[1]
result = nnkTupleTy.newTree()
for i, ch in typ: # is `nnkObjectTy`
result.add nnkIdentDefs.newTree(ident(ch[0].strVal), # ch is `nnkIdentDefs`
ch[1],
newEmptyNode())
var t: Spectrum[int]
var tmp: genCompatibleTuple(t)Or in the original example: macro genCompatibleTuple*(t: typed): untyped =
let typ = t.getTypeImpl[2]
...
proc copyFlat[T: object | tuple](x: T) =
var tmp: genCompatibleTuple(block:
var t: T
t)
...Or a wrapper for the above: macro genCompatibleTupleImpl(t: typed): untyped =
let typ = t.getTypeImpl[2]
...
template genCompatibleTuple*(T: typed): untyped =
var t: T
genCompatibleTupleImpl(t)
proc copyFlat[T: object | tuple](x: T) =
var tmp: genCompatibleTuple(T)
...Please don't bisect this, the behavior is the same in all versions. |
metagn
added a commit
to metagn/Nim
that referenced
this issue
Sep 4, 2023
Araq
pushed a commit
that referenced
this issue
Sep 5, 2023
fixes #22639 A `tyGenericInst` has its last son as the instantiated body of the original generic type. However this type keeps its original `sym` field from the original generic types, which means the sym's type is uninstantiated. This causes problems in the implementation of `getType`, where it uses the `sym` fields of types to represent them in AST, the relevant example for the issue being [here](https://github.com/nim-lang/Nim/blob/d13aab50cf465a7f2edf9c37a4fa30e128892e72/compiler/vmdeps.nim#L191) called from [here](https://github.com/nim-lang/Nim/blob/d13aab50cf465a7f2edf9c37a4fa30e128892e72/compiler/vmdeps.nim#L143). To fix this, create a new symbol from the original symbol for the instantiated body during the creation of `tyGenericInst`s with the appropriate type. Normally `replaceTypeVarsS` would be used for this, but here it seems to cause some recursion issue (immediately gives an error like "cannot instantiate HSlice[T, U]"), so we directly set the symbol's type to the instantiated type. Avoiding recursion means we also cannot use `replaceTypeVarsN` for the symbol AST, and the symbol not having any AST crashes the implementation of `getType` again [here](https://github.com/nim-lang/Nim/blob/d13aab50cf465a7f2edf9c37a4fa30e128892e72/compiler/vmdeps.nim#L167), so the symbol AST is set to the original generic type AST for now which is what it was before anyway. Not sure about this because not sure why the recursion issue is happening, putting it at the end of the proc doesn't help either. Also not sure if the `cl.owner != nil and s.owner != cl.owner` condition from `replaceTypeVarsS` is relevant here. This might also break some code if it depended on the original generic type symbol being given.
metagn
added a commit
to metagn/Nim
that referenced
this issue
Sep 5, 2023
reverts nim-lang#22642 but keeps nim-lang#22639 fixed
metagn
added a commit
to metagn/Nim
that referenced
this issue
Sep 5, 2023
reverts nim-lang#22642, reopens nim-lang#22639, closes nim-lang#22646, refs nim-lang#22640, refs alaviss/union#51
metagn
added a commit
that referenced
this issue
Sep 6, 2023
metagn
added a commit
to metagn/Nim
that referenced
this issue
Sep 6, 2023
Araq
pushed a commit
that referenced
this issue
Sep 7, 2023
fixes #22639 for the third time Nodes generated by `getType` for `tyGenericInst` types, instead of having the original `tyGenericInst` type, will have the type of the last child (due to the `mapTypeToAst` calls which set the type to the given argument). This will cause subsequent `getType` calls to lose information and think it's OK to use the sym of the instantiated type rather than fully expand the generic instantiation. To prevent this, update the type of the node from the `mapTypeToAst` calls to the full generic instantiation type.
narimiran
pushed a commit
that referenced
this issue
Apr 17, 2024
fixes #22639 for the third time Nodes generated by `getType` for `tyGenericInst` types, instead of having the original `tyGenericInst` type, will have the type of the last child (due to the `mapTypeToAst` calls which set the type to the given argument). This will cause subsequent `getType` calls to lose information and think it's OK to use the sym of the instantiated type rather than fully expand the generic instantiation. To prevent this, update the type of the node from the `mapTypeToAst` calls to the full generic instantiation type. (cherry picked from commit ed9e3cb)
This was referenced Dec 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
While
sizeofgenerally works correctly also for arrays that are fields for objects of the formFoo[N: static int], when some macro magic comes into play, the size is sometimes not correctly reported / a debug compiler throws an assertion error.Maybe this can be simplified further, but it shows the problem:
Nim Version
Nim Compiler Version 2.1.1 [Linux: amd64]
Compiled at 2023-09-04
Copyright (c) 2006-2023 by Andreas Rumpf
git hash: c5495f4
active boot switches:
(the debug compiler that produces the assertion error; I got the same problem on a pre 2.0 compiler though)
Current Output
Expected Output
Possible Solution
The
sizeofcall handes thearraytype here:https://github.com/nim-lang/Nim/blob/devel/compiler/sizealignoffsetimpl.nim#L264-L276,
which calls
lengthOrdwith the problematiclastOrdcall here:https://github.com/nim-lang/Nim/blob/devel/compiler/types.nim#L951
As we have a
tyRangein thetyArrayof the form0..static(pred(N, 1)), we end up in:https://github.com/nim-lang/Nim/blob/devel/compiler/types.nim#L867-L870
calling
getOrdValue. Now the issue is thatgetOrdValuegets ankStaticExpr, which is not handled here:https://github.com/nim-lang/Nim/blob/devel/compiler/types.nim#L120-L142
meaning it returns
onError. That eventually leads to the assertion error in the debug compiler.Intuitively it seems like the static expression should simply be evaluated to its actual value before / in the
sizeoflogic, but realistically I suppose the actual solution lies further up the stack? I suppose the actual bug is that we are still left with a static expression at this point in the process in the first place? My attempts at fixing this have all failed at least. :)Additional Information
No response
The text was updated successfully, but these errors were encountered: