Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-27T23:07:08Z

Will post comments about patch once get chance to review them.

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.

Why can't we build the dpi header in the same binary run? This seems cleaner for the user.

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?

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-08-28T00:03:41Z

Why can't we build the dpi header in the same binary run? This seems cleaner for the user.

It would definitely be preferable to get everything you need in one pass from the Verilator binary. Here are the options I see:

1. Run the binary a second time in the Perl wrapper
2. Run the binary a second time from the generated Makefile
3. Add another emitter in V3EmitDpiProtect.cpp similar to EmitCSyms::emitDpiHdr()
4. Create a new AST and run it through the visitor in V3EmitCSyms.cpp to create the DPI header (this one seems crazy)

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?

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.

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-28T00:07:40Z

#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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-08-30T09:59:17Z

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-08-30T11:41:19Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-31T00:12:55Z

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.

+=item --dpi-protect I<name>

Please describe what the argument does.

Nitty stuff:

+  protected:
+    // TODO -- inouts?
+    typedef std::list<AstVar*> VarList;
+    VarList m_inputs;
+    VarList m_outputs;
+    int m_totalPorts;
+    AstNodeModule* m_modp;
+    string m_libName;

Please put members first (with // MEMBERS), then constructors (with // CONSTRUCTOS) then methods.

Need comment on every member. (Elsewhere too).

+  private:
+    void discoverPorts() {

+        AstNode* stmtp = m_modp->stmtsp();
+        while(stmtp) {
...
+            stmtp = stmtp->nextp();

Use a for loop? Space after "while".

+                    varp->v3fatalSrc("Unsupported direction for --dpi-protect: "<<
+                                     varp->direction().ascii());

<< goes on beginning of next line.

+class EmitVWrapper: public EmitWrapper {
+  public:
+    EmitVWrapper(AstNodeModule* modp):
+        EmitWrapper(modp),
+        m_of(v3Global.opt.makeDir()+"/"+m_libName+".sv")
+    {

{ goes on previous line.

+    void emitDpiParameters(VarList& ports) {
...
+            if (varp->direction() == VDirection::INPUT) {
+                if ((width = varp->width()) > 1) {
+                    m_of.puts("const svBitVecVal* ");
+                } else {
+                    m_of.puts("unsigned char ");

I suspect this isn't general enough, but the way to check is make sure your
test tries every supported data type.

+        m_of.puts("void* create_dpi_prot_"+m_libName+" (const char* scope) {\n");

I think we should follow standardish C library naming conventions, where the library name is a prefix on every C function name.

+            of.puts("lib"+v3Global.opt.dpiProtect()+".so: $(VM_PREFIX)__ALL.a $(VK_GLOBAL_OBJS)\n");
+            of.puts("\t$(OBJCACHE) $(CXX) $(CXXFLAGS) $(CPPFLAGS) $(OPT_FAST) -shared -o $@ "+v3Global.opt.dpiProtect()+".cpp $^\n");

Split lines at the + to keep lines under ~100 chars.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-03T11:00:47Z

Thanks for all the feedback.

Wondering why you build a shared object instead of a static library (.a)?

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 think we should consider having your code make an Ast that EmitV and EmitC then can print out.

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-03T11:05:15Z

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-04T13:55:28Z

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:

ERROR: [Simtcl 6-50] Simulation engine failed to start: Simulation exited with status code 1.
Please see the Tcl Console or the Messages for details.

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:

(gdb) bt                                                                                                                      
#0  0x0000555555556970 in DPISetMode@plt ()
#1  0x000055555556bea2 in __gnu_cxx::new_allocator<unsigned int*>::allocate(unsigned long, void const*) ()    
#2  0x000055555556b8b7 in std::allocator_traits<std::allocator<unsigned int*> >::allocate(std::allocator<unsigned int*>&, unsigned long) ()
#3  0x000055555556a5ec in std::_Deque_base<unsigned int, std::allocator<unsigned int> >::_M_allocate_map(unsigned long) ()
#4  0x0000555555568534 in std::_Deque_base<unsigned int, std::allocator<unsigned int> >::_M_initialize_map(unsigned long) ()
#5  0x0000555555566ec8 in std::_Deque_base<unsigned int, std::allocator<unsigned int> >::_Deque_base() ()
#6  0x0000555555565c80 in std::deque<unsigned int, std::allocator<unsigned int> >::deque() ()
#7  0x0000555555564619 in VerilatedImp::VerilatedImp() ()       
#8  0x0000555555563cf0 in __static_initialization_and_destruction_0(int, int) ()                             
#9  0x0000555555563d26 in _GLOBAL__sub_I_verilated.cpp ()
#10 0x000055555556c92d in __libc_csu_init ()
#11 0x00007ffff6ed9b28 in __libc_start_main (main=0x555555557e30 <main>, argc=1, argv=0x7fffffffe0b8, init=0x55555556c8e0 <__libc_csu_init>, fini=<optimized out>, rtld_fini=<optimized out>,
     stack_end=0x7fffffffe0a8) at ../csu/libc-start.c:266
#12 0x000055555555718a in _start ()    

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-20T13:30:25Z

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):
https://github.com/toddstrader/verilator-dev/tree/dpi-compile-ast

This ultimately doesn't work either because the EmitCImp AstCFunc visitor assumes that it is dealing with a method for a module:

         puts(modClassName(m_modp)+"::"+nodep->name()
              +"("+cFuncArgs(nodep)+") {\n");

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-21T13:24:49Z

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.

Agreed, or as you say not for now...

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.

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-23T18:35:02Z

This is very much a WIP, but I want to make sure we're on the same page:
https://github.com/toddstrader/verilator-dev/tree/dpi-compile-ast2

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-23T22:44:52Z

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-24T16:05:07Z

I believe this is ready to go now:
https://github.com/toddstrader/verilator-dev/tree/dpi-compile-ast2

A couple notes/questions:

Emitted comments are being indented. I believe this predates this change. For example, this:

     2:1: TEXTBLOCK 0x55ee1288e260 <e3835#> {d5} ""
     2:1:1: COMMENT 0x55ee1288e340 <e3751#> {d5}  Wrapper module for DPI protected library
     2:1:1: COMMENT 0x55ee1288e420 <e3753#> {d5}  This module requires libsecret.a or libsecret.so to work
     2:1:1: COMMENT 0x55ee1288e500 <e3755#> {d5}  See instructions in your simulator for how to use DPI libraries\n

produces this:

// Wrapper module for DPI protected library
     // This module requires libsecret.a or libsecret.so to work
         // See instructions in your simulator for how to use DPI libraries

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?

of.puts("// DESCR" "IPTION: Verilator generated C++\n");

All of the other outstanding work has been recorded as issues and linked to this one.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-24T23:51:44Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-24T23:53:08Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-25T00:09:44Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-25T00:15:38Z

... think noe that you have AstText you can just hash those in the SV to fingerprint.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-25T20:21:47Z

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:

•    m_seqAssignsp = new AstTextBlock(fl, "if (last_seq_time > "
  

•                                     "last_combo_time) begin\n");
  

Does this work if you get combo & seq events in same timestep?

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:

always @(posedge clk) begin
     su8_in <= {8{crc}};

and then:

always (*) begin
     last_combo_seqnum__V = secret_dpiprotect_combo_update(
         handle__V,
         su8_in,

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.

Main comment would be we should heave sensitivity lists based on what's needed only, but you already made a performance optimization bug.

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.

•    addComment(txtp, fl, "This module requires lib"+m_libName+
  

•               ".a or lib"+m_libName+".so to work");
  

Might be nicer to add and use a new v3Global.opt function

+++ b/src/V3EmitMk.cpp

•        if (v3Global.opt.dpiProtectShared()) {
  

•            of.puts("default: lib"+v3Global.opt.dpiProtect()+".so\n");
  

•        } else {
  

•            of.puts("default: lib"+v3Global.opt.dpiProtect()+".a\n");
  

Use dpiProtectLibName() suggested above.

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.

Please fix all your .v files to be verilog-mode out-of-the-box indentater
clean.

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-26T01:41:17Z

Is dpiProtectLibName() the same thing as the new v3Global.opt function you're suggesting? Does >dpiProtectLibName() return either "libfoo.a" or "libfoo.so"?

Yes.

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.

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.)

... how to indent:

emacs --batch  <filenames.v>  -f verilog-batch-indent

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-26T04:25:02Z

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-26T04:51:51Z

Also . . .

The generated code seems wrongly indented (not indented) for some reason.

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?

If you have a runtime error, then please add a test for it also, as trying
to keep the C++ line coverage good.

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-26T07:34:09Z

perhaps the makefile by default should make both versions?

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:
https://stackoverflow.com/questions/37842036/why-does-including-fpic-to-compile-a-static-library-cause-a-segmentation-fault
But the OP doesn't elaborate much on what happened and the response doesn't really explain why you can't use -fPIC outside of a shared library. I basically says "don't do that".

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:

%.o: %.cpp
         $(OBJCACHE) $(CXX) $(CXXFLAGS) $(CPPFLAGS) $(OPT_FAST) -c -o $@ $<
%.pic.o: %.cpp
         $(OBJCACHE) $(CXX) $(CXXFLAGS) $(CPPFLAGS) $(OPT_FAST) -c -fPIC -o $@ $<

But there's probably a better way to do it.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-26T11:17:32Z

Also, re: testing this specific error:

     +        if (m_modProtected)
     +            v3Global.rootp()->v3error("Unsupported: --dpi-protect with multiple"
     +                                      " top-level modules");

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?

void V3EmitC::emitc() {
     UINFO(2,__FUNCTION__<<": "<<endl);
     // Process each module in turn
     for (AstNodeModule* nodep = v3Global.rootp()->modulesp();

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-26T12:12:04Z

I'm going to suggest --protect-lib.

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.

The generated code seems wrongly indented (not indented) for some reason.
Isn't this because I'm using AstText everywhere which in turn uses putsNoTracking()?

Ah, (in theory) I think everything should work if tracking is on for these.

If you have a runtime error, then please add a test for it also, as trying to keep the C++ line coverage good.
Should these tests be unsupported() or _bad?

Generally I use _bad, and if we later support it move to good.

perhaps the makefile by default should make both versions?

I'm going to proceed with -fPIC for both libraries. But what do you think?

Suggest we go with that as it's easiest and see if that breaks anything.

Also, re: testing this specific error:

•        v3Global.rootp()->v3error("Unsupported: --dpi-protect with multiple" 
  

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
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-26T20:18:34Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-27T03:36:10Z

Multitop should make just one top V/Mk file, I still think this will work fine.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-09-27T08:37:30Z

As discussed, I sliced of the XSim support and new AST nodes as two separate commits and pushed them.

Multitop should make just one top V/Mk file, I still think this will work fine

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:

         if (m_modProtected) {
             v3Global.rootp()->v3error("Unsupported: --dpi-protect with multiple"
                                       " top-level modules");
         }

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-27T12:36:11Z

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:

 UASSERTOBJ(!v3Global.rootp()->modp()->nextp(), "Multiple root modules");

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-10-08T14:15:44Z

I believe this covers all the feedback:
https://github.com/toddstrader/verilator-dev/tree/protect-lib

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-08T22:15:25Z

In this

 +++ b/bin/verilator
 +encrypted RTL (i.e. IEEE P1735).  See examples/protect_lib in the
 +distribution for a demonstration of how to build and use the DPI library.

Fix to "examples/make_protect_lib"

 +++ b/test_regress/t/t_prot_lib.pl
 +die "Could not build secret library" if run(cmd => $cmd);
 +compile(

die may kill the harness not just this test. Use

run(cmd => $cmd);
if (!$Self->{errors}) {
compile(

Though it's probably cleaner to put everything inside a while loop similar
to t_flag_csplit.pl and at the point where you die instead:

run(cmd => $cmd);
last if (!$Self->{errors});

• but we can't see what's inside

• file_grep_not("$Self->{obj_dir}/simx.vcd", qr/secret_value/);

To match protect_ids, suggest rename variables etc inside the secret module
to have secret_ prefix, then this can have stronger test for qr/secret_/i.

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
feature (#�), with you both almost ready to merge. I propose we merge
both and we cleanup "--make cmake --protect-lib" in a later patch set.
I'll file a bug once both hit master.

Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-10-09T10:54:15Z

Squashed and pushed. Agreed that we can fix up --protect-lib under CMake when it's all in the trunk.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-10T19:28:49Z

In 4.022.