Merge pull request #10419 from ethereum/bug-namesimplifier

Fix a bug in NameSimplifier.

Changelog.md

@@ -15,6 +15,8 @@ Compiler Features:
 Bugfixes:
  * Code generator: Do not pad empty string literals with a single 32-byte zero field in the ABI coder v1.
  * SMTChecker: Fix internal compiler error when doing bitwise compound assignment with string literals.
+ * Yul Optimizer: Fix a bug in NameSimplifier where a new name created by NameSimplifier could also be created by NameDispenser.
+ * Yul Optimizer: Removed NameSimplifier from optimization steps available to users.
 
 ### 0.7.5 (2020-11-18)
 

libsolidity/interface/OptimiserSettings.h

@@ -32,7 +32,7 @@ namespace solidity::frontend
 struct OptimiserSettings
 {
 	static char constexpr DefaultYulOptimiserSteps[] =
-		"NdhfoDgvulfnTUtnIf"            // None of these can make stack problems worse
+		"dhfoDgvulfnTUtnIf"            // None of these can make stack problems worse
 		"["
 			"xarrscLM"                 // Turn into SSA and simplify
 			"cCTUtTOntnfDIul"          // Perform structural simplification
@@ -47,7 +47,7 @@ struct OptimiserSettings
 			"gvif"                     // Run full inliner
 			"CTUcarrLsTOtfDncarrIulc"  // SSA plus simplify
 		"]"
-		"jmuljuljul VcTOcul jmulN";     // Make source short and pretty
+		"jmuljuljul VcTOcul jmul";     // Make source short and pretty
 
 	/// No optimisations at all - not recommended.
 	static OptimiserSettings none()

libyul/optimiser/NameDispenser.cpp

@@ -35,8 +35,9 @@ using namespace solidity::yul;
 using namespace solidity::util;
 
 NameDispenser::NameDispenser(Dialect const& _dialect, Block const& _ast, set<YulString> _reservedNames):
-	NameDispenser(_dialect, NameCollector(_ast).names() + std::move(_reservedNames))
+	NameDispenser(_dialect, NameCollector(_ast).names() + _reservedNames)
 {
+	m_reservedNames = move(_reservedNames);
 }
 
 NameDispenser::NameDispenser(Dialect const& _dialect, set<YulString> _usedNames):
@@ -61,3 +62,9 @@ bool NameDispenser::illegalName(YulString _name)
 {
 	return isRestrictedIdentifier(m_dialect, _name) || m_usedNames.count(_name);
 }
+
+void NameDispenser::reset(Block const& _ast)
+{
+	m_usedNames = NameCollector(_ast).names() + m_reservedNames;
+	m_counter = 0;
+}

libyul/optimiser/NameDispenser.h

@@ -51,11 +51,19 @@ public:
 	/// return it.
 	void markUsed(YulString _name) { m_usedNames.insert(_name); }
 
-private:
+	std::set<YulString> const& usedNames() { return m_usedNames; }
+
+	/// Returns true if `_name` is either used or is a restricted identifier.
 	bool illegalName(YulString _name);
 
+	/// Resets `m_usedNames` with *only* the names that are used in the AST. Also resets value of
+	/// `m_counter` to zero.
+	void reset(Block const& _ast);
+
+private:
 	Dialect const& m_dialect;
 	std::set<YulString> m_usedNames;
+	std::set<YulString> m_reservedNames;
 	size_t m_counter = 0;
 };
 

libyul/optimiser/NameSimplifier.cpp

@@ -19,6 +19,8 @@
 #include <libyul/optimiser/NameCollector.h>
 #include <libyul/AST.h>
 #include <libyul/Dialect.h>
+#include <libyul/YulString.h>
+#include <libyul/optimiser/NameDispenser.h>
 #include <libyul/optimiser/OptimizerUtilities.h>
 
 #include <libsolutil/CommonData.h>
@@ -28,19 +30,13 @@
 using namespace solidity::yul;
 using namespace std;
 
-NameSimplifier::NameSimplifier(
-	OptimiserStepContext& _context,
-	Block const& _ast
-):
-	m_context(_context),
-	m_usedNames(_context.reservedIdentifiers)
+NameSimplifier::NameSimplifier(OptimiserStepContext& _context, Block const& _ast):
+	m_context(_context)
 {
-	for (YulString name: m_usedNames)
+	for (YulString name: _context.reservedIdentifiers)
 		m_translations[name] = name;
 
-	set<YulString> allNames = NameCollector(_ast).names();
-	m_usedNames += allNames;
-	for (YulString name: allNames)
+	for (YulString const& name: NameCollector(_ast).names())
 		findSimplification(name);
 }
 
@@ -77,7 +73,7 @@ void NameSimplifier::operator()(FunctionCall& _funCall)
 	ASTModifier::operator()(_funCall);
 }
 
-void NameSimplifier::findSimplification(YulString _name)
+void NameSimplifier::findSimplification(YulString const& _name)
 {
 	if (m_translations.count(_name))
 		return;
@@ -98,19 +94,19 @@ void NameSimplifier::findSimplification(YulString _name)
 		{regex("index_access_t_array"), "index_access"},
 		{regex("[0-9]*_$"), ""}
 	};
+
 	for (auto const& [pattern, substitute]: replacements)
 	{
 		string candidate = regex_replace(name, pattern, substitute);
-		if (
-			!isRestrictedIdentifier(m_context.dialect, YulString(candidate)) &&
-			!m_usedNames.count(YulString(candidate))
-		)
+		if (!m_context.dispenser.illegalName(YulString(candidate)))
 			name = candidate;
 	}
+
 	if (name != _name.str())
 	{
-		m_usedNames.insert(YulString(name));
-		m_translations[_name] = YulString(name);
+		YulString newName{name};
+		m_context.dispenser.markUsed(newName);
+		m_translations[_name] = move(newName);
 	}
 }
 

libyul/optimiser/NameSimplifier.h

@@ -57,19 +57,15 @@ public:
 	void operator()(FunctionDefinition& _funDef) override;
 
 private:
-	NameSimplifier(
-		OptimiserStepContext& _context,
-		Block const& _ast
-	);
+	NameSimplifier(OptimiserStepContext& _context, Block const& _ast);
 
 	/// Tries to rename a list of variables.
 	void renameVariables(std::vector<TypedName>& _variables);
 
-	void findSimplification(YulString _name);
+	void findSimplification(YulString const& _name);
 	void translate(YulString& _name);
 
 	OptimiserStepContext& m_context;
-	std::set<YulString> m_usedNames;
 	std::map<YulString, YulString> m_translations;
 };
 

libyul/optimiser/Suite.cpp

@@ -105,6 +105,7 @@ void OptimiserSuite::run(
 	// ForLoopInitRewriter. Run them first to be able to run arbitrary sequences safely.
 	suite.runSequence("hfgo", ast);
 
+	NameSimplifier::run(suite.m_context, ast);
 	// Now the user-supplied part
 	suite.runSequence(_optimisationSequence, ast);
 
@@ -140,6 +141,9 @@ void OptimiserSuite::run(
 		if (ast.statements.size() > 1 && std::get<Block>(ast.statements.front()).statements.empty())
 			ast.statements.erase(ast.statements.begin());
 	}
+
+	suite.m_dispenser.reset(ast);
+	NameSimplifier::run(suite.m_context, ast);
 	VarNameCleaner::run(suite.m_context, ast);
 
 	*_object.analysisInfo = AsmAnalyzer::analyzeStrictAssertCorrect(_dialect, _object);
@@ -191,7 +195,6 @@ map<string, unique_ptr<OptimiserStep>> const& OptimiserSuite::allSteps()
 			LiteralRematerialiser,
 			LoadResolver,
 			LoopInvariantCodeMotion,
-			NameSimplifier,
 			RedundantAssignEliminator,
 			ReasoningBasedSimplifier,
 			Rematerialiser,
@@ -203,6 +206,7 @@ map<string, unique_ptr<OptimiserStep>> const& OptimiserSuite::allSteps()
 			VarDeclInitializer
 		>();
 	// Does not include VarNameCleaner because it destroys the property of unique names.
+	// Does not include NameSimplifier.
 	return instance;
 }
 
@@ -230,7 +234,6 @@ map<string, char> const& OptimiserSuite::stepNameToAbbreviationMap()
 		{LiteralRematerialiser::name,         'T'},
 		{LoadResolver::name,                  'L'},
 		{LoopInvariantCodeMotion::name,       'M'},
-		{NameSimplifier::name,                'N'},
 		{ReasoningBasedSimplifier::name,      'R'},
 		{RedundantAssignEliminator::name,     'r'},
 		{Rematerialiser::name,                'm'},

test/yulPhaser/Chromosome.cpp

@@ -138,7 +138,7 @@ BOOST_AUTO_TEST_CASE(output_operator_should_create_concise_and_unambiguous_strin
 
 	BOOST_TEST(chromosome.length() == allSteps.size());
 	BOOST_TEST(chromosome.optimisationSteps() == allSteps);
-	BOOST_TEST(toString(chromosome) == "flcCUnDvejsxIOoighTLMNRrmVatpud");
+	BOOST_TEST(toString(chromosome) == "flcCUnDvejsxIOoighTLMRrmVatpud");
 }
 
 BOOST_AUTO_TEST_CASE(optimisationSteps_should_translate_chromosomes_genes_to_optimisation_step_names)