BUG：Issue in Yosys Synthesis: 'std::length_error' Leads to Termination

[DeLoSoCode]

Version

yosys 0.35+56

On which OS did this happen?

Linux

Reproduction Steps

Hello,

While undergoing the synthesis procedure using Yosys, I encountered a complication when processing the design file (rtl.v). The execution of the read_verilog rtl.v command resulted in an error, as indicated in the provided screenshot. This issue arose during the optimization and synthesis phases of the design using Yosys tools. Specifically, the error pertained to a 'std::length_error,' causing the termination of the synthesis process.

Synthesis processes as follows:
read_verilog rtl.v
synth
write_verilog syn_yosys.v

Please find attached the code of RTL .
Thank you in advance for your attention to this matter.
I look forward to hearing from you regarding this issue.
yosys_project_01.zip

Expected Behavior

synthesis success

Actual Behavior

synthesis fail

[nakengelhardt]

Introduced in #3883. This is happening in passes/pmgen/peepopt_shiftadd.pmg:106, somehow offset ends up much larger than the width of the signal old_a. @phsauter would you have time to take a look?

Separately, we should probably add log_asserts on offset and length in the various extract methods. I'll make a PR for that.

[phsauter]

Introduced in #3883. This is happening in passes/pmgen/peepopt_shiftadd.pmg:106, somehow offset ends up much larger than the width of the signal old_a. @phsauter would you have time to take a look?

Sure, I will take a look at it this evening.

[phsauter]

I think I figured it out, a possible 'fix' is here:
f177031

But I have some thoughts before actually doing anything about it.
the problem here was that a constant shift of 181 bits to the right was performed on a signal only 19 bit wide.
This shifts out the signal completely, this case was not handled.
It comes from these lines:

input wire signed [(5'h12):(1'h0)] wire32;
wire signed [(5'h14):(1'h0)] wire35;
assign wire35 = {wire32};
assign wire66 = ((^$unsigned($signed((wire31 ? wire31 : wire33)))) ?
                      {($unsigned({wire32}) >> $signed((wire35 + (8'hb5))))} : wire31[(2'h2):(2'h2)]);

wire32 is 19 bits wide, wire35 is 21 bits wide.
wire32 is assigned via concatenation, meaning it is zero extended, not sign-extended to the larger wire35.
Then wire32 is shifted to the right by (wire35 + 181) but since the upper two bits of wire35 are always zero, wire35 must be positive and cannot counteract the shift by 181. This means wire32 is shifted to the right by 181 bit or even more, always shifting out the signal.

The reason I did not handle this case before is because I feel like something like this is likely unintended behavior of the RTL (ie faulty RTL).
I have now included a warning to point this out. @nakengelhardt do you think this is a good solution or should it be something else instead? (just a log-message or even nothing at all)
As-is it looks like this:

shiftadd pattern in module30: shift=$shr$rtl.v:168$122, add/sub=$add$rtl.v:168$121, offset: 181
Warning: signal \wire32 always shifted out by \wire32, did you mean for it to be signed?

@DeLoSoCode I would make sure the implemented function is actually what you intend it to do. The above code is currently equivalent to this:

assign wire66 = ( ^($signed((wire31 ? wire31 : wire33)) ) ? '0 : wire31[2];

I assume this is generated but I would still go over the section generating the constant offset h'hb5 (181).

[nakengelhardt]

Thank you! For context, we've had a couple of these reports before and this is fuzzer-generated code, @DeLoSoCode is working on a paper on finding bugs in EDA tools (there's some description on their profile and repo). The generated code is a bit hard to work with but has been finding valid issues.

I think this is probably unlikely enough to be intended that a warning is justified. Not sure if "always shifted out" is clear enough for people who don't have context, maybe something along the lines of "result of shift operation is always constant" along with some detail of operands and source line number of the shift operator?

[phsauter]

The wording definitely needs work and I like the idea of adding the file and line number.
Is there an official way to get this information or am I supposed to just parse the name of the instances?
(they are usually named something like $operation$file:line so I could see if it follows that pattern and use that information).

[nakengelhardt]

You can use cell->get_src_attribute(), it's not guaranteed to exist but the frontend does its best to put them on everything. It will return an empty string if no src attribute is found. You shouldn't try to interpret the name; it's not intended to be semantic and private names (the ones starting with $) have no guarantees whatsoever on their value - passes can rename, delete and recreate these at their leisure, and more often they end up named after the line of yosys code that last modified them, not something from the input.

(There's also wire->get_src_attribute() but if it's a public wire that points at the wire/reg declaration, not where it's used, so it's probably better to go for the src on the cell which should always point at the operator itself.)

[phsauter]

Before this issue is closed, are there any plans to add DeLoSo as its own test run or should this be added to the shiftadd/shiftmul tests to avoid regressions?

[povik]

add DeLoSo as its own test run

Not sure what you mean there. Do you mean if we should make a test out of the code posted with this issue? I think adding something minimal to tests/various/peepopt.ys to exercise the code from #4165 would be the best.

[phsauter]

I was refering to:

For context, we've had a couple of these reports before and this is fuzzer-generated code, @DeLoSoCode is working on a paper on finding bugs in EDA tools (there's some description on their profile and repo). The generated code is a bit hard to work with but has been finding valid issues.

Maybe you want to use DeLoSo and run the generated-code directly as tests against yosys instead of adding each thing it may find separately.

I definitely think it should be added as a test case and I am not against adding it to peepopt.ys, I just thought you may have already plans for DeLoSo tests.

[nakengelhardt]

No, we don't actually know anything about the project aside from what it says on the github page (they've never replied to any enquiries/issue comments), so we have no idea how to run those tools or how mature they are. But in general I would expect running a fuzzer is not compatible with the CI environment, just in terms of execution time needed for useful coverage...

[povik]

@phsauter Ah, I wasn't aware DeLoSo is the name of the tool: https://github.com/DeLoSoCode/DeLoSo