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

[devel regression] {.exportc.} tagged procs no longer export to compiled .so objects #13416

Closed
kaushalmodi opened this issue Feb 14, 2020 · 6 comments · Fixed by #13425
Closed

Comments

@kaushalmodi
Copy link
Contributor

kaushalmodi commented Feb 14, 2020

This is a serious show stopper for my SystemVerilog projects relying on Nim libaries.

In my Nim libraries, I export the procs using the {.exportc.} pragma, and the C layer of SystemVerilog imports those exported symbols to the SystemVerilog side.

But now the {.exportc.} procs do not get exported at all.

Example

Compile the below file on Linux using nim c --out:libdpi.so --app:lib -d:release --gc:none libdpi.nim.

# libdpi.nim
proc hello() {.exportc.} =
  let
    str = when defined(cpp):
            "C++!"
          else:
            "C!"
  echo "Hello from Nim via " & str

Now run nm ./libdpi.so | grep -P '\bT\b' (Note: This is nm binary found on Linux systems, not nim.)

Current Output

!! hello symbol is not exported !!

0000000000007ab0 T NimMain
0000000000007b54 T _fini
00000000000007f8 T _init

Expected Output

Note that the hello symbol is exported: 0000000000005f80 T hello.

0000000000006070 T NimMain
00000000000060a0 T NimMainInit
0000000000005f70 T NimMainInner
0000000000006040 T PreMain
0000000000005f60 T PreMainInner
00000000000060f4 T _fini
00000000000018b0 T _init
0000000000004bf0 T addChar
00000000000040c0 T copyString
0000000000003eb0 T cstrToNimstr
0000000000001c10 T echoBinSafe
0000000000005f80 T hello
0000000000005710 T incrSeqV3
0000000000004d10 T mnewString
0000000000005610 T mulInt
00000000000047b0 T newObj
00000000000056b0 T newSeq
0000000000004f70 T nimIntToStr
00000000000054f0 T raiseExceptionEx
00000000000057c0 T raiseIndexError2
00000000000055b0 T raiseOverflow
0000000000004b20 T rawNewString
0000000000003af0 T rawNewStringNoInit
0000000000004b50 T resizeString
0000000000004d20 T setLengthStr
0000000000003ee0 T signalHandler
0000000000003e80 T toNimStr

Possible Solution

None that I know of.

Additional Information

$ nim -v
Nim Compiler Version 1.1.1 [Linux: amd64]
Compiled at 2020-02-13
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: 1e303100f8ca32e60b154bf997a9102f36eb23d0
active boot switches: -d:release
@kaushalmodi kaushalmodi changed the title [showstopper] {.exportc.} tagged procs no longer export to compiled .so objects [showstopper] [devel regression] {.exportc.} tagged procs no longer export to compiled .so objects Feb 14, 2020
@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Feb 14, 2020

I already found this difference.. when the symbol exporting was working, the hello symbol had capital T before it in the nm output.

> nm ./libdpi_64.good.so | rg hello 
0000000000005f80 T hello

In the not-working .so, the hello symbol has lower-case t before it.

> nm ./libdpi_64.bad.so | rg hello 
00000000000079c0 t hello

I do not understand the deal betweeen T and t, but apparently it definitely needs to be T for the Nim/SystemVerilog interop to work.

@kaushalmodi kaushalmodi changed the title [showstopper] [devel regression] {.exportc.} tagged procs no longer export to compiled .so objects [devel regression] {.exportc.} tagged procs no longer export to compiled .so objects Feb 14, 2020
@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Feb 14, 2020

Ref:

@kaushalmodi
Copy link
Contributor Author

This got fixed after adding dynlib pragma to exportc:

proc hello() {.exportc, dynlib.} =
  let
    str = when defined(cpp):
            "C++!"
          else:
            "C!"
  echo "Hello from Nim via " & str

Now I see:

> nm ./libdpi_64.so | rg '\bT\b'
0000000000007ad0 T NimMain
0000000000007b74 T _fini
0000000000000818 T _init
00000000000079e0 T hello

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Feb 14, 2020

@alaviss Can you please add a changelog for this, and also update the manual: https://nim-lang.github.io/Nim/manual#foreign-function-interface-exportc-pragma

It needs to be highlighted that when the user expects to export a symbol to a .so, etc., it needs to have both {.exportc, dynlib.}.

This also needs to change: https://nim-lang.github.io/Nim/manual#foreign-function-interface-dynlib-pragma-for-export

This pragma only has an effect for the code generation on the Windows target, so when this pragma is forgotten and the dynamic library is only tested on Mac and/or Linux, there won't be an error.


I seriously wonder if there's a way for .exportc. to now implicitly do .dynlib.

From my perspective, it looks like:

  • Most of .exportc.'s use requires .dynlib. too, so may be .exportc. should automatically behave as if .dynlib. was used.
  • .dynlib. can then be deprecated ..
  • Can .exportc. respect a special symbol in its argument? Like: .exportc: "!". ? Another example with a specified export symbol name: {.export: "!foo".}.

That would then export the symbols locally (the nm t symbol) instead of globally (the nm T symbol).

This proposed change would be much less breaking as the {.export: "!".} use, I believe, would be mainly internal to the nim repo, right?

kaushalmodi added a commit to kaushalmodi/nim-systemverilog-dpic that referenced this issue Feb 14, 2020
@juancarlospaco
Copy link
Collaborator

Awesome research.

alaviss added a commit to alaviss/Nim that referenced this issue Feb 17, 2020
Araq pushed a commit that referenced this issue Feb 18, 2020
* manual: documents changes regarding dynlib

Closes #13416

* manual: clean up sentence phrasing
@timotheecour
Copy link
Member

I seriously wonder if there's a way for .exportc. to now implicitly do .dynlib.

nah, visibility is not same as mangling. The change to dynlib was a good one.

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