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

Defining multiple structs after the same typedef causes compilation error #47

Closed
kaushalmodi opened this issue Jan 19, 2019 · 20 comments
Closed

Comments

@kaushalmodi
Copy link

kaushalmodi commented Jan 19, 2019

Hello,

I installed nimterop using nimble.

Here is the minimum working example:

# libdpi.nim
import nimterop/cimport

cImport("svdpi.h")

# Input: none
# Return: none
proc hello() {.exportc.} =
  echo "svdpi version: " & svDpiVersion()
  echo "Hello from Nim!"

and the svdpi.h (this is a publicly shareable file, so you can add this to your tests too if you like).

On running:

nim c --out:libdpi.so --app:lib libdpi.nim

I get this error:

Hint: unicodeplus [Processing]
Importing /home/kmodi/sandbox/systemverilog/dpi_c/nim_svdpi/svdpi.h
toast --pnim --preprocess /home/kmodi/sandbox/systemverilog/dpi_c/nim_svdpi/svdpi.h
stack trace: (most recent call last)
../../../../.nimble/pkgs/nimterop-0.1.0/nimterop/cimport.nim(230, 22) cImport
../../../../.nimble/pkgs/nimterop-0.1.0/nimterop/cimport.nim(90, 11) getToast
../../../../stow/pkgs/nim/devel/lib/system.nim(3989, 20) failedAssertImpl
../../../../stow/pkgs/nim/devel/lib/system.nim(3982, 11) raiseAssert
../../../../stow/pkgs/nim/devel/lib/system.nim(3016, 7) sysFatal
libdpi.nim(4, 8) template/generic instantiation of `cImport` from here
../../../../stow/pkgs/nim/devel/lib/system.nim(3016, 7) Error: unhandled exception: /home/kmodi/.nimble/pkgs/nimterop-0.1.0/nimterop/cimport.nim(90, 12) `exitCode == 0` 1
@kaushalmodi kaushalmodi changed the title Attempting to use cInclude a header file but crashes Attempting to cInclude a header file but crashes Jan 19, 2019
@zedeus
Copy link

zedeus commented Jan 19, 2019

What happens if you run this line?
toast --pnim --preprocess /home/kmodi/sandbox/systemverilog/dpi_c/nim_svdpi/svdpi.h

@kaushalmodi
Copy link
Author

kaushalmodi commented Jan 19, 2019

Thanks, in that toast output, I found a gcc command, and running that I realized that I had forgotten to put this svdpi_compatibility.h in the path too.

But after adding that too, I get error:

libdpi.nim(5, 8) template/generic instantiation of `cImport` from here
../../../../stow/pkgs/nim/devel/lib/core/macros.nim(525, 20) Error: undeclared identifier: 's_vpi_vecval'

@zedeus
Copy link

zedeus commented Jan 19, 2019

Seems like a bug.

After --preprocess:

typedef struct vpi_vecval {
    uint32_t a;
    uint32_t b;
} s_vpi_vecval, *p_vpi_vecval;
typedef s_vpi_vecval svLogicVecVal;

After --pnim:

type
  svLogicVecVal* = s_vpi_vecval

Bug gets triggered by defining multiple structs after the same typedef.

Possibly a tree-sitter bug?

typedef struct foo {
    int i;
} a, b;

Gets translated to:

(translation_unit 1 1 39
 (type_definition 1 1 39
  (struct_specifier 1 9 25
   (type_identifier 1 16 3)
   (field_declaration_list 1 20 14
    (field_declaration 2 5 6
     (primitive_type 2 5 3)
     (field_identifier 2 9 1)
    )
   )
  )
  (ERROR 3 3 2
   (type_identifier 3 3 1)
  )
  (type_identifier 3 6 1)
 )
)

But without the extra name:

(translation_unit 1 1 36
 (type_definition 1 1 36
  (struct_specifier 1 9 25
   (type_identifier 1 16 3)
   (field_declaration_list 1 20 14
    (field_declaration 2 5 6
     (primitive_type 2 5 3)
     (field_identifier 2 9 1)
    )
   )
  )
  (type_identifier 3 3 1)
 )
)

@zedeus
Copy link

zedeus commented Jan 19, 2019

Issue addressing the bug here tree-sitter/tree-sitter-cpp#30

@kaushalmodi kaushalmodi changed the title Attempting to cInclude a header file but crashes Defining multiple structs after the same typedef causes compilation error Jan 19, 2019
@kaushalmodi
Copy link
Author

kaushalmodi commented Jan 19, 2019

Thanks guys!

It compiles after this workaround:

import strformat
import nimterop/cimport

# Below manual definition of s_vpi_vecval is a workaround for
# https://github.com/genotrance/nimterop/issues/47.
type
  s_vpi_vecval* {.importc: "s_vpi_vecval".} = object
    aval: uint32
    bval: uint32

cImport("svdpi.h")

# Input: none
# Return: none
proc hello() {.exportc.} =
  echo svDpiVersion()
  echo "Hello from Nim!"

genotrance added a commit that referenced this issue Jan 19, 2019
@genotrance
Copy link
Collaborator

I'm thinking this can be marked as fixed by providing better error handling when preprocessor complains. Nimterop will now highlight preprocessor errors. It will make it more obvious to users.

We can track the tree-sitter bug in their issue tracker. Let me know if this is acceptable.

@kaushalmodi
Copy link
Author

I'm thinking this can be marked as fixed by providing better error handling when preprocessor complains. Nimterop will now highlight preprocessor errors. It will make it more obvious to users.

That's great! For this issue, I had to run toast to get the gcc command, and then the gcc command to get the actual error. With improved error, will I get to see the error thrown by gcc?

We can track the tree-sitter bug in their issue tracker.

Agreed.

Let me know if this is acceptable.

Yes, it is. Thanks.

@genotrance
Copy link
Collaborator

Yes you should see the error right there inline. Can you please try with this same library?

@kaushalmodi
Copy link
Author

kaushalmodi commented Jan 19, 2019

Yes, now I get:

st --pnim --preprocess /home/kmodi/sandbox/systemverilog/dpi_c/nim_svdpi/svdpi.h
stack trace: (most recent call last)
../../../../.nimble/pkgs/nimterop-0.1.0/nimterop/cimport.nim(237, 22) cImport
../../../../.nimble/pkgs/nimterop-0.1.0/nimterop/cimport.nim(98, 11) getToast
../../../../stow/pkgs/nim/devel/lib/system.nim(3989, 20) failedAssertImpl
../../../../stow/pkgs/nim/devel/lib/system.nim(3982, 11) raiseAssert
../../../../stow/pkgs/nim/devel/lib/system.nim(3016, 7) sysFatal
libdpi.nim(11, 8) template/generic instantiation of `cImport` from here
../../../../stow/pkgs/nim/devel/lib/system.nim(3016, 7) Error: unhandled exception: /home/kmodi/.nimble/pkgs/nimterop-0.1.0/nimterop/cimport.nim(98, 12) `ret == 0`
ERROR: : svdpi_compatibility.h: No such file or directory

image

Minor nitpick: Can that last ERROR also be in red color as the Error above?


Note that I reproduced the earlier error. Not the last one.

@genotrance
Copy link
Collaborator

Am not keen on adding a terminal dependency into cimport at this time. Will reconsider later when I get a chance to evaluate performance and binary size impacts.

Closing.

@kaushalmodi
Copy link
Author

Should this issue be reopened given that the upstream bug is fixed: tree-sitter/tree-sitter-cpp#30?

Will that fix auto-propagate to nimterop? Or does nimterop need to be updated to absorb that upstream fix?

@genotrance
Copy link
Collaborator

I believe this works in nimterop as of today.

@kaushalmodi
Copy link
Author

kaushalmodi commented May 14, 2019

I removed this workaround code:

type
  s_vpi_vecval* {.importc: "s_vpi_vecval".} = object
    aval: uint32
    bval: uint32

and updated nimterop from head.

But I still the same error:

/home/kmodi/usr_local/apps/6/nim/devel/lib/core/macros.nim(595, 33) Error: undeclared identifier: 's_vpi_vecval'


Full Nim wrapper

# svdpi.nim
import nimterop/cimport
import os

static:
  cDisableCaching()

const
  xlmIncludePath = getEnv("XCELIUM_ROOT") / ".." / "include"
static:
  doAssert fileExists(xlmIncludePath / "svdpi.h")
  doAssert fileExists(xlmIncludePath / "svdpi_compatibility.h")
  cAddSearchDir(xlmIncludePath)

cDefine("DPI_COMPATIBILITY_VERSION_1800v2012")

# # Below manual definition of s_vpi_vecval is a workaround for
# # https://github.com/nimterop/nimterop/issues/47.
# type
#   s_vpi_vecval* {.importc: "s_vpi_vecval", header: xlmIncludePath / "svdpi.h".} = object
#     aval*: uint32 # we need to export the object elements too!
#     bval*: uint32

cImport(cSearchPath("svdpi.h"))

Snippet from svdpi.h Header file

#ifndef VPI_VECVAL
#define VPI_VECVAL
typedef struct vpi_vecval {
    uint32_t a;
    uint32_t b;
} s_vpi_vecval, *p_vpi_vecval;
#endif

/* (a chunk of) packed logic array */
typedef s_vpi_vecval svLogicVecVal;

Ref

Matching output from cDebug()

It looks like the s_vpi_vecval and *p_vpi_vecval do not get defined at all.

# scalar
#
# * DPI representation of packed arrays.
# * 2-state and 4-state vectors, exactly the same as PLI's avalue/bvalue.
#
# (a chunk of) packed logic array
  svLogicVecVal* {.impsvdpi.} = s_vpi_vecval

@kaushalmodi
Copy link
Author

kaushalmodi commented May 14, 2019

OK .. what's interesting is that svdpi_compatibility.h is not getting parsed at all.

import nimterop/cimport
import os

static:
  cDisableCaching()
  cDebug()

const
  xlmIncludePath = getEnv("XCELIUM_ROOT") / ".." / "include"
static:
  doAssert fileExists(xlmIncludePath / "svdpi_compatibility.h")
  cAddSearchDir(xlmIncludePath)

cDefine("DPI_COMPATIBILITY_VERSION_1800v2012")
cImport(cSearchPath("svdpi_compatibility.h"))

This outputs:

{.passC: "-DDPI_COMPATIBILITY_VERSION_1800v2012".}

# Importing /masking/the/actual/company/internal/path/svdpi_compatibility.h
# Generated at 2019-05-14T15:38:10-04:00
# Command line:
#   /home/kmodi/.nimble/pkgs/nimterop-#head/nimterop/toast --pnim --preprocess --defines+=DPI_COMPATIBILITY_VERSION_1800v2012 --nim:/home/kmodi/usr_local/apps/6/nim/devel/bin/nim /masking/the/actual/company/internal/path/svdpi_compatibility.h

{.hint[ConvFromXtoItselfNotNeeded]: off.}

import nimterop/types

const
  headersvdpi_compatibility {.used.} = "/masking/the/actual/company/internal/path/svdpi_compatibility.h"

{.pragma: impsvdpi_compatibility, importc, header: headersvdpi_compatibility.}
{.pragma: impsvdpi_compatibilityC, impsvdpi_compatibility, cdecl.}

@kaushalmodi
Copy link
Author

kaushalmodi commented May 14, 2019

Actually, it's the same bug. I can easily reproduce with just this in svdpi_compatibility.h:

typedef struct t_vpi_vecval {
uint32_t aval;
uint32_t bval;
} s_vpi_vecval, *p_vpi_vecval;

With above, I get:

# Generated at 2019-05-14T15:44:52-04:00
# Command line:
#   /home/kmodi/.nimble/pkgs/nimterop-#head/nimterop/toast --pnim --preprocess --defines+=DPI_COMPATIBILITY_VERSION_1800v2012 --nim:/home/kmodi/usr_local/apps/6/nim/devel/bin/nim ../includes/svdpi_compatibility.h

{.hint[ConvFromXtoItselfNotNeeded]: off.}

import nimterop/types

const
  headersvdpi_compatibility {.used.} = "../includes/svdpi_compatibility.h"

{.pragma: impsvdpi_compatibility, importc, header: headersvdpi_compatibility.}
{.pragma: impsvdpi_compatibilityC, impsvdpi_compatibility, cdecl.}

With:

typedef struct t_vpi_vecval {
uint32_t aval;
uint32_t bval;
} s_vpi_vecval;

( I removed the , *p_vpi_vecval portion. )

I get:

# Generated at 2019-05-14T15:46:53-04:00
# Command line:
#   /home/kmodi/.nimble/pkgs/nimterop-#head/nimterop/toast --pnim --preprocess --defines+=DPI_COMPATIBILITY_VERSION_1800v2012 --nim:/home/kmodi/usr_local/apps/6/nim/devel/bin/nim ../includes/svdpi_compatibility.h

{.hint[ConvFromXtoItselfNotNeeded]: off.}

import nimterop/types

const
  headersvdpi_compatibility {.used.} = "../includes/svdpi_compatibility.h"

{.pragma: impsvdpi_compatibility, importc, header: headersvdpi_compatibility.}
{.pragma: impsvdpi_compatibilityC, impsvdpi_compatibility, cdecl.}

type
  t_vpi_vecval* {.importc: "struct t_vpi_vecval", header: headersvdpi_compatibility, bycopy.} = object
    aval*: uint32
    bval*: uint32
  s_vpi_vecval* {.impsvdpi_compatibility, bycopy.} = t_vpi_vecval

@genotrance
Copy link
Collaborator

Can you delete the build folder and rebuild nimterop? You're probably still using the old tree-sitter.

@kaushalmodi
Copy link
Author

Can you delete the build folder and rebuild nimterop?

I had already done that. I again did:

nimble uninstall nimterop
nimble install nimterop@#head
nim c svdpi.nim

and I got the same error.

I confirmed the time stamp of the toast binary and the files under ~/.nimble/pkgs/nimterop-#head/nimterop/treesitter/ .. they all are the latest.

You're probably still using the old tree-sitter.

How do I verify the tree-sitter version?

@kaushalmodi
Copy link
Author

Looks like the ~/.nimble/pkgs/nimterop-#head/build is auto-deleted each time after nimble install nimterop@#head. So I should be always fetching the latest tree-sitter.

@kaushalmodi
Copy link
Author

I can reproduce the same issue even after pulling latest nimterop and doing nimble install.

@kaushalmodi
Copy link
Author

@genotrance Once you cut a new release of nimterop, I plan to update my svdpi wrapper with this change: kaushalmodi/nim-svdpi@57268e7

kaushalmodi added a commit to kaushalmodi/nim-svdpi that referenced this issue May 12, 2020
Ref: nimterop/nimterop#47

- Add "wrap" nimscript task.
- Bump nimterop dep and svdpi version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants