Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  BugVise
  CovVise
  Force-Gate-Sim
  Gspice
  IPC::Locker
  Rsvn
  SVN::S4
  Voneline
  WFH
General Info
  Papers

Issue #1305

Error messages do not contain hierarchical information

Added by Todd Strader over 1 year ago. Updated 12 days ago.

Status:
Confirmed
Priority:
Normal
Assignee:
Category:
Lint
% Done:

0%


Description

Please see the t_func_const2_bad test in: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace

I currently get this error message:
%Error: t/t_func_const2_bad.v:21: Expecting expression to be constant, but can't determine constant for FUNCREF 'f_add2'

Which gives me the line number, but not the hierarchical location of the problem. The error occurs in module 'c9', however there are multiple instances of 'c9'. It would be handy if the error message told me that the location of the problem was in 't.b8_a7.c9'.

I suspect that the most useful time to get this information would be during V3Const. However, I would ideally like to see it whenever possible.

I don't see any simple way to add this, but I could obviously be missing something. Is there a way to ask a node if it has a scope and what it is? If so, this additional reporting could be added to v3error(). If not, I could collect this information while traversing the AST for V3Const and add it to the error message when needed.

Any thoughts on how to proceed would be appreciated.

last.tree (43.6 KB) Todd Strader, 05/03/2018 06:03 PM

History

#1 Updated by Wilson Snyder over 1 year ago

  • Category set to Lint
  • Status changed from New to Confirmed

I can see how that is useful.

In theory try from the nodep causing the error to follow backp() looking for any AstScope's. Note these only exist at some phases, elsewhere it would be a mess and non-general.

#2 Updated by Todd Strader about 1 year ago

I dumped the tree (attached) at the time of the error and I also traced the backp()'s. Here is the latter:
FUNCREF 0x161c0d0 <e714#> {e21} @dt=0x160a990@(sw32)  f_add2 pkg=0x160a760 -> FUNC 0x160aa70 <e497#> {e13} @dt=0x160a990@(sw32)  f_add2
VAR 0x161bec0 <e715#> {e21} u5=0x1 @dt=0x15da5f0@(G/sw32)  SOMEP LPARAM
VAR 0x161bb90 <e708#> {e19} u4=0x42 u5=0x1 @dt=0x1609b60@(G/swu32/4)  B GPARAM
VAR 0x161b860 <e704#> {e18} u4=0x41 u5=0x1 @dt=0x1610750@(G/swu32/3)  A GPARAM
MODULE 0x161b750 <e606#> {e17} u5=0x1  c9__A7_B8  L4
MODULE 0x160dad0 <e332> {e17}  c9  L4
MODULE 0x1614e90 <e525#> {e25} u5=0x1  b8__A6  L3
MODULE 0x15db170 <e534#> {e25} u5=0x1  b8__A7  L3
MODULE 0x160f0b0 <e331> {e25} u5=0x1  b8  L3
PACKAGE 0x160a760 <e330> {a0}  __024unit  L3 [LIB]
MODULE 0x1610300 <e326> {e35} u5=0x1  t  L2
NETLIST 0x15c9c20 <e1> {a0}
I only see one SCOPENAME in the whole AST, which is somehow attached to the $fatal():
1:2:3:2: DISPLAY 0x160a2c0 <e81> {e9}
1:2:3:2:1: SFORMATF 0x160a380 <e87> {e9} u3=0x161d9b0 @dt=0x160a460@(G/str)  f_add = 15
1:2:3:2:1:2: SCOPENAME 0x15d99e0 <e400> {e9} @dt=0x15d8530@(G/w64)
1:2:3:2: STOP 0x160a590 <e89> {e9}

I don't think that is useful.

What I really seem to want is to be able to trace the CELL -> MODULE linkage. Starting here:
1: MODULE 0x161b750 <e606#> {e17} u5=0x1  c9__A7_B8  L4
Find a cell that points to the module:
1:2: CELL 0x16158f0 <e242> {e31}  c9 -> MODULE 0x161b750 <e606#> {e17} u5=0x1  c9__A7_B8  L4
Use backp() to find the parent module, repeat until you find the top:
1: MODULE 0x15db170 <e534#> {e25} u5=0x1  b8__A7  L3
1:2: CELL 0x1613080 <e311> {e43}  b8_a7 -> MODULE 0x15db170 <e534#> {e25} u5=0x1  b8__A7  L3
1: MODULE 0x1610300 <e326> {e35} u5=0x1  t  L2
 NETLIST 0x15c9c20 <e1> {a0}

Of course, this means I need to search for the CELL -> MODULE links because I don't think there are back pointers for them. I believe we can limit the searching by following backp() until we find modules and then look in the module for cells that we're searching for. I'm game if you don't think this is a crazy idea. Beyond that, we could add the module stack information to this particular stage (V3Const) because I think it's most likely that this information would be handy at this stage. But I'd prefer a more generic solution.

I also realized that there's not always going to be one right answer for what the hierarchical location of the error is since a module with a problem can be instantiated multiple times. However, I still think it's beneficial to have even one of the offending hierarchical paths.

Thoughts? I'll probably try the CELL -> MODULE reverse searching next, but if there's a better way I'd love to hear it.

#3 Updated by Wilson Snyder about 1 year ago

That there are cells means the design hasn't yet been flattened. Your find-a-cell approach will only work until that happens (in V3LinkDot), then the scope scheme mentioned originally should work ok. FWIW I want to long term move to more processing happening after flattening because this will solve several internal issues and some bugs related to the late elaboration of the current method.

#4 Updated by Todd Strader 3 months ago

I've posted a proposed change for this here: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace

One issue is that V3Number does not know what AstNode it is in. So instead of calling AstNode::v3errorEnd(), it just calls FileLine::v3errorEnd(). I propose that instead of constructing a V3Number with a FileLine*, we use an AstNode* instead. That would give us the ability to dump out module hierarchy for V3Number errors/warnings as well.

Questions:
  • Are there other things like V3Number that toss errors/warnings that don't have an AstNode to report from? I took a look and I think V3Number is the only thing that does this and could have AstNode information.
  • Is my proposal for V3Number OK? If so, I'll go ahead and do that.

#5 Updated by Wilson Snyder 3 months ago

V3Number started long ago as a relatively independent thing without concept of nodes, but don't see a reason it has to remain that way. Go for it.

(Long term it might make sense for numbers to be a node that is all of V3Number operates on AstConst like nodes. Work for later...)

#6 Updated by Todd Strader 3 months ago

I've posted the latest changes here: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace

This now associates V3Number with AstNode so that we can get the module hierarchy in errors/warnings coming from within V3Number (see t/t_func_const3_bad). It also handles the post-flattening cases. VarScope does hold the information we want, but it isn't in the backp path. VarRefs do point back to VarScopes, but the VarRefs also seem to have a fully expanded name at this point, so I just grabbed that. Please let me know if you see any issues with this approach.

I also have a question around verilog.l. V3ParseImp::newNumber() uses a FileLine to create a V3Number. This gets called three times in verilog.l (well, the code that that produces). Because I don't fully understand what's going on here, I've made V3Number able to be constructed with either an AstNode or a FileLine (for this case). If we fall back on the latter, then we don't get module hierarchy information in the error messages. I'd prefer not to have this dichotomy.

If I'm reading this code correctly, yylval (V3ParseBisonYYSType) here only holds a FileLine and V3Number. I don't know what's going on with the scp field, but the comment says it's for symbol table scope so I don't think that would be useful anyway. At this point in the lexer, is there an AstNode I can associate with this V3Number? Or should I just leave it as-is?

Beyond all that, it would also be nice to have generate names in the pre-flattened case. As it is now, we only get the module in question. I'll open a new issue for that as I'd like to get this in before the diff gets too large.

And a final question: do you think we should format the hierarchy information differently? Riviera does something like so, "Instance: /foo/bar". I think the dots are more Verilogy but perhaps we should have a prefix before this information. Thoughts?

#7 Updated by Todd Strader 3 months ago

Another question: it seems that there are quite a few _bad tests that are still using expect regexes instead of expect_filename. Do you have any objection to converting everything to golden files?

#8 Updated by Wilson Snyder 3 months ago

Nodes don't exist in the lexer. I suppose we cold make fake temp nodes, but don't like this as some alternative future parsers build ast trees themselves and probably will choke, and seems strange. There is also no hierarchy existing then, so will not affect reporting.

I think we sould use dots as a seperator as that is closest to the std.

msg7: I think the remaing expects were left to use regexps to avoid needing updates on minor test or output differences. In general if one breaks you should be able to convert it. Please do this in a seperate patch/branch so it is easier to isolate what your big patch does. (That is your big patch will just update the gilden file).

#9 Updated by Todd Strader 3 months ago

I seem to have worked myself into a corner and therefore have backtracked a bit. I believe/hope this branch is ready to go upstream: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_2

The problem with the approach I was taking in #6 is that since V3Number is not an AstNode it is not properly handled when destroying/cloning nodes. I tried to resolve this issue in this half-baked branch that I'm posting for discussion purposes only: https://github.com/toddstrader/verilator-dev/tree/mst_astnode_numlist_wip

Since I was running into all sorts of problems with that approach and since you mentioned that long term you wanted V3Number to be an AstNode I switched gears and am now grabbing module hierarchy information when the V3Number is constructed. This is clearly a little more expensive while verilating but has the advantage of working.

Please let me know what you think.

#10 Updated by Wilson Snyder 3 months ago

I'm finding this is a little massive, can you peel off the V3Number-node stuff into a subpatch I can review/apply first? (That is doesn't do location reporting).

Also another separate patch to do the expect_filenames. Should be able to roughly take your current patch, then revert sources, then rerun with --golden and you'll end up with the patch that just adds the expects.

I'm not very in love with the error format

%Error: t/t_array_backw_index_bad.v:13: (Location: t) Slice selection '[1:3]' has backward indexing versus data type's '[3:0]'

How do other simulators report this? Should it be Scope instead of Location?

#11 Updated by Wilson Snyder 3 months ago

Oh, also please merge from upstream; I merged your MANIFEST.SKIP change.

#12 Updated by Todd Strader 3 months ago

I'm juggling git commits now. In the meantime, I've got this to survey other tools' error messages: https://www.edaplayground.com/x/2JfQ

I was copying Riviera's message format (except with dots instead of slashes):
# KERNEL: Time: 0 ns,  Iteration: 0,  Instance: /t/foo_inst,  Process: @INITIAL#3_0@.
VCS doesn't have a prefix and uses dots:
Fatal: "design.sv", 3: t.foo_inst: at time 0 ns
And ncsim just tosses it out like some kind of non sequitur:
ncsim: *F,FATSEV (./design.sv,3): (time 0 FS).
t.foo_inst

I don't really care what the format is, but if I had to choose I would pick what I've already got because the 'Location:' prefix tells you what you are looking at. If you have a different preference I can do that instead.

Please let me know what you think.

#13 Updated by Todd Strader 3 months ago

The V3Number refactor is isolated here: https://github.com/toddstrader/verilator-dev/tree/v3number_hier

And the golden file change is here: https://github.com/toddstrader/verilator-dev/tree/mst_golden_files

And here is the full solution based on the first two patches: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_3

Once we land the first two, I'll rebase the third one.

#14 Updated by Wilson Snyder 3 months ago

Golden file is committed.

V3Number comments:

+ AstConst(FileLine* fl, V3NumberConstructor)

I could puzzle it through, but why is V3NumberConstructor needed?

+V3Number::V3Number(AstNode* nodep, const char* sourcep, FileLine* fl) {

Isn't the idea a FileLine have always EITHER a nodep or a fl? That is one or the other, and if nodep is used then fileline is implied. So the constructor could be this

class NoNode; V3Number::V3Number(NoNode, const char* sourcep, FileLine* fl) {

the idea is you call it with NoNode() to indicate it's the strange FileLine only case (to make it more obvious a node is the common case)

Then have an accessor

FileLine* fileline() const { return m_nodep ? m_nodep->fileline() : m_fileline; }

and I think you don't need setNames.

-string V3Number::displayed(FileLine*fl, const string& vformat) const { +string V3Number::displayed(const string& vformat) const {

I believe displayed takes a fileline because the constant that is being passed in may have a different fileline than the $display's fileline. I would think the right thing is to have displayed take a nodep input (similar to how it used to take a FileLine).

+ string m_hierName; // Module hierarchy for errors/warnings

I'm perhaps missing something but think m_hierName is never set to anything non-empty. I also don't understand why this would be needed. I would think we'd have this

#define v3NumberError(msg) { if (m_nodep) m_nodep->v3error(msg); else m_fileline->v3error(msg); }

the idea is most things have node and report on that, some numbers don't and have a fileline they report.

Random nit: Use NULL not nullptr as (yet) C++11 is not required.

#15 Updated by Todd Strader 3 months ago

I've updated this branch based on the items below: https://github.com/toddstrader/verilator-dev/tree/v3number_hier

+ AstConst(FileLine* fl, V3NumberConstructor)

I could puzzle it through, but why is V3NumberConstructor needed?

V3NumberConstructor is used wherever there used to be a V3Number constructed and then passed into an AstConst constructor. Now, the V3Number construction happens in the AstConst constructor so it can pass this to the V3Number constructor. e.g.:

- V3Number n(V3Number::VerilogStringLiteral(), fl, v);
- return new AstConst(fl,n);
+ return new AstConst(fl,AstConst::V3NumberConstructor(),V3Number::VerilogStringLiteral(),v);

All the different AstConst constructor flavors with V3NumberConstructor are just the different flavors of V3Number constructors. I thought V3NumberConstructor would make that clearer and it also prevents a signature collision with an existing AstConst constructor. Please let me know if you think there's a cleaner way to do this.

+V3Number::V3Number(AstNode* nodep, const char* sourcep, FileLine* fl) {

Isn't the idea a FileLine have always EITHER a nodep or a fl? That is one or the other, and if nodep is used then fileline is implied. So the constructor could be this

You're right. It should only ever be one or the other. I broke this up into the two constructors it really should have been.

The FileLine-only version of the constructor is only called once in V3ParseImp::newNumber(). This gets back to my bison question in msg#6.

and I think you don't need setNames.

I have setNames because this happens in both the constructors and in V3Number::nodep() which used to be fileline(). This is only called in SimulateVisitor::allocNumber() where there's some V3Number reuse going on so the node needs to be changed on the fly.

I believe displayed takes a fileline because the constant that is being passed in may have a different fileline than the $display's fileline. I would think the right thing is to have displayed take a nodep input (similar to how it used to take a FileLine).

This is true. However, the only two exceptions that displayed() can toss basically say that the number doesn't work with the display format. This switches the error reference from the format to the number. Both ways seem right to me, but I've changed this back to the way it was.

I'm perhaps missing something but think m_hierName is never set to anything non-empty. I also don't understand why this would be needed. I would think we'd have this

Sorry, there's a note about this in the commit message:

NB: m_hierName is a placeholder for issue #1305

You can see how it is used in the mod_stack_trace_3 branch: https://github.com/toddstrader/verilator-dev/blob/mod_stack_trace_3/src/V3Number.cpp#L277

It is currently benign and speaks of things to come.

#define v3NumberError(msg) { if (m_nodep) m_nodep->v3error(msg); else m_fileline->v3error(msg); }

This is what my first attempt was (see the intermediate commits). This failed because of the problems I mentioned in msg#9. As it is now, AstNodes are only going to be used to populate m_fileline and (eventually) m_hierName. You'll notice I removed the AstNode* member from V3Number because of this.

Random nit: Use NULL not nullptr as (yet) C++11 is not required.

Done.

#16 Updated by Wilson Snyder 3 months ago

All the different AstConst constructor flavors with V3NumberConstructor are just the

different flavors of V3Number constructors. I thought V3NumberConstructor would make that clearer and it also prevents a signature collision with an existing AstConst constructor. Please let me know if you think there's a cleaner way to do this.

I commented them out to find the flavors that are used. So how's the below look instead? I think this more closely matches how V3Const takes other arguments, hiding V3Number details.

class WidthedValue {};  // for creator type-overload selection
AstConst(FileLine* fl, WidthedValue, int width, uint32_t value)
    : AstNodeMath(fl)
    , m_num(this, width, value) {
    initWithNumber();
}
class StringToParse {};  // for creator type-overload selection
AstConst(FileLine* fl, StringToParse, const char* sourcep)
    : AstNodeMath(fl)
    , m_num(this, sourcep) {
    initWithNumber();
}
typedef VerilogStringLiteral {};  // for creator type-overload selection
AstConst(FileLine* fl, VerilogStringLiteral, const string& str)
    : AstNodeMath(fl)
    , m_num(V3Number::VerilogStringLiteral(), this, str) {
    initWithNumber();
}

I got rid of needing two of the flavors with the new code put in master; you'll need to merge (and reconcile conflicts.)

I believe displayed takes a fileline because the constant that is being passed in may have a different fileline than the $display's fileline. I would think the right thing is to have displayed take a nodep input (similar to how it used to take a FileLine).

...I've changed this back to the way it was.

But then won't you (eventually) need the proper hierName from the callee so you can nodep->v3error? Thus I think you'd want to pass a errNodep down.

I'm perhaps missing something but think m_hierName is never set to anything non-empty. ...

It is currently benign and speaks of things to come.

If it's not used in this commit I'd be inclined to not include it in this patch, and add it to next patch set.

#define v3NumberError(msg) { if (m_nodep) m_nodep->v3error(msg); else m_fileline->v3error(msg); }

Thinking about this further I suggest it's cleaner to add:

void V3Number::v3errorEnd(std::ostringstream& str) {
   std::ostringstream nsstr;
   nsstr<&lt;m_hierName<&lt;str.str();
   m_fileline->v3errorEnd(nsstr);
}

then the normal m_fileline->v3error calls can be used in V3Number and they will call into v3errorEnds and you can get rid of the specialized v3NumberError macros, which pain me.

Another alternative is to add a FileLineHier class with a FileLine and string in it, and changing V3Number to use that.... Then V3Number reports errors with just m_filelinehier->v3error(...) as the v3errorEnd would be part of that class. Not sure if that is a win or not.

Note when I looked at how this works I now realize that V3Graph also reports errors, but seems to be only assertions so should be OK as is.

#17 Updated by Todd Strader 3 months ago

I've rebased onto master and taken care of the above: https://github.com/toddstrader/verilator-dev/tree/v3number_hier_2

One note about displayed(): You're right that I should have been passing in an AstNode* so that the error message could get both the FileLine and hierName. However, that doesn't work when displayed() is called within V3Number since there is no AstNode because of msg#9. I have it working for both cases now.

#18 Updated by Wilson Snyder 3 months ago

The v3number patches look reasonable. Tonight (ish) I'll push a release, then apply this with some spacing fixes.

#19 Updated by Wilson Snyder 2 months ago

v3number_hier_2 is pushed to git, just cleaned up some spacing and names, thanks for this.

#20 Updated by Todd Strader 2 months ago

Here's the rest (actual error location reporting) which has been rebased onto upstream: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_4

#21 Updated by Wilson Snyder 2 months ago

1. I don't think you should attach scopeName to modules. A module may have many uses at different scopes. Instead, look upwards for a Scope. While Scopes don't always exist at least when it is reported it will be correct, misleading information IMO is worse than omitting it.

2. I played a bit using the new error format and in my Emacs anyways I usually truncate lines. So what happens is long scopes hide the relevant error info e.g.

V--90 columns--                                                                                                     V -> rest not visible            
%Error: project/fc/fx.v:1234: (Location: top.fc.roc.foowrap.foo.ctl.csr.shim) Illegal bit or array select; type does not have a bit range, or bad dimension: type is real

What do you think of a more gcc-ish error?

%Error: project/fc/fx.v:1234: Illegal bit or array select; type does not have a bit range, or bad dimension: type is real
%Error:                     : In instance top.fc.roc.foowrap.foo.ctl.csr.shim

3. I think we should consider (in a new bug) to report column numbers and source text like GCC:

%Error: project/fc/fx.v:1234:23: Illegal bit or array select; type does not have a bit range, or bad dimension: type is real
   source_error_line = realvar[bad_select];
                              ^~~~
%Error:                        : In instance top.fc.roc.foowrap.foo.ctl.csr.shim

#22 Updated by Todd Strader 2 months ago

1. I don't think you should attach scopeName to modules.

I presume we're talking about m_hierName. Is this really a correctness issue or is it a completeness one? I agree that if you instantiate a module three times and get an error which reports m_hierName, you'll only get one of them. While it would be more complete to report all three locations, each one is independently correct (i.e. Error foo in instance bar.qux) and will steer you towards fixing your problem. I would even argue that reporting all three would be overkill and potentially confusing (I wouldn't want to see 100 instance paths for a common module's error).

I agree that we shouldn't provide incorrect information. However, I don't yet see how this patch does that. Is it possible that the path in m_hier could be flat out wrong? I would like to get the location information into the error messages for pre-flattened designs some way since that case represents a good chunk of the cases I'm handling today (71 tests failed when I took this out).

Also, re: scopes, see #6:

VarScope does hold the information we want, but it isn't in the backp path. VarRefs do point back to VarScopes, but the VarRefs also seem to have a fully expanded name at this point, so I just grabbed that.

Do you think this approach is OK?

What do you think of a more gcc-ish error?

I like this better. The one line solution was getting a bit cluttered.

3. I think we should consider (in a new bug) to report column numbers and source text like GCC:

Agreed. I'll open a new ticket.

I'll make the adjustment for #2, but please let me know what you think about #1.

#23 Updated by Wilson Snyder 2 months ago

Please pull from master and see test_regress/t/t_param_scope_bad and test_regress/t/t_clk_scope_bad

These are examples of errors where your proposed format will break, and knowing scope is important for understanding the error is. I intended this to show why you need to look for AstScopes, but having written these now realize we have a worse problem which is the scope information is not always present due to inlining.

#24 Updated by Todd Strader 2 months ago

Inlining is a bummer wrt to this issue. It seems like we'd have to possibly tag every node with a scope name before inlining to be able to report the error location. I'm inclined to defer that to another issue and get the rest of this straightened out.

I believe all the tests including your two new ones are either reporting the correct location or not reporting location at all (probably due to inlining): https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_5

One formatting issue is that some error messages have trailer lines added onto them before they get to V3Error::v3errorEnd(). This means that the location line is at the end of this whole thing as opposed to right after the first line, which I think would look better/make more sense. See: t_func_const2_bad.

I could search for the first '\n' and inject the location there to solve this, but that feels a little kludgy. But I think I'd like the error messages better that way. Please let me know what you think.

#25 Updated by Todd Strader about 2 months ago

I updated the branch based on my musings about the presentation in #24: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_5

I do think I like this layout better. Again, as far as I can tell this should be good to go. But there very well may be other cases I'm missing.

#26 Updated by Wilson Snyder about 2 months ago

1. First the trivial:

string locationMsg = msgPrefix()+locationStr;

Should be

string locationMsg = warnMore()+locationStr;

2.

%Error: t/t_array_backw_index_bad.v:13: Slice selection '[1:3]' has backward indexing versus data type's '[3:0]'
+%Error:                               : In instance t
 %Error: t/t_array_backw_index_bad.v:14: Slice selection '[3:1]' has backward indexing versus data type's '[0:3]'
+%Error:                               : In instance t

This matches the other multiline errors and is what I suggested, but thinking about it it seems not obviously grouped with the upper line; I think we should only print %Error only for the first error, this also helps editors that jump between errors:

%Error: t/t_array_backw_index_bad.v:14: Slice selection '[3:1]' has backward indexing versus data type's '[0:3]'
                                      : In instance t

Likewise when there's multiple file lines change to this:

%Warning-SYNCASYNCNET: t/t_lint_syncasyncnet_bad.v:15: Signal flopped as both synchronous and async: rst_both_l
                       t/t_lint_syncasyncnet_bad.v:90: ... Location of async usage
                       t/t_lint_syncasyncnet_bad.v:58: ... Location of sync usage
                       Use "/* verilator lint_off SYNCASYNCNET */" and lint_on around source to disable this message.

What do you think? If you agree it's cleaner I'll change warnMore and when you pull your code will get fixed.

3. Please merge from master - use -Xignore-all-space as did a one time cleanup.

4. I'm still pained about having hier known to V3Number, it just, well, feels wrong that a number class needs to know this. Also we would need to (ideally eventually) teach V3Graph about it, and anything else that reports errors. Options:

A. I get over it.
B. Put hier into FileLine.  To some extent this seems perfect as it captures all location information in the error-location-tracking class, but perhaps is coupling too tightly this class to specific usages, and not sure what else will get ugly.
C. Get hier from nodep->. I was looking at msg 9's repo again, can we just make sure when "reclaiming" a number in V3Simulate we zero nodep, then when reuse it reassign nodep?

#27 Updated by Wilson Snyder about 2 months ago

1. First the trivial:

string locationMsg = msgPrefix()+locationStr;

Should be

string locationMsg = warnMore()+locationStr;

2.

%Error: t/t_array_backw_index_bad.v:13: Slice selection '[1:3]' has backward indexing versus data type's '[3:0]'
%Error:                               : In instance t

This matches the other multiline errors and is what I suggested, but thinking about it it seems not obviously grouped with the upper line; I think we should only print %Error only for the first error, this also helps editors that jump between errors:

%Error: t/t_array_backw_index_bad.v:14: Slice selection '[3:1]' has backward indexing versus data type's '[0:3]'
                                      : In instance t

Likewise when there's multiple file lines change to this:

%Warning-SYNCASYNCNET: t/t_lint_syncasyncnet_bad.v:15: Signal flopped as both synchronous and async: rst_both_l
                       t/t_lint_syncasyncnet_bad.v:90: ... Location of async usage
                       t/t_lint_syncasyncnet_bad.v:58: ... Location of sync usage
                       Use "/* verilator lint_off SYNCASYNCNET */" and lint_on around source to disable this message.

What do you think? If you agree it's cleaner I'll change warnMore and when you pull your code will get fixed.

3. Please merge from master - use -Xignore-all-space as did a one time cleanup.

4. I'm still pained about having hier known to V3Number, it just, well, feels wrong that a number class needs to know this. Also we would need to (ideally eventually) teach V3Graph about it, and anything else that reports errors. Options:

A. I get over it.

B. Put hier into FileLine. To some extent this seems perfect as it captures all location information in the error-location-tracking class, but perhaps is coupling too tightly this class to specific usages, and not sure what else will get ugly.

C. Get hier from nodep->. I was looking at msg 9's repo again, can we just make sure when "reclaiming" a number in V3Simulate we zero nodep, then when reuse it reassign nodep?

#28 Updated by Todd Strader about 2 months ago

1 + 2: This sounds good to me. I'll change to warnMore() and look for your commit.

3: This commit just made my day.

4:

  • Let's start with C. I liked msg6's approach better, but I abandoned it because I didn't know how deep the hole went. I'll give it another pass.
  • Maybe I'm missing something, but I don't see how B could work since one FileLine can represent numerous hierarchical locations. Unless you're talking about cloning and uniquifying FileLines like Modules in V3Param or something.
  • And I can't comment on A, that sounds like a personal journey. Hopefully C works out.

I'm going to head in this direction. Please let me know if you think I should be headed elsewhere.

#29 Updated by Wilson Snyder about 2 months ago

I committed the error spacing change.

For option B I was assuming we'd clone filelines for each hier. Note we do something similar when warnings get suppressed.

I looked quickly at nodes in numbers, and yes there's a lot of staleness. We could use VL_LEAK_CHECK to see which are live; I did some cleanups against VL_LEAK_CHECK last night - there's certainly many leaks that have crept in since last used. If you want to do this, I'd turn off the leak check itself, just use V3Broken::isAllocated to know what nodep's are valid.

#30 Updated by Todd Strader about 1 month ago

Clarification on AstNode clean up: Will AstNodes ever be destroyed without calling deleteNode()? Or is that always a precursor to the destructor?

#31 Updated by Todd Strader about 1 month ago

Oh, ack. That is part of what VL_LEAK_CHECK/V3Broken::isAllocated is for, right? I clearly hadn't gotten around to trying that yet.

#32 Updated by Todd Strader 19 days ago

OK, here's the latest iteration: https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_8

I went ahead with option C. Some notes:

I added AstNode::cloneCleanup() as a post-step to clone() so that I could connect cloned AstConsts to their V3Numbers.

I ran into an issue in V3Hashed that required the tweak in here. It was trying to dereference the end() iterator. I unfortunately can't reproduce this now, but I'm sure the way I have it now is the correct way to short circuit that test.

I am currently living on both sides of the great tab purge. I know there are tabs in here and I'll clean them up once we've got the functionality settled.

#33 Updated by Wilson Snyder 16 days ago

I stumbled on the V3Hashed iterator problem earlier too, this is fixed in trunk.

I'm confused, why does AstNode needs to track num's? Seems error prone. Also any increase in what's in a node adds noticeably to memory and runtime.

Note for bug1315 I am suggesting we have V3Simulate only use AstNodes; I think that might also solve the problems here as then numbers involving any error will have a node attached.

Sorry the there's so much churn getting this bug resolved, we'll get there.

#34 Updated by Todd Strader 16 days ago

I currently have it so that V3Numbers point back to their AstNode so at error time they can get location information from the node. The AstNode's num list is there so the node can clean up these pointers when destroyed.

I'll have to check out bug1315. Do you think the only times V3Number errors out is within V3Simulate? Or are there other scenarios where V3Number will want to have an AstNide pooter or some way to get location information?

#35 Updated by Wilson Snyder 16 days ago

Yes, I think if V3Simulate uses AstConst, then the only standalone number use left is in the parser and that shouldn't error. Also at that point having a AstNode attached to every number would work ok (and only NULL when in parser).

#36 Updated by Todd Strader 12 days ago

So then would we have an AstNode* (or maybe AstConst*) in V3Number and set that pointer inside simulateVisitor::newNumber()? If so, would that be guaranteed to be safe? Namely, would a V3Number ever exist past the point where the AstConst it is pointing at exists?

#37 Updated by Wilson Snyder 12 days ago

Reply in bug1315.

Also available in: Atom