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
Add an option to create a DPI protected library #1490
Comments
Original Redmine Comment Will post comments about patch once get chance to review them.
Why can't we build the dpi header in the same binary run? This seems cleaner for the user.
Some of the library runtines, e.g. tracing, multithreading and some introspection will only work if there is a single Verilator runtime instance. I suggest we punt the problem of different runtime versions as a future problem to clean up. One possibility would be to make there be a stable API for the shared routines, and make all of the other runtime routines (e.g. most VL_*) be run through a pre-archive linker pass so those routines are removed from being needed in the later final link stage. |
Original Redmine Comment
It would definitely be preferable to get everything you need in one pass from the Verilator binary. Here are the options I see:
I dismissed #3 because it seems weird to create code in Verilator to do something Verilator already does. But it wouldn't be hard. What do you think about #3? Or do you see another, better way to do this?
Yeah, I don't see this as a problem that we need to solve for v0. But for my own edification, could you point out a specific thing that would't work if we just namespaced everything? I would imagine that all classes and static objects would then have the namespace as part of their symbols and not collide with other Verilators. And preprocessor stuff would be gone by the time we got to the .so. I'm not sure what the performance implications of having multiple copies of the runtime would be (I'm sure it wouldn't make things better). However, the main advantage I see to this approach is that Verilator runtime API version doesn't have to be part of the build matrix that IP vendors would have to wrangle. We've already got OS (both kernel and precision), architecture and C++ ABI. It would be nice to limit the degrees of freedom in that matrix. |
Original Redmine Comment #3 is what I was thinking. Or, ideally refactor the DPI emit code to output something closer to pure Ast, and have V3Task and the new code create that Ast code. If you namespace everything then any function in Verilated* that is a singleton will now no longer be a singleton but once per namespace. This will break stuff, e.g. tracing, threading, etc. Hence I was suggesting we separate stuff that is truly singleton-required from everything else. |
Original Redmine Comment Turns out that option #5 is to not need the DPI header at all. I'm not sure why I thought I needed it, but I don't. Either I don't understand something fundamental about the Verilator singletons or I'm not explaining my namespace thoughts well. Either way, this thread doesn't get us closer to a MVP which is what I'd like to aim for. I'll be back to beat this dead horse later once I've done some more research. I think the current branch is feature complete but currently missing testing (that's not in my external repo). I'd like to aim to land this once I have t_*pl test(s). Please let me know if you think we'll need anything else before pushing the feature. |
Original Redmine Comment To be clear, I think the MVP is feature complete (something self-contained that people could try out). There's a lot more to do for the larger feature, but I see those items being broken up into future commits. |
Original Redmine Comment In addition to a test_regress, please add an example/, then refer to it in the bin/verilator docs. Bunch of TODOs to clean up. Would be good to use "FIXME" so these can't be forgotten. Wondering why you build a shared object instead of a static library (.a)? One of the things that bothers me with V3EmitC is the hacks used while printing code, versus having a transformation that basically makes an almost pure C++ Ast, then the emitter just prints the Ast. While this is a bit more complicated it helps code reuse and allows us to add transformations later. I think we should consider having your code make an Ast that EmitV and EmitC then can print out.
Please describe what the argument does. Nitty stuff:
Please put members first (with // MEMBERS), then constructors (with // CONSTRUCTOS) then methods. Need comment on every member. (Elsewhere too).
Use a for loop? Space after "while".
<< goes on beginning of next line.
{ goes on previous line.
I suspect this isn't general enough, but the way to check is make sure your
I think we should follow standardish C library naming conventions, where the library name is a prefix on every C function name.
Split lines at the + to keep lines under ~100 chars. |
Original Redmine Comment Thanks for all the feedback.
Because both the ModelSim and XSim documentation mentions loading shared objects. I assumed that they were only able to dlopen() these things, but it seems that XSim is fine either way. I tried to test ModelSim (Intel Starter Edition) but I cannot get their crusty old compiler to work on my system. I don't know about all flavors of ModelSim, but at least for this one we'll need a separate build to be able to handle it. So in and of itself, it's not a reason to go with .so over .a. Do you know if you can link DPI static archives with ModelSim? And do you know how this works on the other major simulators? Agreed that it's nicest just to compile with the archive and not have to worry about runtime loading.
I assume this would be a structure completely separate from the main AST. Is that what you're imagining here? I'm planning to work on the tests first and then I'll come back to clean up these items. |
Original Redmine Comment I'm unaware of how Modelsim handles DPI. Was thinking the structure would be part of the AST (as everything tries to be) but under some new top leaf under AstNetlist perhaps. |
Original Redmine Comment More for my own notes than anything else: running XSim with a static DPI library is not as simple as I had earlier indicated. The simple (non-Verilator) example I used to test .a vs .so works fine. But with the dpi-compile example, XSim gives me this terribly unhelpful error message:
There are no other messages to be found. I tried to debug the simulation kernel, and while it's clearly not intended for me to invoke directly I do get this backtrace:
Which is kind of weird because it looks like we're creating the static VerilatedImp object but then while it's setting up a member deque, DPISetMode@plt() is called. However, I don't see DPISetMode in the Verilator codebase. All this is to say, I'm again unsure if static libraries will be generally usable by other simulators. It clearly works fine for Verilator itself, but other simulators will require further research. |
Original Redmine Comment More questions about the in-AST refactor: Currently "--dpi-protect secret" creates foo.sv and foo.cpp. It also slightly modifies the emitted Makefile, but I'm going to assume we're not going to represent the Makefile in the AST, so let's ignore that for now. I've attached the .cpp and .sv files that are currently generated by t_dpi_prot_secret for reference. To represent the .cpp I'll need to build up AstC* nodes that produce the same result. I thought at first I could build another top-level AstModule so that emitc() would just kick out another .cpp, but that produces a C++ class implementation which is not what I need here. Next I tried to use EmitCImp::mainDoFunc() to kick out vanilla C functions without the C++ accoutrements. This is what I have here (for reference): This ultimately doesn't work either because the EmitCImp AstCFunc visitor assumes that it is dealing with a method for a module:
I believe the both the AST nodes and EmitCImp are not abstract enough to represent/emit generic C or C++. This of course makes sense because that's not what Verilator does. But I'm also not sure if trying to create --dpi-protect artifacts in the AST is going to work unless we greatly expand the way we model C/C++ in the AST. And I don't think that's worthwhile for just this one feature. Wilson, what do you think? Am I misunderstanding something here? Given this do you think it's best to proceed without AST-ifying the --dpi-protect files? That would be my vote. |
Original Redmine Comment
Agreed, or as you say not for now...
I appreciate that the current code that prints stuff isn't that complex, but I see this is likely to become a very important feature, and new features will creep in and make it unwieldly, so making something a bit more partitioned will help us. I certainly regret V3Emit doesn't work this way now (i.e. separate transformer and dumper). More specifically maybe it would be under AstCFile, then maybe that m_modp code can also look at m_cfilep or something. Perhaps a compromise is your code makes a AstCFile/AstVFile and underneath where there's no existing appropriate Ast, uses a lot of AstText (which just says "put out this code")? That's the hack used currently where I wanted something to go out, but didn't feel worthy of making a real Ast for later processing. Then we can abstract the AstText's away to new Ast types if/when we need to. Suppose that's also how I could start cleaning up V3EmicC. Likewise for the .v it should be able to make an AstVFile and AST, and later there's a call to EmitV, rather than printing the code directly. It seems like your code has a great deal of duplication of the existing EmitV. (Including being called EmitVWrapper - but never seems to use EmitV.) |
Original Redmine Comment This is very much a WIP, but I want to make sure we're on the same page: I started moving the SV wrapper over to the AST with the AstText scheme you mentioned. I had to create a new AST node called AstTextBlock for this code to be a little more reasonable. Without it, I'd still be creating lists of input, clock and output ports and then have a bunch of code that iterates over those lists. It would have looked pretty much the same as my first pass emitter. AstTextBlock allows me to create a bunch of lists that ProtectVisitor will fill in as it goes. I believe this is far easier to read/understand/maintain. The SV wrapper is coming along and I should be able to do the same thing for the C++ wrapper. I'm proceeding along these lines assuming this is essentially what you were talking about. Please let me know if I'm mistaken. |
Original Redmine Comment Thanks, I think this looks much better & reasonable. I'd suggest using AstComment where you can instead of text with "//....\n" AstComment was printing after the comment " at ". I think your usage (no "at") will be more common so if you pull from trunk the default is now no "at" with a new option to request the "at", retrofitted to old calls. |
Original Redmine Comment I believe this is ready to go now: A couple notes/questions: Emitted comments are being indented. I believe this predates this change. For example, this:
produces this:
What is the difference between isUsedClock() and attrClocker()? I'm using the former now to detect clocks, but wasn't sure which I should use. t_dpi_prot.pl uses system() to invoke t_dpi_prot_secret.pl. The latter builds the library and the former uses it. I initially tried to do this within the same driver.pl process, but it got especially complicated when I was trying to run the sim against another simulator while building the library with Verilator. Why is "DESCRIPTION" broken up like this sometimes?
All of the other outstanding work has been recorded as issues and linked to this one. |
Original Redmine Comment Good stuff. Review comments attached. Main comment would be we should heave sensitivity lists based on what's needed only, but you already made a performance optimization bug. |
Original Redmine Comment Forgot one of your questions, it's "DESCR" "IPTION" as I have personal scripts that check every source file has a description, and it would think that this comment you are inserting is the comment for the file itself. |
Original Redmine Comment Forgot another. The generated V should pass a fingerprint of the info used to make the V to the DPI create for checking. Users are certain to otherwise mismatch versions and break things. See how the Save/Restore stuff does this. (Note if only internals change no need for fingerprint to divver as it will work - all that matters is if .v file is same - I think....) |
Original Redmine Comment ... think noe that you have AstText you can just hash those in the SV to fingerprint. |
Original Redmine Comment Thanks for all the feedback. I've got a bunch of the items cleaned up and posted to my branch. But I've got a couple questions while I work on the remaining items:
That's a good question. First off, t_dpi_prot already has this scenario, so I took a look at the VCD. The weird thing is that last_combo_time was never being updated even though its process was being evaluated. So that may be a new bug, but it's just getting in the way (eyes on the prize) so I've got a branch capturing the behavior and can come back to it later. In any case, I've been meaning to get rid of $time because I would like to minimize the amount I'm leaning on SystemVerilog to make this work with as many simulators as possible. I've instead shifted to an in-library sequence number scheme so now I've got reasonable looking last_(combo|seq)_seqnum__V values. This is up on my branch as well now. So since I'm not using $time now, it's impossible for these two values to be the same. This appears to be doing the right thing because when I:
and then:
At a given time in the simulation, the seq_update function is always called before the combo_update function. This makes sense to me because the clock edge triggers the seq_update call and updates su8_in. But su8_in doesn't update until the clock edge is complete. So this all seems fine to me, but I guess I don't know enough about how Verilator deals with clocks. Is there a notion of before (or during) and after the clock edge for a given time? If that's a reasonable way to think about it, is it ever possible for a signal to change (triggering an always process) in the before/during clock edge stage? I would think this wouldn't be possible, but again I don't really understand how that works.
Agreed that this is really low-hanging performance fruit. But I'd like to get some sort of benchmarking in place after we land the minimum feature here but before we start performance tuning. That way we'll be able to observe the effect of our changes.
I think these two comments go together, but I'm not quite sure what they mean. Is dpiProtectLibName() the same thing as the new v3Global.opt function you're suggesting? Does dpiProtectLibName() return either "libfoo.a" or "libfoo.so"? If so, I still think we want both the .a and .so called out in the comment because I could see an IP vendor shipping one .sv and multiple library files to support multiple simulators. I guess you'd have to run Verilator multiple times to do that (maybe we'll rethink that) but the .sv file should be the same every time.
I'm pretty sure you've told me how to do this before, but I can't find it. Is there a way to get emacs with verilog-mode to auto-format files for me? I'm not an emacs user, so you'll have to speak slowly. |
Original Redmine Comment
Yes.
That's a good point, perhaps the makefile by default (unless flags given) should make both versions? I don't see why two Verilator runs are then needed. Perhaps the same compile can do both (I think PIC code can be later statically linked though haven't tried.)
I'm wondering if we should rename --dpi-protect to --protect-dpi, my thinking is we might have other protection mechanisms like symbol protection (e.g. --protect-syms, --protect-key FOOBAR) and having the flags all start with --protect might be more user-helpful. |
Original Redmine Comment Thanks again. Also, regarding names, I hate naming things. And yeah, I'm already pretty sure that --dpi-protect is not ideal. I've already typed it in so many different ways that renaming would be non-trivial, but I'm fine with getting it right. However, I'm thinking now that having DPI in the name may not be a good idea either because I could imagine that one may additionally want to add PLI and FLI hooks to the library for maximum simulator support. This is more thinking long-term, but I could see wanting to expose PLI hooks and add an `ifdef in the .sv wrapper for a PLI mode for simulators that don't support the DPI (e.g. Icarus). And I'm not familiar with the FLI, but based on my cursory reading I think we'd be able to play the same trick there. So for the case where an IP vendor delivers both Verilog and VHDL, we could expose both DPI and FLI hooks in the library and additionally build a .vhdl wrapper for the FLI (V3EmitVHDL?). This would mean that the IP vendor is delivering verilated Verilog for both their Verilog and VHDL customers, but I don't see why that would matter after it's been compiled. Given all that, I'd suggest we go with something DPI/PLI/FLI agnostic. I'm going to suggest --protect-lib. Thoughts? |
Original Redmine Comment Also . . .
Isn't this because I'm using AstText everywhere which in turn uses putsNoTracking()? I think the ultimate correct solution is to represent this with non-AstText nodes in which case the indenting will happen as usual. I'd suggest we punt on the indenting until then. Are you OK with that?
Should these tests be unsupported() or _bad? I gather that the former don't actually run unless you ask them to but the latter obviously do since we're checking for expected error messages. So I'm guessing they should be _bad tests, but does this mean that eventually we should convert all unsupported() tests to _bad tests? |
Original Redmine Comment
Yeah, the only reason I have this as two separate Verilator runs now is because I was afraid of -fPIC in the static library. I would definitely prefer to have one pass through Verilator and have the Makefile kick out both flavors of library. I just tried t_dpi_prot where I compiled libsecret.a with -fPIC but still compiled the top-level module without it. This is working for me. I also tried to read up on this, but have not found a lot of great material. I did find this: Since it works for me, I'm going to proceed with -fPIC for both libraries. But what do you think? Should we just have the Makefile build all the objects with and without -fPIC? And if so, where do you think we should do that? Naively, I would just copy lines in verilated.mk like so until things start working:
But there's probably a better way to do it. |
Original Redmine Comment Also, re: testing this specific error:
I thought I could just verilate two independent modules to make this happen, but I now see I didn't understand MULTITOP works. V3EmitC aniticipates multiple modules under the root. How can I make this happen?
|
Original Redmine Comment
Like that, let's sleep on it. Is protect-library better or worse? I'm always unsure if it's better to have more reasonable lengths or more readable. You'll see a mismash of options at present.
Ah, (in theory) I think everything should work if tracking is on for these.
Generally I use _bad, and if we later support it move to good.
Suggest we go with that as it's easiest and see if that breaks anything.
I should have thought more about when made my comments. The way multitop works is there is only a single top wrapper with multiple children. I see no reason why a multitop wouldn't work fine without any changes in dpi-protect. So remove the error. (Don't see this as something you have to write a test for unless want to.) |
Original Redmine Comment I'm ambivalent towards the choice between --dpi-lib and --dpi-library but I do think one of those would be better than --dpi-protect. Re: multiple modules under the root netlist. This will definitely break --dpi-protect as it will try to create the same AstCFile and AstVFile multiple times with different contents. I'm not sure exactly what would happen, but it wouldn't be intentional. If there's no way to drive this perhaps I should remove "Unsupported" from the error message and make it a fatal/internal error instead. I'm not comfortable with removing the check entirely since this scenario can be expressed in the AST. |
Original Redmine Comment Multitop should make just one top V/Mk file, I still think this will work fine. |
Original Redmine Comment As discussed, I sliced of the XSim support and new AST nodes as two separate commits and pushed them.
The problem is in the new ProtectVisitor. It will create a C++ and SV wrapper for each module it encounters under the root node unless I add this check, which now looks like:
Multitop was a red herring because I didn't understand how it worked. But you can still have multiple modules under the root node. I don't know if it's possible to get Verilator to produce such an AST, but it is representable in the AST so I'd like to protect against it. Also, let me know what you think about --protect-lib vs --protect-library. I'm presenting this at ORConf on Sunday and it would be good if I got the name of the option right. The only major chunk of work remaining is getting rid of my bespoke type construction/data copying logic. I'm trying to figure out how to refactor some of the V3Task code so that it will work with both the fully semantic AST construction and the dumb-AstText nodes we're using in V3DpiProtect. |
Original Redmine Comment Since you're neutral on lib vs library, if we use --protect-lib I believe your paper/this feature will be top google hit for "protect-lib" so let's use that. Multitop has only a single module under root (the wrapper). The multiples are a layer lower, hence why I think this will work. An assertion (which could be internal and so untested) for your worry is reasonable:
|
Original Redmine Comment I believe this covers all the feedback: Also, per some of the other threads going on I renamed the example directory to examples/make_protect_lib and I made --protect-lib imply --protect-ids. |
Original Redmine Comment In this
Fix to "examples/make_protect_lib"
die may kill the harness not just this test. Use run(cmd => $cmd); Though it's probably cleaner to put everything inside a while loop similar run(cmd => $cmd);
To match protect_ids, suggest rename variables etc inside the secret module Suggest use '--protect-key SECRET_FAKE_KEY' so the test is run-to-run stable. With these you may squash and push when ready. Note there's a potential collision with between the protect (#�) and cmake |
Original Redmine Comment Squashed and pushed. Agreed that we can fix up --protect-lib under CMake when it's all in the trunk. |
Original Redmine Comment In 4.022. |
Author Name: Todd Strader (@toddstrader)
Original Redmine Issue: 1490 from https://www.veripool.org
Original Assignee: Todd Strader (@toddstrader)
Jumping off from here:
https://www.veripool.org/boards/3/topics/3037
I've got a WIP branch to show where I'm going:
https://github.com/toddstrader/verilator-dev/tree/dpi-compile
This adds a --dpi-protect option to Verilator which causes it to emit the wrapper SV and C++ files needed to build the library. It also modifies the emitted Makefile to build the library. Presently, the generated library demonstrates basic functionality when simulated with both Verilator and XSim.
Currently, I'm using my proof-of-concept codebase to test --dpi-protect:
https://github.com/toddstrader/dpi-compile
General comments are welcomed, but I do have a couple specific questions:
To build the library, I need to run Verilator again on the wrapper SV file to get the DPI header. I'd like this to be as push-button as possible, so I'm planning to have the Verilator Perl wrapper call the binary again when run with --dpi-protect to build the DPI header. Alternatively, I could add a step to the emitted Makefile to do this, but I think the former is simpler. Thoughts? I also plan to add (via a separate issue) a mode to only emit the DPI header.
Since the Verilator runtime objects need to be linked against this library for it to do anything, we need to handle incompatibilities between different versions of Verilator. I don't believe that code was ever intended to have a stable API, so I think the safest option for now is to wrap each library's (one may have multiple) Verilator runtime in its own namespace. Thoughts? Is there a better way to handle this?
The branch needs a lot more testing and polishing, but I'd like to focus on getting a minimum viable feature in the trunk before adding a ton of additional features (e.g. symbol hashing).
The text was updated successfully, but these errors were encountered: