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

lent parameter: invalid C codegen #16898

Closed
mratsim opened this issue Feb 1, 2021 · 6 comments · Fixed by #17628
Closed

lent parameter: invalid C codegen #16898

mratsim opened this issue Feb 1, 2021 · 6 comments · Fixed by #17628

Comments

@mratsim
Copy link
Collaborator

mratsim commented Feb 1, 2021

While trying to work around #16897 I got the compiler to generate invalid C with lent input parameters.

Test case

type
  Fp[N: static int, T] = object
    big: array[N, T]

type
  QuadraticExt* = concept x
    ## Quadratic Extension concept (like complex)
    type BaseField = auto
    x.c0 is BaseField
    x.c1 is BaseField

{.experimental:"views".}

func prod(r: var QuadraticExt, a, b: lent QuadraticExt) =
  discard

type
  Fp2[N: static int, T] = object
    c0, c1: Fp[N, T]

# This should be passed by reference,
# but concepts do not respect the 24 bytes rule
# or `byref` pragma.
var r, a, b: Fp2[6, uint64]

prod(r, a, b)

The proc is defined with pointers:

/* section: NIM_merge_PROCS */
N_LIB_PRIVATE N_NIMCALL(void, prod__j3ijsYk5xsFR79akOCPoMqg)(tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* r, tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* a, tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* b) {
}

but is called with values

N_LIB_PRIVATE N_NIMCALL(void, NimMainModule)(void) {
{
	prod__j3ijsYk5xsFR79akOCPoMqg((&r__mSc0P1JKql3o5d2mQ9bx9cMQ), a__IakjUEvgThFFu4dR6kEN0A, b__4KxaakBDXQrcdXChd3FntQ);
}
}

Compiler error

[...]/nimcache/concept_param/@mconcept_param.nim.c: In function ‘NimMainModule’:
[...]/nimcache/concept_param/@mconcept_param.nim.c:99:64: error: incompatible type for argument 2 of ‘prod__j3ijsYk5xsFR79akOCPoMqg’
   99 |  prod__j3ijsYk5xsFR79akOCPoMqg((&r__mSc0P1JKql3o5d2mQ9bx9cMQ), a__IakjUEvgThFFu4dR6kEN0A, b__4KxaakBDXQrcdXChd3FntQ);
      |                                                                ^~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                |
      |                                                                tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ
[...]/nimcache/concept_param/@mconcept_param.nim.c:56:143: note: expected ‘tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ *’ but argument is of type ‘tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ’
   56 | ALL(void, prod__j3ijsYk5xsFR79akOCPoMqg)(tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* r, tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* a, tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* b) {
      |                                                                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

[...]/nimcache/concept_param/@mconcept_param.nim.c:99:91: error: incompatible type for argument 3 of ‘prod__j3ijsYk5xsFR79akOCPoMqg’
   99 |  prod__j3ijsYk5xsFR79akOCPoMqg((&r__mSc0P1JKql3o5d2mQ9bx9cMQ), a__IakjUEvgThFFu4dR6kEN0A, b__4KxaakBDXQrcdXChd3FntQ);
      |                                                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                                           |
      |                                                                                           tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ
[...]/nimcache/concept_param/@mconcept_param.nim.c:56:185: note: expected ‘tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ *’ but argument is of type ‘tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ’
   56 | oMqg)(tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* r, tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* a, tyObject_Fp2__SVgH9c6gmNsJfyYVtl0YdSQ* b) {
      |                                                                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

Error: execution of an external compiler program 'gcc -c  -w -fmax-errors=3 -O3 -fno-strict-aliasing -fno-ident   -I'[...]/.choosenim/toolchains/nim-#devel/lib' -I[...]/build -o [...]/nimcache/concept_param/@mconcept_param.nim.c.o [...]/nimcache/concept_param/@mconcept_param.nim.c' failed with exit code: 1
@Araq
Copy link
Member

Araq commented Feb 1, 2021

func prod(r: var QuadraticExt, a, b: lent QuadraticExt) is not a valid declaration. lent is for return types.

@mratsim
Copy link
Collaborator Author

mratsim commented Feb 1, 2021

In that case I think the compiler should refuse to compile.

@Araq
Copy link
Member

Araq commented Feb 1, 2021

Of course but it's more important for me to unblock you...

@mratsim
Copy link
Collaborator Author

mratsim commented Feb 1, 2021

I'm not blocked, I was trying a workaround just in case it worked.

@konsumlamm
Copy link
Contributor

func prod(r: var QuadraticExt, a, b: lent QuadraticExt) is not a valid declaration. lent is for return types.

What is the reasoning behind not allowing lent parameters? (Fwiw, you can also have var and openArray parameters, or are these different somehow?) I imagine it to be useful for guaranteeing that the parameter is only a view of some object and thus cannot be modified (even if it's a view for a ref object). Or is there some alternative/workaround that allows something like that? (I know that func/{.noSideEffect.} disallows side effects, including writing to ref parameters, but that can't be applied to individual parameters afaict.)

@Araq
Copy link
Member

Araq commented Apr 2, 2021

a, b: QuadraticExt already is the immutable view, no need for lent. However, your extension wrt ref object makes some sense. But in general I don't like re-using type modifiers or keywords in an ad-hoc fasion.

@ringabout ringabout assigned ringabout and unassigned ringabout Apr 3, 2021
ringabout added a commit to ringabout/Nim that referenced this issue Apr 3, 2021
ringabout added a commit to ringabout/Nim that referenced this issue Apr 3, 2021
Araq pushed a commit that referenced this issue Apr 6, 2021
* fix #16898
* fix #17621

* Update compiler/semtypes.nim
PMunch pushed a commit to PMunch/Nim that referenced this issue Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants