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

Clang error on Rosencrantz #9441

Closed
andreaferretti opened this issue Oct 19, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@andreaferretti
Copy link
Collaborator

commented Oct 19, 2018

When I try to compile a server written with Rosencrantz I get a code generation error:

Error: execution of an external compiler program 'clang -c  -w  -I/Users/andrea/.mynim/devel/lib -o /Users/andrea/.cache/nim/todo_d/rosencrantz_core.c.o /Users/andrea/.cache/nim/todo_d/rosencrantz_core.c' failed with exit code: 1

/Users/andrea/.cache/nim/todo_d/rosencrantz_core.c:1956:29: error: use of undeclared identifier 'Result'
        unsureAsgnRef((void**) (&(*Result).ClE_0), T1_.ClE_0);
                                   ^
/Users/andrea/.cache/nim/todo_d/rosencrantz_core.c:1957:4: error: use of undeclared identifier 'Result'
        (*Result).ClP_0 = T1_.ClP_0;
          ^
/Users/andrea/.cache/nim/todo_d/rosencrantz_core.c:1979:42: error: too many arguments to function call, expected single argument 'h', have 2 arguments
        handle_KWZqMKo4jK6QndqNqas79bw(handler, (&T2_));
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~          ^~~~~~
/Users/andrea/.cache/nim/todo_d/rosencrantz_core.c:1942:1: note: 'handle_KWZqMKo4jK6QndqNqas79bw' declared here
N_LIB_PRIVATE N_NIMCALL(tyProc_Uw19bu69asBAAr9cWcwt29abvQ, handle_KWZqMKo4jK6QndqNqas79bw)(tyProc_BVDTkhtEGHG4JR7Jc5jePQ h) {
^
/Users/andrea/.mynim/devel/lib/nimbase.h:184:25: note: expanded from macro 'N_LIB_PRIVATE'
#  define N_LIB_PRIVATE __attribute__((visibility("hidden")))
                        ^
/Users/andrea/.cache/nim/todo_d/rosencrantz_core.c:1981:46: error: operand of type 'tyProc_eUIIOjlOnzfDQx5dZlthOg' where arithmetic or pointer type is required
        T3_.ClP_0 = ((TM_hgnz02HHjbFW63grqbggMg_47) (T2_)); T3_.ClE_0 = NIM_NIL;
                                                    ^~~~~
4 errors generated.

To reproduce, just clone Rosencrantz repo and launch nimble server or nimble todo. I will try to pinpoint the error more exactly, but for now this is what I have.

Nim version is eaca5be and the error must be related to one of the latest commits (I update Nim and test my libraries daily)

@andreaferretti

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 19, 2018

For comparison, version 82a1576 works just fine

@andreaferretti

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 19, 2018

The commit that introduced the issue is eaca5be

@Araq

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Thanks, I assume it's due to the fact that we compute too late that the proc has the closure calling convention.

@cooldome

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

Hi Andrea,

While I looking into it, here is workaound if you are interested.
The workaround is change line in core.nim from:
proc server(req: Request): Future[void] {.async.}
to
proc server(req: Request): Future[void] {.closure, async.}

@cooldome

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

Ok,

Here is the proc causing the problem


proc handle*(h: Handler): auto =
  proc server(req: Request): Future[void] {.async.} =
    let emptyCtx = Context(
      position: 0,
      accept: true,
      headers: emptyList[StrPair]()
    )
    var reqHeap = new(Request)
    reqHeap[] = req
    var
      f: Future[Context]
      ctx: Context
    try:
      f = h(reqHeap, emptyCtx)
      ctx = await f
    except:
      discard
    if f.failed:
      await req.respond(Http500, "Server Error", {"Content-Type": "text/plain;charset=utf-8"}.newHttpHeaders)
    else:
      if not ctx.accept:
        await req.respond(Http404, "Not Found", {"Content-Type": "text/plain;charset=utf-8"}.newHttpHeaders)

  return server

The issue that its return type is auto and the compiler doesn't know the proc is going to return is closure until lambda lifting is done.
I have tried changing the codegen to call transformBody before generating the header. It helped to get proc handle correct, but proc serve remained broken as it was generated before handle was transformed.

IMO,
the correct fix instead would be to add to transformSymAux a rule like:

if s.kind in routineKinds and mayIncludeClosure(s.typ[0]):
      discard transformBody(c.graph, s)

But I surprised to see that the calling convention of the return type for handle was ccDefault.

@Araq,
I thought that semantic should be conservative and if it is not sure if proc is normal or closure, it should put closure conservatively and let lambda lifting to do the exact checking. Am I right?

@andreaferretti

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 22, 2018

@cooldome Great! In fact I am fine with being a little more explicit and adding the closure annotation, I will just leave it there

@Araq

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

I thought that semantic should be conservative and if it is not sure if proc is normal or closure, it should put closure conservatively and let lambda lifting to do the exact checking. Am I right?

There is indeed logic for this but I think this logic should be moved to sempass2.

Araq added a commit that referenced this issue Dec 5, 2018

@Araq Araq closed this in 7a0191a Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.