zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
561 stars 55 forks source link

false "field not found in struct" error, source unclear #155

Closed mkorbel1 closed 3 years ago

mkorbel1 commented 3 years ago

I'm receiving an error like this:

sv2v: field '...' not found in struct
...
CallStack (from HasCallStack):
  error, called at src/Convert/Struct.hs:470:14 in main:Convert.Struct

The code being parsed already compiles as valid SystemVerilog in other tools, so I'm skeptical that there is actually an access to a field that doesn't exist in the struct. The structs being parsed are hierarchical (i.e. structs of structs of structs...), if that is relevant. I cannot paste the full output of the error in a public forum.

It is unclear to me what is causing the error, which lines in the original code are related, or even which specific structs it is trying to parse where it is looking for that field.

First, can this error be more helpful towards debugging? Second, I suspect there is a bug in sv2v causing this failure, but without more details it's hard to tell what precisely the bug is.

zachjs commented 3 years ago

Given the above information, I agree that some bug in sv2v is likely responsible for this error. Hopefully this will help locate it without too much pain!

I am very interested in providing additional context in errors raised beyond the frontend, but haven't yet settled on how best to go about this. However, I've thrown together a small branch (struct-debug) which attempts to provide more useful information specifically in the above error case (though the same strategy could be used elsewhere). On this branch, the error message now shows the base expression of the struct access, as well as the rough hierarchical location of failing access, starting with the module name. Further, if you run with -v/--verbose, the error will also contain an approximate nearby source location. Please let me know what you find!

mkorbel1 commented 3 years ago

Thanks, I was working off of b0b7962, but after pulling up to latest master (or strict-debug) I'm getting new errors related to something else. They appear to be valid new errors, but the other tools that were parsing this must have flagged them as warnings instead. I'm working through those and then will see once I reach the struct error again (if it still exists).

mkorbel1 commented 3 years ago

The extra print statements help a lot, thank you! It turns out that there was an issue in the code, but it was protected around a hard-coded localparam.

localparam SOME_PARAM = 1;
if(SOME_PARAM) begin
// good code
end else begin
// bad code, field accessed that does not exist in struct
end

I'm not sure the technical rule on this in SystemVerilog. The tools we used for this code haven't flagged the issue before, but maybe it is fair for sv2v to flag it as an issue?

zachjs commented 3 years ago

If SOME_PARAM were a parameter rather than localparam, I'd argue sv2v would have to raise an error. Missing fields always yield an error in other frontends, and these errors cannot be "represented" in Verilog-2005.

If you use if (1) or if (0), sv2v will indeed remove the dead code and avoid the error. When using a localparam set to a constant, I could have sv2v remove the blocks before reaching the struct conversion, but I wonder how far this could go. What if it's set to the result of some constant function? What if that constant function has complex procedural logic, loops, etc.?

Arguably the only benefit of disabling logic via generate conditionals over `ifdef or comments is that the disabled code is still lexed and parsed. That said, I understand it isn't reasonable to require significant rewrites to an existing codebase for it to work with sv2v.

Naturally, my preference would be for sv2v to "do less", but I'm eager to better understand and address potential usability issues.

mkorbel1 commented 3 years ago

I tend to agree with you here. It seems a little unreasonable for sv2v to support poor code just because some other tool supported it unless it really has a big impact. I'll be fixing our code up a bit to avoid this situation, since I think that's the right thing to do, and I'll report back if it appears to be a broader common issue.

I think the print messages are super valuable and worth keeping around.

zachjs commented 3 years ago

@mkorbel1 I've brought in a refined version of the new debugging information logic for the struct conversion and in a few other places. For example, running sv2v -v test/error/struct_unknown_field.sv now produces:

sv2v: field 'y' not found in struct packed {
    logic x;
}, in expression x.y, within scope top, near test/error/struct_unknown_field.sv:8:5
zachjs commented 3 years ago

Given the new error messaging has been merged some time without issue, I'm closing this issue. Thanks for your feedback! Please feel free to reopen or file a new issue if necessary.