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

[Regression] Missing type declarations in C code if members are used in {.emit.} #7363

Closed
yglukhov opened this issue Mar 19, 2018 · 18 comments
Closed

Comments

@yglukhov
Copy link
Member

yglukhov commented Mar 19, 2018

Introduced in 70b28a3.

type Foo {.exportc.} = object
  a: cint
proc f(foo: ptr Foo): cint =
  {.emit: "`result` = `foo`->a;".}
echo f(nil)
Error: execution of an external compiler program 'clang -c  -w  -I/Users/yglukhov/Projects/nim/lib -o /Users/yglukhov/nimcache/test.o /Users/yglukhov/nimcache/test.c' failed with exit code: 1

/Users/yglukhov/nimcache/test.c:82:14: error: incomplete definition of type 'struct Foo'
        result = foo->a;
                 ~~~^
/Users/yglukhov/nimcache/test.c:24:16: note: forward declaration of 'struct Foo'
typedef struct Foo Foo;
               ^
1 error generated.

@cooldome, can we fix it so that {.exportc.} types are always declared completely?

@cooldome
Copy link
Member

Hi Yuriy,
I think exportc is irrelevant here, what is actually needs to be patched is genEmit. I will have a look

@cooldome
Copy link
Member

I have a patch, I will prepare PR later today

@Zdreni
Copy link

Zdreni commented Mar 19, 2018

I think this may be related to that issue. Seems that ptr or var reference to structure type doesn't cause generation of C code for that type. For example:

type stbtt_hheap_chunk {.exportc.} = object
    next: ptr stbtt_hheap_chunk

proc test(x: ptr stbtt_hheap_chunk) =
   {.emit: """
   if (`x`->next) {} else {}
   """.}

However adding next line:

var testHeapChunk: stbtt_hheap_chunk

causes code to be generated

@cooldome
Copy link
Member

Hi Zdreni,
Your example compiles without an issue. Exclusion of stbtt_hheap_chunk is intensional in this case, you are not using it

@cooldome
Copy link
Member

Zdreni, maybe you have more comprehensive example

@yglukhov
Copy link
Member Author

The talk is probably about ttf lib which is exactly the reason why I filed this issue.

@Zdreni
Copy link

Zdreni commented Mar 19, 2018

Sorry, example was fixed. This was a bit shortened extraction of code from ttf (0.2.4) nimble package.
The error should be: "incomplete definition of type 'struct stbtt_hheap_chunk'"

@cooldome cooldome mentioned this issue Mar 19, 2018
@Araq Araq closed this as completed in 2323057 Mar 20, 2018
@Zdreni
Copy link

Zdreni commented Mar 21, 2018

After fix, the issue still remains in other form. Just use additional level of indirection:

type stbtt_hheap_chunk {.exportc.} = object
    next: ptr stbtt_hheap_chunk

type stbtt_hheap {.exportc.} = object
    head: ptr stbtt_hheap_chunk

proc test(x: ptr stbtt_hheap) =
   {.emit: """
   if (`x`->head->next) {} else {}
   """.}

Causes:
error: incomplete definition of type 'struct stbtt_hheap_chunk'

@Araq Araq reopened this Mar 22, 2018
@Zdreni
Copy link

Zdreni commented Mar 22, 2018

I also need to mention that simple tracking of field names mentioned in emit possibly may not help in corner cases where name of field is generated by macro/concatenating two strings. TBH, I doubt there is a reason for optimizing out structures, that may be possibly used in emit blocks.

@cooldome
Copy link
Member

checking...

@cooldome
Copy link
Member

cooldome commented Mar 22, 2018

I suggest I'll add support for expression backticking in emit pragma:
Then the following is going to work:

proc test(x: ptr stbtt_hheap) =
   {.emit: """
   if (`x.head.next`) {} else {}
   """.}

and also

proc test(x: ptr stbtt_hheap) =
   {.emit: """
   if (`x.head`->next) {} else {}
   """.}

It is useful feature on its own plus it will allow Nim to trace type usage properly.

Let me know if you are ok with the proposal.

@yglukhov
Copy link
Member Author

@cooldome, I'm ok with your proposal, although I think exportc is a nice indicator for the type to be defined. I mean why else would you want to place exportc on it?

@Zdreni
Copy link

Zdreni commented Mar 23, 2018

Proposed solution with expression backticking, however, will not work on lot of existing code, leaving packages broken. And yes, I agree with @yglukhov. I see no harm in generating exportc structures, because being not used they will be wiped by C compiler.

cooldome pushed a commit to cooldome/Nim that referenced this issue Mar 23, 2018
@cooldome
Copy link
Member

@yglukhov

Yuriy,
What you are asking is not unreasonable, but it has nothing to do with this regression. Compiler did ignore not used types before and it ignores them now, no changes to exportc treatment were introduced either.
I suggest raising a separate feature request. The implementation would require codegen pass to walk the type definitions (currently type sections completely ignored) and check their flags.

@cooldome
Copy link
Member

I have submittted the PR with backticking of expressions

@yglukhov
Copy link
Member Author

Seems like this issue was closed by irrelevant PR. Also note, that this can be easily worked around by adding

(void)sizeof(`MyType`);

to the emit pragma.

@ghost ghost reopened this Mar 30, 2018
@cooldome
Copy link
Member

I have raised separate feature request #7448
Mean while I suggest using proposed workaround and close this issue.

@Araq
Copy link
Member

Araq commented Dec 5, 2018

Too much of a fringe case.

@Araq Araq closed this as completed Dec 5, 2018
@Araq Araq added the Won't Fix label Dec 5, 2018
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

5 participants