Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't ignore errors reported after analysis #15624

Merged
merged 2 commits into from
Dec 11, 2024
Merged

Don't ignore errors reported after analysis #15624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e1486d
Fix unimplemented errors during Solidity and Yul parsing being treate…
cameel Dec 6, 2024
95c6e84
Don't ignore errors reported at optimization and assembling steps in …
cameel Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions libsolidity/interface/CompilerStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ bool CompilerStack::parse()
catch (UnimplementedFeatureError const& _error)
{
reportUnimplementedFeatureError(_error);
return false;
}

return true;
Expand Down Expand Up @@ -783,14 +784,17 @@ bool CompilerStack::compile(State _stopAfter)
// but CodeGenerationError is one case where someone decided to just throw Error.
solAssert(_error.type() == Error::Type::CodeGenerationError);
m_errorReporter.error(_error.errorId(), _error.type(), SourceLocation(), _error.what());
return false;
}
catch (UnimplementedFeatureError const& _error)
{
reportUnimplementedFeatureError(_error);
return false;
}

if (m_errorReporter.hasErrors())
return false;
}

solAssert(!m_errorReporter.hasErrors());
m_stackState = CompilationSuccessful;
this->link();
return true;
Expand Down Expand Up @@ -1610,6 +1614,14 @@ void CompilerStack::generateEVMFromIR(ContractDefinition const& _contract)
std::string deployedName = IRNames::deployedObject(_contract);
solAssert(!deployedName.empty(), "");
tie(compiledContract.evmAssembly, compiledContract.evmRuntimeAssembly) = stack.assembleEVMWithDeployed(deployedName);

if (stack.hasErrors())
{
for (std::shared_ptr<Error const> const& error: stack.errors())
reportIRPostAnalysisError(error.get());
return;
}

assembleYul(_contract, compiledContract.evmAssembly, compiledContract.evmRuntimeAssembly);
}

Expand Down Expand Up @@ -2004,6 +2016,27 @@ experimental::Analysis const& CompilerStack::experimentalAnalysis() const

void CompilerStack::reportUnimplementedFeatureError(UnimplementedFeatureError const& _error)
{
solAssert(_error.comment(), "Unimplemented feature errors must include a message for the user");
solAssert(_error.comment(), "Errors must include a message for the user.");

m_errorReporter.unimplementedFeatureError(1834_error, _error.sourceLocation(), *_error.comment());
}

void CompilerStack::reportIRPostAnalysisError(Error const* _error)
{
solAssert(_error);
solAssert(_error->comment(), "Errors must include a message for the user.");
solAssert(!_error->secondarySourceLocation());

// Do not report Yul warnings and infos. These are only reported in pure Yul compilation.
cameel marked this conversation as resolved.
Show resolved Hide resolved
if (!Error::isError(_error->severity()))
return;

m_errorReporter.error(
_error->errorId(),
_error->type(),
// Ignore the original location. It's likely missing, but even if not, it points at Yul source.
// CompilerStack can only point at locations in Solidity sources.
SourceLocation{},
*_error->comment()
);
}
1 change: 1 addition & 0 deletions libsolidity/interface/CompilerStack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,7 @@ class CompilerStack: public langutil::CharStreamProvider, public evmasm::Abstrac
) const;

void reportUnimplementedFeatureError(langutil::UnimplementedFeatureError const& _error);
void reportIRPostAnalysisError(langutil::Error const* _error);

ReadCallback::Callback m_readFile;
OptimiserSettings m_optimiserSettings;
Expand Down
56 changes: 29 additions & 27 deletions libsolidity/interface/StandardCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,9 +1609,35 @@ Json StandardCompiler::compileYul(InputsAndSettings _inputsAndSettings)
std::string const& sourceName = _inputsAndSettings.sources.begin()->first;
std::string const& sourceContents = _inputsAndSettings.sources.begin()->second;

// Inconsistent state - stop here to receive error reports from users
if (!stack.parseAndAnalyze(sourceName, sourceContents) && !stack.hasErrors())
solAssert(false, "No error reported, but parsing/analysis failed.");
std::string contractName;
bool const wildcardMatchesExperimental = true;
MachineAssemblyObject object;
MachineAssemblyObject deployedObject;

bool successful = stack.parseAndAnalyze(sourceName, sourceContents);
if (!successful)
// Inconsistent state - stop here to receive error reports from users
solAssert(stack.hasErrors(), "No error reported, but parsing/analysis failed.");
else
{
contractName = stack.parserResult()->name;
if (isArtifactRequested(_inputsAndSettings.outputSelection, sourceName, contractName, "ir", wildcardMatchesExperimental))
output["contracts"][sourceName][contractName]["ir"] = stack.print();

if (isArtifactRequested(_inputsAndSettings.outputSelection, sourceName, contractName, "ast", wildcardMatchesExperimental))
{
Json sourceResult;
sourceResult["id"] = 0;
sourceResult["ast"] = stack.astJson();
output["sources"][sourceName] = sourceResult;
}
stack.optimize();
std::tie(object, deployedObject) = stack.assembleWithDeployed();
if (object.bytecode)
object.bytecode->link(_inputsAndSettings.libraries);
if (deployedObject.bytecode)
deployedObject.bytecode->link(_inputsAndSettings.libraries);
}

for (auto const& error: stack.errors())
{
Expand All @@ -1628,30 +1654,6 @@ Json StandardCompiler::compileYul(InputsAndSettings _inputsAndSettings)
if (stack.hasErrors())
return output;

std::string contractName = stack.parserResult()->name;

bool const wildcardMatchesExperimental = true;
if (isArtifactRequested(_inputsAndSettings.outputSelection, sourceName, contractName, "ir", wildcardMatchesExperimental))
output["contracts"][sourceName][contractName]["ir"] = stack.print();

if (isArtifactRequested(_inputsAndSettings.outputSelection, sourceName, contractName, "ast", wildcardMatchesExperimental))
{
Json sourceResult;
sourceResult["id"] = 0;
sourceResult["ast"] = stack.astJson();
output["sources"][sourceName] = sourceResult;
}
stack.optimize();

MachineAssemblyObject object;
MachineAssemblyObject deployedObject;
std::tie(object, deployedObject) = stack.assembleWithDeployed();

if (object.bytecode)
object.bytecode->link(_inputsAndSettings.libraries);
if (deployedObject.bytecode)
deployedObject.bytecode->link(_inputsAndSettings.libraries);

for (auto&& [kind, isDeployed]: {make_pair("bytecode"s, false), make_pair("deployedBytecode"s, true)})
if (isArtifactRequested(
_inputsAndSettings.outputSelection,
Expand Down
16 changes: 12 additions & 4 deletions libyul/YulStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ bool YulStack::parse(std::string const& _sourceName, std::string const& _source)
catch (UnimplementedFeatureError const& _error)
{
reportUnimplementedFeatureError(_error);
return false;
}

if (!m_errorReporter.hasErrors())
Expand Down Expand Up @@ -248,9 +249,14 @@ MachineAssemblyObject YulStack::assemble(Machine _machine)
std::pair<MachineAssemblyObject, MachineAssemblyObject>
YulStack::assembleWithDeployed(std::optional<std::string_view> _deployName)
{
yulAssert(m_charStream);

auto [creationAssembly, deployedAssembly] = assembleEVMWithDeployed(_deployName);
yulAssert(creationAssembly, "");
yulAssert(m_charStream, "");
if (!creationAssembly)
{
yulAssert(!deployedAssembly);
return {MachineAssemblyObject{}, MachineAssemblyObject{}};
}

MachineAssemblyObject creationObject;
MachineAssemblyObject deployedObject;
Expand Down Expand Up @@ -284,6 +290,7 @@ YulStack::assembleWithDeployed(std::optional<std::string_view> _deployName)
catch (UnimplementedFeatureError const& _error)
{
reportUnimplementedFeatureError(_error);
return {MachineAssemblyObject{}, MachineAssemblyObject{}};
}

return {std::move(creationObject), std::move(deployedObject)};
Expand Down Expand Up @@ -340,9 +347,10 @@ YulStack::assembleEVMWithDeployed(std::optional<std::string_view> _deployName)
catch (UnimplementedFeatureError const& _error)
{
reportUnimplementedFeatureError(_error);
return {nullptr, nullptr};
}

return {std::make_shared<evmasm::Assembly>(assembly), {}};
return {std::make_shared<evmasm::Assembly>(assembly), nullptr};
}

std::string YulStack::print() const
Expand Down Expand Up @@ -414,6 +422,6 @@ std::shared_ptr<Object> YulStack::parserResult() const

void YulStack::reportUnimplementedFeatureError(UnimplementedFeatureError const& _error)
{
solAssert(_error.comment(), "Unimplemented feature errors must include a message for the user");
yulAssert(_error.comment(), "Errors must include a message for the user.");
m_errorReporter.unimplementedFeatureError(1920_error, _error.sourceLocation(), *_error.comment());
}
38 changes: 22 additions & 16 deletions solc/CommandLineInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1232,9 +1232,10 @@ void CommandLineInterface::assembleYul(yul::YulStack::Language _language, yul::Y

bool successful = true;
std::map<std::string, yul::YulStack> yulStacks;
for (auto const& src: m_fileReader.sourceUnits())
std::map<std::string, yul::MachineAssemblyObject> objects;
for (auto const& [sourceUnitName, yulSource]: m_fileReader.sourceUnits())
{
auto& stack = yulStacks[src.first] = yul::YulStack(
auto& stack = yulStacks[sourceUnitName] = yul::YulStack(
m_options.output.evmVersion,
m_options.output.eofVersion,
_language,
Expand All @@ -1244,20 +1245,28 @@ void CommandLineInterface::assembleYul(yul::YulStack::Language _language, yul::Y
DebugInfoSelection::Default()
);

if (!stack.parseAndAnalyze(src.first, src.second))
successful = false;
successful = successful && stack.parseAndAnalyze(sourceUnitName, yulSource);
if (!successful)
solAssert(stack.hasErrors(), "No error reported, but parsing/analysis failed.");
else
stack.optimize();

if (successful && m_options.compiler.outputs.asmJson)
{
std::shared_ptr<yul::Object> result = stack.parserResult();
if (result && !result->hasContiguousSourceIndices())
if (
m_options.compiler.outputs.asmJson &&
stack.parserResult() &&
!stack.parserResult()->hasContiguousSourceIndices()
Comment on lines +1255 to +1256
Copy link
Member

@r0qs r0qs Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, stack.optimize() was called prior to invoking stack.parserResult(). However, now it is called afterward (on line 1264). If I’m not mistaken, the Object returned by parserResult() can also be modified by optimize(), meaning that hasContiguousSourceIndices will not be checked against the optimized object anymore, no? Is this intentional, or am I misunderstanding something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that was intentional. The check was inserted after optimize() probably simply because it was more convenient to do it there. IMO it makes more sense to do such checks on the unoptimized code that the user is submitting rather than on some modified version so the change is good.

In any case, it should not affect solc behavior in practice - optimize() theoretically could change the code in an arbitrary way, but in practice it will never touch the @use-src annotation so this property should remain unchanged.

We also have a second validation against this in the asm export code, which would catch this if it did happen:

solidity/libevmasm/Assembly.cpp

Lines 509 to 514 in ea52aca

solAssert(maxSourceIndex + 1 >= _sourceIndices.size());
solRequire(
_sourceIndices.size() == 0 || _sourceIndices.size() == maxSourceIndex + 1,
AssemblyImportException,
"The 'sourceList' array contains invalid 'null' item."
);

We have the check in the CLI only because we don't catch AssemblyImportError there. Perhaps we should later change it into a CodeGenerationError so that it gets caught by YulStack and reported as a normal error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the check in the CLI only because we don't catch AssemblyImportError there. Perhaps we should later change it into a CodeGenerationError so that it gets caught by YulStack and reported as a normal error.

Yeah, I don't see why not do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll keep it in mind for when we're done with this series of PRs and can actually handle those errors properly.

)
solThrow(
CommandLineExecutionError,
"Generating the assembly JSON output was not possible. "
"Source indices provided in the @use-src annotation in the Yul input do not start at 0 or are not contiguous."
);

stack.optimize();

yul::MachineAssemblyObject object = stack.assemble(_targetMachine);
if (object.bytecode)
object.bytecode->link(m_options.linker.libraries);
objects.insert({sourceUnitName, std::move(object)});
}
}

Expand All @@ -1281,13 +1290,14 @@ void CommandLineInterface::assembleYul(yul::YulStack::Language _language, yul::Y
solThrow(CommandLineExecutionError, "");
}

for (auto const& src: m_fileReader.sourceUnits())
for (auto const& [sourceUnitName, yulSource]: m_fileReader.sourceUnits())
{
solAssert(_targetMachine == yul::YulStack::Machine::EVM);
std::string machine = "EVM";
sout() << std::endl << "======= " << src.first << " (" << machine << ") =======" << std::endl;
sout() << std::endl << "======= " << sourceUnitName << " (" << machine << ") =======" << std::endl;

yul::YulStack& stack = yulStacks[src.first];
yul::YulStack const& stack = yulStacks[sourceUnitName];
yul::MachineAssemblyObject const& object = objects[sourceUnitName];

if (m_options.compiler.outputs.irOptimized)
{
Expand All @@ -1297,10 +1307,6 @@ void CommandLineInterface::assembleYul(yul::YulStack::Language _language, yul::Y
sout() << stack.print() << std::endl;
}

yul::MachineAssemblyObject object;
object = stack.assemble(_targetMachine);
object.bytecode->link(m_options.linker.libraries);

if (m_options.compiler.outputs.binary)
{
sout() << std::endl << "Binary representation:" << std::endl;
Expand Down
16 changes: 10 additions & 6 deletions test/libyul/ObjectCompilerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,22 @@ TestCase::TestResult ObjectCompilerTest::run(std::ostream& _stream, std::string
OptimiserSettings::preset(m_optimisationPreset),
DebugInfoSelection::All()
);
if (!stack.parseAndAnalyze("source", m_source))
bool successful = stack.parseAndAnalyze("source", m_source);
MachineAssemblyObject obj;
if (successful)
{
stack.optimize();
obj = stack.assemble(YulStack::Machine::EVM);
}
if (stack.hasErrors())
{
AnsiColorized(_stream, _formatted, {formatting::BOLD, formatting::RED}) << _linePrefix << "Error parsing source." << std::endl;
SourceReferenceFormatter{_stream, stack, true, false}
.printErrorInformation(stack.errors());
return TestResult::FatalError;
}
stack.optimize();

MachineAssemblyObject obj = stack.assemble(YulStack::Machine::EVM);
solAssert(obj.bytecode, "");
solAssert(obj.sourceMappings, "");
solAssert(obj.bytecode);
solAssert(obj.sourceMappings);

m_obtainedResult = "Assembly:\n" + obj.assembly->assemblyString(stack.debugInfoSelection());
if (obj.bytecode->bytecode.empty())
Expand Down