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

[archive] Read through alterations #1

Closed
umarcor opened this issue Apr 13, 2020 · 0 comments
Closed

[archive] Read through alterations #1

umarcor opened this issue Apr 13, 2020 · 0 comments

Comments

@umarcor
Copy link
Owner

umarcor commented Apr 13, 2020

@RocketRoss, I had to recreate this repo, for it to be identified as a fork of ghdl/ghdl-cosim. Otherwise, I could not open PRs from here. As a result, the discussion related to your PR there is now gone. Although it was already merged, I copied the content here for future reference.


RocketRoss:

Mostly small changes. Biggest ones are:

updating hint about new -shared elaboration flag.
referencing shghdl example in hint warning against calling ghdl-main more than once

https://ghdl-cosim.readthedocs.io/en/latest/vhpidirect/examples/shared.html


RocketRoss:

Brilliant examples. Really nice work!

Only one question is really up for discussion, and it existed in the original documentation but is now questionable because of the work that you're doing:

In the type declarations:

Unconstrained arrays are represented by a fat pointer. Do not use unconstrained arrays in foreign subprograms.

But we are about to finalise V1 of the ghdl.h which shows that unconstrained arrays are very capable and stable (provided one free's malloc'd pointers in their C).


umarcor:

Brilliant examples. Really nice work!

Thanks to you for being the catalyzer! All these examples have been laying around for so many years and I needed an excuse to put them nicely. There is still much more interesting content to be added: packages/shared variables/protected types, using pipes, X windows, environment variables, exit codes, etc. Also, although I know little about VPI, there are also nice things that can be done with it (e.g. SpinalHDL/SpinalHDL#146).

Only one question is really up for discussion, and it existed in the original documentation but is now questionable because of the work that you're doing

This is a general (non)issue with the documentation. Many complementary features of GHDL (cosimulation, language server, synthesis, etc.) were written by Tristan several years ago as plain proofs of concept. At that moment, he wrote the bare minimum descriptions for him (or for any other really advanced user) to remember them in the future. That's why there are multiple comments in the docs regarding "don't do this, it is too complex", which might feel weird/dated today. Those are meant for new users, specially those who know VHDL but not so much about C/compilers.

Progressively, as features improve and get out of the "proof of concept" status, comments in the docs are updated. In this specific case, I think that we are not there yet. When the PR that introduces ghdl.h is discussed, tested and ready, we can remove that comment. Actually, all the "Restrictions on type declarations" need to be rewritten accoding to ghdl.h. I believe the comment about in/out/inout ports is outdated/wrong too.


umarcor:

I think it is ok to point to section 'Linking' instead of 'Wrapping'. However, I'd point to COSIM:VHPIDIRECT:Linking instead of a specific subsection. This is because, in the context where this comment is placed, both Linking_with_foreign_object_files and Linking_with_Ada are valid solutions.

RocketRoss:

There was just more information on actually linking object files to the exec at linking with... than the starting a simulation.

Changed it to reference COSIM:VHPIDIRECT:Linking...


umarcor:

I'm not sure about using "GHDL" here, as it might mislead users thinking that we are talking about GHDL itself. What about "... when the binary (simulation model) is built..."? Or ... when the simulation binary is built...?

RocketRoss:

I was trying to say that it needs to be used when building GHDL.... because that's when I used it: make -Wl,-pie && make install

So far I haven't needed the -Wl,-pie flag when building any simulations.. (I may be forgetting a case, but it doesn't ring a bell).

I made the change because I want to really know when to use it. (:

umarcor:

I believe that the original comment was related to generating an executable binary with GCC, --bind and --list-link. But I'm neither sure. Let's just remove it, and we will add it again (if required) when we have the extended example set of how to build shared libraries.


umarcor:

I'd specify that -shared "replaces the default :option:...". It is not actually a replacement of the feature, because AFAIU it is possible to provide a custom version script.

Hence, GHDL itself can filter the symbols or not; and the user can filter the symbols or not. That provides four different use cases. Apart from that, users might want to expose ghdl_main only, or a list of additional symbols (as in VUnit/cosim). That creates other three use cases. Furthermore, because 3-4 options are listed here, potentially 20+ different ways to do it can exist.

This is why I put a very short comment, and I kept further details in 45e43a5 (which is ahead of master), to be tackled in a separate PR when the initial version is ready.

Moreover, I don't think that the comment about the file name belongs in here. In fact, I've been moving a similar one from a section to another, until I decided to remove it. This is because -o is accepted by ghdl -e. Hence, the possibility to define a custom output name is available in all examples, not only here. I thought that it would be easier if we just used it whenever it is required (linking/bind, wrapping/shghdl). Nonetheless, I think that option -o might be undocumented in the main docs. You can open either an issue or a PR in ghdl/ghdl about it.

Last, I would delay adding the last sentence. I am not sure about it yet. Let's wait until we have written multiple use cases (not one only), so that we can understand/debug this new feature. Overall, I don't want to go mad with introducing all the latest details. I think it is better to focus on the "stable" resources first.

RocketRoss:

Nice to know about the -o

I just commented out my version, and put yours back

I agree, we can update the docs when we know what is going on

AFAIU the -shared replaces the --version-script. In fact I get /etc/ghdl-llvm-PIC/bin/ghdl:error: unknown option '--version-script=./custom.ver' when I run /etc/ghdl-llvm-PIC/bin/ghdl -e -shared --version-script=./custom.ver -Wl,-Wl,-u,ghdl_main -o ghdl.so Bugtest on the latest master-built ghdl exec.

So i wanted to add to the comment in case someone tries --version-script and it throws the same error at them.

I think we may need to get more info from TGingold

umarcor:

AFAIU the -shared replaces the --version-script. In fact I get /etc/ghdl-llvm-PIC/bin/ghdl:error: unknown option '--version-script=./custom.ver' when I run /etc/ghdl-llvm-PIC/bin/ghdl -e -shared --version-script=./custom.ver -Wl,-Wl,-u,ghdl_main -o ghdl.so Bugtest on the latest master-built ghdl exec.

--version-script is not an option of GHDL, but an option of the linker. That's why it needs to be used as ghdl -e -Wl,-Wl,--version-script=. See https://ghdl.readthedocs.io/en/latest/using/CommandReference.html#cmdoption-ghdl-list-link. It is the same reason you use -Wl,-Wl,-u,ghdl_main instead of ghdl -e -u ghdl_main.

I think we may need to get more info from TGingold

Yes. He already told me that he is ok with creating ghdl/ghdl-cosim and archiving ghdl-systemc-fosdem16. As soon as it is done, we can handle this an other issues there.


umarcor:

I believe the comment about in/out/inout ports is outdated/wrong too.

Yeah, I just keep ignoring that section because I can't make heads nor tails of what is cracking

VHDL functions/procedures can have multiple "return" values, since arguments can be of type out. However, this is difficult to map in C. The actual error in the docs is that in is the default, while out and inout are the ones that are difficult.

In practice, these arguments are passed as additional parameters of the function and C is expected to replace/modify them by reference. The context is similar to when you want to pass a string defined in C as an unconstrained array in VHDL. We will have to tackle this in ghdl.h. You might find interesting the conversation starting in December 9, 2019 9:22 PM. It lasted until December 30, 2019 6:47 AM, when, as you see, I opened ghdl/ghdl#1059. Specifically about the 1D, 2D, 3D, etc. see December 12, 2019 12:34 PM.

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

1 participant