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
Comments
Original Redmine Comment 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. |
Original Redmine Comment I dumped the tree (attached) at the time of the error and I also traced the backp()'s. Here is the latter:
I only see one SCOPENAME in the whole AST, which is somehow attached to the $fatal():
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:
Find a cell that points to the module:
Use backp() to find the parent module, repeat until you find the top:
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. |
Original Redmine Comment 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. |
Original Redmine Comment I've posted a proposed change for this here: 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:
|
Original Redmine Comment 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...) |
Original Redmine Comment I've posted the latest changes here: 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? |
Original Redmine Comment 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? |
Original Redmine Comment 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). |
Original Redmine Comment 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: 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: 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. |
Original Redmine Comment 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? |
Original Redmine Comment Oh, also please merge from upstream; I merged your MANIFEST.SKIP change. |
Original Redmine Comment I'm juggling git commits now. In the meantime, I've got this to survey other tools' error messages: I was copying Riviera's message format (except with dots instead of slashes):
VCS doesn't have a prefix and uses dots:
And ncsim just tosses it out like some kind of non sequitur:
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. |
Original Redmine Comment The V3Number refactor is isolated here: And the golden file change is here: And here is the full solution based on the first two patches: Once we land the first two, I'll rebase the third one. |
Original Redmine Comment Golden file is committed. V3Number comments:
I could puzzle it through, but why is V3NumberConstructor needed?
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; 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.
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'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. |
Original Redmine Comment I've updated this branch based on the items below:
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.:
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.
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.
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.
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.
Sorry, there's a note about this in the commit message:
You can see how it is used in the mod_stack_trace_3 branch: It is currently benign and speaks of things to come.
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.
Done. |
Original Redmine Comment All the different AstConst constructor flavors with V3NumberConstructor are just the
I commented them out to find the flavors that are used. So how's the below
I got rid of needing two of the flavors with the new code put in master;
But then won't you (eventually) need the proper hierName from the callee
If it's not used in this commit I'd be inclined to not include it in this patch,
Thinking about this further I suggest it's cleaner to add: void V3Number::v3errorEnd(std::ostringstream& str) { then the normal m_fileline->v3error calls can be used in V3Number and they will call Another alternative is to add a FileLineHier class with a FileLine and Note when I looked at how this works I now realize that V3Graph |
Original Redmine Comment I've rebased onto master and taken care of the above: 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. |
Original Redmine Comment The v3number patches look reasonable. Tonight (ish) I'll push a release, then apply this with some spacing fixes. |
Original Redmine Comment v3number_hier_2 is pushed to git, just cleaned up some spacing and names, thanks for this. |
Original Redmine Comment Here's the rest (actual error location reporting) which has been rebased onto upstream: |
Original Redmine Comment
V--90 columns-- V -> rest not visible 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: project/fc/fx.v:1234:23: Illegal bit or array select; type does not have a bit range, or bad dimension: type is real |
Original Redmine Comment
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:
Do you think this approach is OK?
I like this better. The one line solution was getting a bit cluttered.
Agreed. I'll open a new ticket. I'll make the adjustment for #2, but please let me know what you think about #1. |
Original Redmine Comment 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. |
Original Redmine Comment 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): 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. |
Original Redmine Comment I updated the branch based on my musings about the presentation in #24: 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. |
Original Redmine Comment
Should be
+%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]' 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 What do you think? If you agree it's cleaner I'll change warnMore and when you pull your code will get fixed.
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? |
Original Redmine Comment
Should be
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]' 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 What do you think? If you agree it's cleaner I'll change warnMore and when you pull your code will get fixed.
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? |
Original Redmine Comment 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:
I'm going to head in this direction. Please let me know if you think I should be headed elsewhere. |
Original Redmine Comment 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. |
Original Redmine Comment Clarification on AstNode clean up: |
Original Redmine Comment 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. |
Original Redmine Comment OK, here's the latest iteration: 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. |
Original Redmine Comment 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. |
Original Redmine Comment 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? |
Original Redmine Comment 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). |
Original Redmine Comment 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? |
Original Redmine Comment Reply in #�. |
Original Redmine Comment After the refactor in #�, I think we're probably/hopefully there: 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:
Is this intentional? Do we need two passes of visitCell() for each AstCell? |
Original Redmine Comment 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? |
Original Redmine Comment OK, none of the .out files are looking for internal faults now. And CI is green: |
Original Redmine Comment Multiple cell visiting in AstParam is expected as it does the original cell, then the one it clones to de-parameterize.
Nit, either one line it, or multiline ifs should use { }.
"lstr<<std::setw(ascii().length())<<" "<<": "" should instead be "warnMore()" Larger issue is can you see if this can be moved into warnContext() (only
The inserted line should be 2 lines lower as the next two comments refer to the "int" above.
Nit, use { } in multiline if's. Check other places too.
Can put temporary inside the if to ensure not misused. Also "p" on end of pointers:
Likewise if() and "p", also add dangling:
Suggest it should be ": ... In instance t" as now trying to use ... when print continuations of a previous error. |
Original Redmine Comment I updated the branch with all these changes: 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: 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).
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:
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:
#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. |
Original Redmine Comment Sold. This has been squashed, pushed and is green. |
Original Redmine Comment In 4.018. |
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:
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.
The text was updated successfully, but these errors were encountered: