Merge pull request #5277 from YosysHQ/krys/fix_4983_alt

autoname: Avoid integer overflow

passes/cmds/autoname.cc

@@ -22,11 +22,24 @@
 USING_YOSYS_NAMESPACE
 PRIVATE_NAMESPACE_BEGIN
 
-int autoname_worker(Module *module, const dict<Wire*, int>& wire_score)
+typedef struct name_proposal {
+	string name;
+	unsigned int score;
+	name_proposal() : name(""), score(-1) { }
+	name_proposal(string name, unsigned int score) : name(name), score(score) { }
+	bool operator<(const name_proposal &other) const {
+		if (score != other.score)
+			return score < other.score;
+		else
+			return name.length() < other.name.length();
+	}
+} name_proposal;
+
+int autoname_worker(Module *module, const dict<Wire*, unsigned int>& wire_score)
 {
-	dict<Cell*, pair<int, string>> proposed_cell_names;
+	dict<Cell*, name_proposal> proposed_cell_names;
-	dict<Wire*, pair<int, string>> proposed_wire_names;
+	dict<Wire*, name_proposal> proposed_wire_names;
-	int best_score = -1;
+	name_proposal best_name;
 
 	for (auto cell : module->selected_cells()) {
 		if (cell->name[0] == '$') {
@@ -36,14 +49,14 @@ int autoname_worker(Module *module, const dict<Wire*, int>& wire_score)
 					if (bit.wire != nullptr && bit.wire->name[0] != '$') {
 						if (suffix.empty())
 							suffix = stringf("_%s_%s", log_id(cell->type), log_id(conn.first));
-						string new_name(bit.wire->name.str() + suffix);
+						name_proposal proposed_name(
-						int score = wire_score.at(bit.wire);
+							bit.wire->name.str() + suffix,
-						if (cell->output(conn.first)) score = 0;
+							cell->output(conn.first) ? 0 : wire_score.at(bit.wire)
-						score = 10000*score + new_name.size();
+						);
-						if (!proposed_cell_names.count(cell) || score < proposed_cell_names.at(cell).first) {
+						if (!proposed_cell_names.count(cell) || proposed_name < proposed_cell_names.at(cell)) {
-							if (best_score < 0 || score < best_score)
+							if (proposed_name < best_name)
-								best_score = score;
+								best_name = proposed_name;
-							proposed_cell_names[cell] = make_pair(score, new_name);
+							proposed_cell_names[cell] = proposed_name;
 						}
 					}
 			}
@@ -54,37 +67,44 @@ int autoname_worker(Module *module, const dict<Wire*, int>& wire_score)
 					if (bit.wire != nullptr && bit.wire->name[0] == '$' && !bit.wire->port_id) {
 						if (suffix.empty())
 							suffix = stringf("_%s", log_id(conn.first));
-						string new_name(cell->name.str() + suffix);
+						name_proposal proposed_name(
-						int score = wire_score.at(bit.wire);
+							cell->name.str() + suffix,
-						if (cell->output(conn.first)) score = 0;
+							cell->output(conn.first) ? 0 : wire_score.at(bit.wire)
-						score = 10000*score + new_name.size();
+						);
-						if (!proposed_wire_names.count(bit.wire) || score < proposed_wire_names.at(bit.wire).first) {
+						if (!proposed_wire_names.count(bit.wire) || proposed_name < proposed_wire_names.at(bit.wire)) {
-							if (best_score < 0 || score < best_score)
+							if (proposed_name < best_name)
-								best_score = score;
+								best_name = proposed_name;
-							proposed_wire_names[bit.wire] = make_pair(score, new_name);
+							proposed_wire_names[bit.wire] = proposed_name;
 						}
 					}
 			}
 		}
 	}
 
+	int count = 0;
+	// compare against double best score for following comparisons so we don't
+	// pre-empt a future iteration
+	best_name.score *= 2;
+
 	for (auto &it : proposed_cell_names) {
-		if (best_score*2 < it.second.first)
+		if (best_name < it.second)
 			continue;
-		IdString n = module->uniquify(IdString(it.second.second));
+		IdString n = module->uniquify(IdString(it.second.name));
 		log_debug("Rename cell %s in %s to %s.\n", log_id(it.first), log_id(module), log_id(n));
 		module->rename(it.first, n);
+		count++;
 	}
 
 	for (auto &it : proposed_wire_names) {
-		if (best_score*2 < it.second.first)
+		if (best_name < it.second)
 			continue;
-		IdString n = module->uniquify(IdString(it.second.second));
+		IdString n = module->uniquify(IdString(it.second.name));
 		log_debug("Rename wire %s in %s to %s.\n", log_id(it.first), log_id(module), log_id(n));
 		module->rename(it.first, n);
+		count++;
 	}
 
-	return proposed_cell_names.size() + proposed_wire_names.size();
+	return count;
 }
 
 struct AutonamePass : public Pass {
@@ -110,12 +130,13 @@ struct AutonamePass : public Pass {
 			// }
 			break;
 		}
+		extra_args(args, argidx, design);
 
 		log_header(design, "Executing AUTONAME pass.\n");
 
 		for (auto module : design->selected_modules())
 		{
-			dict<Wire*, int> wire_score;
+			dict<Wire*, unsigned int> wire_score;
 			for (auto cell : module->selected_cells())
 			for (auto &conn : cell->connections())
 			for (auto bit : conn.second)

tests/various/autoname.ys

@@ -1,10 +1,11 @@
+# prefer output name
+design -reset
 read_rtlil <<EOT
-autoidx 2
 module \top
-  wire output 3 $y
+  wire output 3 \y
   wire input 1 \a
   wire input 2 \b
-  cell $and \b_$and_B
+  cell $and $name
     parameter \A_SIGNED 0
     parameter \A_WIDTH 1
     parameter \B_SIGNED 0
@@ -12,8 +13,193 @@ module \top
     parameter \Y_WIDTH 1
     connect \A \a
     connect \B \b
+    connect \Y \y
+  end
+end
+EOT
+logger -expect log "Rename cell .name in top to y_.and_Y" 1
+logger -expect log "Renamed 1 objects" 1
+debug autoname
+logger -check-expected
+
+# fallback to shortest name if output is private
+design -reset
+read_rtlil <<EOT
+autoidx 2
+module \top
+  wire output 3 $y
+  wire input 1 \ab
+  wire input 2 \abcd
+  cell $or $name
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \ab
+    connect \B \abcd
     connect \Y $y
   end
 end
 EOT
-autoname
+logger -expect log "Rename cell .name in top to ab_.or_A" 1
+logger -expect log "Renamed 1 objects" 1
+debug autoname
+logger -check-expected
+
+# prefer low fanout over low name length
+design -reset
+read_rtlil <<EOT
+module \top
+  wire output 1 $y
+  wire input 2 \a
+  wire input 3 \bcd
+  wire input 4 \c_has_a_long_name
+  cell $and $name
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \a
+    connect \B \bcd
+    connect \Y $y
+  end
+
+  cell $or $name2
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \a
+    connect \B \c_has_a_long_name
+    connect \Y $y
+  end
+end
+EOT
+logger -expect log "Rename cell .name in top to bcd_.and_B" 1
+logger -expect log "Rename cell .name2 in top to c_has_a_long_name_.or_B" 1
+logger -expect log "Renamed 2 objects" 1
+debug autoname
+logger -check-expected
+
+# names are unique
+design -reset
+read_rtlil <<EOT
+module \top
+  wire output 3 \y
+  wire input 1 \a
+  wire input 2 \b
+  cell $and $name
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \a
+    connect \B \b
+    connect \Y \y
+  end
+
+  cell $and $name2
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \a
+    connect \B \b
+    connect \Y \y
+  end
+end
+EOT
+logger -expect log "Rename cell .name in top to y_.and_Y" 1
+logger -expect log "Rename cell .name2 in top to y_.and_Y_1" 1
+logger -expect log "Renamed 2 objects" 1
+debug autoname
+logger -check-expected
+
+# wires get autonames too
+design -reset
+read_rtlil <<EOT
+module \top
+  wire output 1 $y
+  wire input 2 \a
+  wire input 3 \bcd
+  wire $c
+  wire $d
+  wire $e
+  cell $__unknown $name
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \a
+    connect \B \bcd
+    connect \Y $c
+  end
+
+  cell $or \or
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A \a
+    connect \B \bcd
+    connect \Y $d
+  end
+
+  cell $or $name2
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A $c
+    connect \B $d
+    connect \Y $e
+  end
+
+  cell $and $name3
+    parameter \A_SIGNED 0
+    parameter \A_WIDTH 1
+    parameter \B_SIGNED 0
+    parameter \B_WIDTH 1
+    parameter \Y_WIDTH 1
+    connect \A $c
+    connect \B $e
+    connect \Y $y
+  end
+end
+EOT
+design -save order_test
+
+# wires are named for being cell outputs
+logger -expect log "Rename wire .d in top to or_Y" 1
+logger -expect log "Rename cell .name2 in top to or_Y_.or_B" 1
+logger -expect log "Renamed 2 objects" 1
+debug autoname t:$or
+logger -check-expected
+
+# $name gets shortest name (otherwise bcd_$__unknown_B)
+logger -expect log "Rename cell .name in top to a_.__unknown_A" 1
+# another output wire
+logger -expect log "Rename wire .e in top to or_Y_.or_B_Y" 1
+logger -expect log "Renamed 4 objects" 1
+debug autoname
+logger -check-expected
+
+# don't rename prematurely (some objects should be named after $name2)
+design -load order_test
+
+# $c gets shortest name, since the cell driving it doesn't have known port
+# directions (otherwise a_$__unknown_A_Y)
+logger -expect log "Rename wire .c in top to or_Y_.or_B_A" 1
+# $name3 named for lowest fanout wire (otherwise a_$__unknown_A_Y_$and_A)
+logger -expect log "Rename cell .name3 in top to or_Y_.or_B_Y_.and_B" 1
+logger -expect log "Renamed 6 objects" 1
+debug autoname
+logger -check-expected