autoname: Check for potential overflow

[KrystalDelusion]

What are the reasons/motivation for this change?
Fix #4983
While unlikely, a wire with a score greater than about INT32_MAX/20000 will lead to an interger overflow when trying to rename users of that wire.

Explain how this is achieved.
Call log_error if any of the wires have a score over INT32_MAX/24000. Because the score is calculated with (10000*score + new_name.size())*2, if the new name is longer than 2000 characters long then there could still be an integer overflow, but this seems sufficiently unlikely that I thought checking each wire score once was better than checking each proposal, while still allowing for a fanout of almost 90000 bits from a single wire.

If applicable, please suggest to reviewers how they can test the change.

[jix]

I wouldn't expect a pass like autoname to produce a fatal error. While that is certainly better than undefined behavior, so I'm not opposed to merging this as a first step, how difficult would it be to make autoname generate slightly less exhaustive names when they would otherwise become to long?

[KrystalDelusion]

how difficult would it be to make autoname generate slightly less exhaustive names when they would otherwise become to long?

The case in #4983 is primarily due to high fanout rather than the length of name; rather the case for very long names is still vulnerable to integer overflow if the name is over 2000 characters. Quoting past me from that issue:

The highest "wire_score" (which I think corresponds to the fanout of a wire, as a number of bits) is $auto$hilomap.cc:39:hilomap_worker$945986 with 113633. The score for using that wire in the name of a cell then becomes score = 10000*score + new_name.size();, which in this case appears to be the 1136330020 in your runtime error, and is more than half of intmax (2,147,483,647).

It's the 10000*score which is the problematic part, which I think is done so that the length of the name is acting as tie-breaker if two wire_score's are otherwise equal. The best_score is later multiplied by 2, though I'm not entirely clear on what that is achieving. So in light of that, I think a refactor to remove the 10000*score portion while still using name length as tie-breaker instead of decision maker wouldn't be too hard (store an extra int and perform a second comparison when needed).

[KrystalDelusion]

Okay, so I made https://github.com/YosysHQ/yosys/tree/krys/fix_4983_alt, which leaves the score as-is and only compares the name length as necessary. It also changes the wire score from int to unsigned int so that instead of overflowing if the score is over INT32_MAX/20000, the wire fanout now needs to be over INT32_MAX, and is never dependent on the name length.

Unfortunately it is fully crashing my WSL when trying to run the example from #4983 (which is also what happens if I try to run without this PR that catches the potential error). So uhh, not quite sure what's actually going on there; I suspect it may not just be the integer overflow that's causing problems in that example. I'm guessing it's one or both of the proposed_*_names dictionaries getting too big and causing it to run out of memory, rather than actually having anything to do with the integer overflow; but normally when Yosys runs out of memory it gets killed by the OS so 🤷.

[nakengelhardt]

close in favor of #5277