Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error messages do not contain hierarchical information #1305

Closed
veripoolbot opened this issue Apr 20, 2018 · 45 comments
Closed

Error messages do not contain hierarchical information #1305

veripoolbot opened this issue Apr 20, 2018 · 45 comments
Assignees
Labels
area: lint Issue involves SystemVerilog lint checking resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


Author Name: Todd Strader (@toddstrader)
Original Redmine Issue: 1305 from https://www.veripool.org

Original Assignee: Todd Strader (@toddstrader)


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-04-21T11:40:05Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2018-05-03T18:23:28Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-05-04T10:38:46Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-04-12T16:18:44Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-04-12T17:19:15Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-04-16T16:26:34Z


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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-04-17T16:28:12Z


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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-04-17T23:56:49Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-04-23T17:29:40Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-01T23:46:39Z


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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-01T23:49:32Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-02T18:02:47Z


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

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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-02T18:31:04Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-02T23:45:11Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-03T14:21:38Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-04T01:37:41Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-07T18:27:59Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-08T11:11:13Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-10T00:03:31Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-10T13:35:11Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-12T19:56:03Z


  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

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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-13T18:16:25Z


  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.

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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-13T23:32:02Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-14T21:04:41Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-29T17:26:00Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-30T01:00:09Z


  1. First the trivial:

      string locationMsg = msgPrefix()+locationStr;
    

Should be

     string locationMsg = warnMore()+locationStr;
%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.

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

  2. 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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-30T01:01:26Z


  1. First the trivial:

      string locationMsg = msgPrefix()+locationStr;
    

Should be

     string locationMsg = warnMore()+locationStr;
%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.

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

  2. 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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-30T12:02:49Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-31T00:33:08Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-06-19T20:58:18Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-06-20T11:36:29Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-03T14:26:53Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-06T18:20:25Z


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 #� 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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-06T19:15:32Z


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 #�. 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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-06T19:25:01Z


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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-10T19:13:44Z


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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-10T21:58:57Z


Reply in #�.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-25T14:44:41Z


After the refactor in #�, I think we're probably/hopefully there:
https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_10

I realized that I never handled generate blocks in the location names. That's in there now and there's a test to go along with it.

I also noticed along the way that ParamVisitor::visitModules() was operating on an m_cellps which had two copies of each AstCell it was going to work on:

(rr) p m_cellps
$8 = std::__debug::deque with 8 elements = {0x564e337e77c0, 0x564e337e8df0, 0x564e337ebd30, 0x564e337ed380, 0x564e337e77c0, 0x564e337e8df0, 0x564e337ebd30, 0x564e337ed380}

Is this intentional? Do we need two passes of visitCell() for each AstCell?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-25T15:27:59Z


Nope. Apparently during my git juggling of the _bad.out files, I missed that I committed two test which are now tossing internal faults. So it's passing locally for me because I've told it to expect the internal fault. For whatever reason, Travis isn't tossing the internal fault on this so it failed on my branch.

I'll clean this up and follow up with another commit.

Perhaps we should disallow "Verilator internal fault" in the .out files?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-25T17:07:54Z


OK, none of the .out files are looking for internal faults now. And CI is green:
https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_10

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-25T22:39:47Z


Multiple cell visiting in AstParam is expected as it does the original cell, then the one it clones to de-parameterize.

+++ b/src/V3Ast.cpp
+string AstNode::locationStr() const {
...
+            if (scopep->isTop())
+                break;

Nit, either one line it, or multiline ifs should use { }.

+++ b/src/V3Error.cpp
+void V3Error::v3errorEnd(std::ostringstream& sstr, const string& locationStr) {
...
+    if (!locationStr.empty()) {
+        string locationMsg = warnMore()+locationStr;
+        if (locationMsg[locationMsg.length()-1] != '\n') locationMsg += '\n';
+        size_t pos = msg.find("\n");
+        msg.insert(pos + 1, locationMsg);
+    }
...
+void FileLine::v3errorEnd(std::ostringstream& str, const string& locationStr) {
...
+    std::ostringstream lstr;
+    if (!locationStr.empty()) {
+        lstr<<std::setw(ascii().length())<<" "<<": "<<locationStr;
+    }

"lstr<<std::setw(ascii().length())<<" "<<": "" should instead be "warnMore()"

Larger issue is can you see if this can be moved into warnContext() (only
printed when !secondary)? That has better positioning for some multiple-line messages.

+++ b/src/V3Param.cpp
     //   AstVar::user4()        // int    Global parameter number (for naming new module)
+    //   AstCell::user5p()      // string* Generate portion of hierarchical name
     //                          //        (0=not processed, 1=iterated, but no number,
     //                          //         65+ parameter numbered)

The inserted line should be 2 lines lower as the next two comments refer to the "int" above.

+                if (m_modp->hierName().empty())
+                    m_modp->hierName(m_modp->origName());

Nit, use { } in multiline if's. Check other places too.

+                            string* genHierName = (string *) nodep->user5p();
+                            if (genHierName)
+                                fullName += *genHierName;

Can put temporary inside the if to ensure not misused. Also "p" on end of pointers:

    if (string* genHierNamep = (string *) nodep->user5p()) {
        fullName += *genHierNamep;
    }


+                    string* genHierName = (string *) cellp->user5p();
+                    if (genHierName) {
+                        delete genHierName;
+                        cellp->user5p(NULL);
+                    }

Likewise if() and "p", also add dangling:

   if (string* genHierNamep = (string *) cellp->user5p()) {
       delete genHierNamep; VL_DANGLING(genHierNamep);
       cellp->user5p(NULL);
   }


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

Suggest it should be ": ... In instance t" as now trying to use ... when print continuations of a previous error.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-26T15:51:09Z


I updated the branch with all these changes:
https://github.com/toddstrader/verilator-dev/tree/mod_stack_trace_10

However, the warnContext() change has some possibly undesirable consequences. If you look at the last commit on the branch, some of the .out files have largely duplicate error messages now:
toddstrader/verilator-dev@54ed758#diff-184eb43806923a0cc96a95c9d4125485

That is because error message de-duping is done in V3Error::v3errorEnd(). And I specifically added the location string to the message after the de-duping was done so that even if there were multiple instances tossing an error you'd only get one. I figured this was better in the case where you have a ton of instances with the same problem and workable where you had two separate problems with the same message (you'd fix one, re-run and then fix the next).

void V3Error::v3errorEnd(std::ostringstream& sstr, const string& locationStr) {
...
     // Suppress duplicate messages
     if (s_messages.find(msg) != s_messages.end()) return;
     s_messages.insert(msg);
     if (!locationStr.empty()) {
         string locationMsg = warnMore()+locationStr+"\n";
         size_t pos = msg.find("\n");
         msg.insert(pos + 1, locationMsg);
     }

</code>

Before the warnContext() change, FileLine::v3errorEnd() was passing locationStr into V3Error::v3errorEnd() so that it could both append it and ignore it for de-duplication purposes. But now, FileLine::v3errorEnd() is doing the appending and V3Error::v3errorEnd() no longer gets locationStr and can't ignore it for de-duping:

-    else if (!V3Error::errorContexted()) nsstr<<warnContextPrimary();
-    V3Error::v3errorEnd(nsstr, lstr.str());
+    else if (!V3Error::errorContexted()) nsstr<<warnContextPrimary(locationStr);
+    V3Error::v3errorEnd(nsstr);
</code>

I agree that the general format/placement of these messages works better with warnContext(). But I think the largely duplicative messages are bad.

What do you think? I see three options:

  1. This way is OK and we're fine with the duplication
  2. The old code structure was fine
  3. Error messages need to be a new class with message, location, primaryContext, secondaryContext, etc. members so that we can de-dup on only the things we want

#3 would allow us to keep using warnContext() and get rid of these duplicate messages. But that would be a big refactor since it would potentially touch everything that reports an error. I'm partial to #2 myself. Thoughts? Do you see a better way to handle this?

Also, if we go with #1, I think I should move the dup checking to after the location string appending in V3Error::v3errorEnd() for nodes that don't have a FileLine so that those messages will behave the same as everything else.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-26T16:30:29Z


Long term I think we need a nicer way to handle error messages but a project better left for later. So I agree #2 is ok, you can go back to that, then feel free to squash and push to trunk.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-26T17:30:34Z


Sold. This has been squashed, pushed and is green.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-29T23:14:11Z


In 4.018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: lint Issue involves SystemVerilog lint checking resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

2 participants