verilator / verilator

Verilator open-source SystemVerilog simulator and lint system
https://verilator.org
GNU Lesser General Public License v3.0
2.55k stars 611 forks source link

Missing include to compile VL_TO_STRING #3930

Closed toddstrader closed 1 year ago

toddstrader commented 1 year ago

I'm starting a new thread as this is a separate topic, but this relates back to my comments about t_class_forward in #3870.

An implementation file for ClsB is missing a VL_TO_STRING definition for ClsA. I'm wondering where the missing #include should go. I could drop it in the implementation file (called Vt_class_forward_P__03a__03aClsB__Vclpkg__DepSet_h1963a8f3__0.cpp at least for me). However, that appears to come from EmitCImp::emitCommonImp():

            std::set<string> headers;
            headers.insert(prefixNameProtect(m_fileModp));
            headers.insert(symClassName());

            openNextOutputFile(headers, "");

which doesn't seem to invite adding other header files for things which may be used in this implementation file.

Should I add it in ClsB's header? That seems more than necessary since the header isn't what is trying to use a ClsA function. But it would clearly fix the problem too.

toddstrader commented 1 year ago

Actually, I think I see now. EmitCBaseVisitor::emitModCUse() can kick out both an include and a forward class declaration. For reasons which I haven't determined yet, isInclude() is false so we don't get the #include we need here.

toddstrader commented 1 year ago

After some more digging, I see I'm missing a CUse of type IMP_INCLUDE or INT_INCLUDE in ClsB and ClsA for each other. One problem with that is that (at least in this example) Vt_class_forward_P__03a__03aClsB is defined in Vt_class_forward_P__03a__03aClsB__Vclpkg.h. I'm not sure if this __Vclpkg pattern will always be followed. Also, the use of VL_TO_STRING for ClsA inside of ClsB is just Text by the time we get to it with V3CUse:

    1:2:3: CSTMT 0x155554fae4d0 <e1911> {d13ab}
    1:2:3:1: TEXT 0x155554fac620 <e1910> {d13ab} "out += ", a:" + VL_TO_STRING(__PVT__a);..."

and I assume we don't want V3CUse to start parsing text to see what's going on.

I also tried an overly aggressive solution of always including instead of forward declaring classes. The problem with that is the combination of Vt_class_forward_P__03a__03aClsB__Vclpkg and Vt_class_forward_P__03a__03aClsB (for example). I don't know how to go from a class name to its header file, which means I can't even try this approach.

I'm still looking, but any thoughts or advice will be appreciated.

toddstrader commented 1 year ago

I'm also noticing that nothing actually uses VL_TO_STRING for these classes (except for the loop I'm trying to resolve). Presumably something, somewhere actually cares about classes having this function? Or is it possible to just not give classes a VL_TO_STRING?

wsnyder commented 1 year ago

All classes and data types need a VL_TO_STRING so that e.g. $sformatf("%p", object) works. Add that to an appropriate test and you should see errors if any VL_TO_STRINGS are missing implementations. All class headers should declare VL_TO_STRING. The implementation should then be in the implementation file. The VL_TO_STRING should call any member's VL_TO_STRING, so that the implementation only needed to include the class's members' header files.

wsnyder commented 1 year ago

@toddstrader did you have a sharable test showing this behavior and/or a pull request to fix it?

toddstrader commented 1 year ago

@wsnyder I found this by working on #3870. I don't know how to demonstrate it without that verilator_includer Makefile change. While I do think this is a real issue, #3870 is purely a function of heavy public usage (for VPI access). Given that, this is starting to feel like we're going down a rabbit hole.

@AndrewNolte and I have been talking and we think it's probably easier (and arguably cleaner) to resolve the original problem by adding a --no-public argument as the place where we're running into the problem with the single-machine build does not care about the VPI so it should be fine if we pretended we didn't have any public signals. If this makes sense to you I can close out these two threads in favor of that new effort.