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.

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

[solidity,llvm-context] Improve support for debugging the compiler #32

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

wpt967
Copy link
Collaborator

@wpt967 wpt967 commented Aug 23, 2024

Add option --recursive-process-input for use with --recursive-process to specify the name of a file to use instead of reading from stdin.

If --debug-output-dir is set, dump the file passed to the recursive invocation of the compiler as a JSON file suitable for use with --recursive-process-input.

These changes are intended to support debugging the compiler and are only available with DEBUG builds.

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

LGTM, just some nitpicks

.expect("Error reading from input file");
}
None => {
stdin.read_to_end(&mut buffer).expect("Stdin reading error");
Copy link
Member

Choose a reason for hiding this comment

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

Same as above :)

match input_file {
Some(ins) => {
ins.read_to_end(&mut buffer)
.expect("Error reading from input file");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We usually unwrap results in case we are guaranteed to never panic (the rust compiler can not now) and use expect(...) to explain the reason as to why the unwrap would never panic.

But since the this function does already return a anhow::Result it seems like here we can be just bubble up the error instead?

@@ -74,6 +82,11 @@ pub fn call(input: Input) -> anyhow::Result<Output> {
anyhow::anyhow!("{:?} subprocess spawning error: {:?}", executable, error)
})?;

#[cfg(debug_assertions)]
if let Some(dbg_config) = &input.debug_config {
let _ = dbg_config.dump_stage_output(&input.contract.path, Some("stage"), &input_json);
Copy link
Member

Choose a reason for hiding this comment

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

Is silently swallowing the error here intended? Intuitively should either bubble up or panic here?

executable,
error,
)
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
});
})?;

.read_to_end(&mut buffer)
.map_err(|error| -> anyhow::Result<()> {
anyhow::bail!("Failed to read recursive process input file: {:?}", error);
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
});
})?;

"Failed to read recursive process input from stdin: {:?}",
error
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
});
})?;

Add option --recursive-process-input <filename> for use with
--recursive-process to specify the name of a file to use instead of
reading from stdin.

If --debug-output-dir is set, dump the file passed to the recursive
invocation of the compiler as a JSON file suitable for use with
--recursive-process-input.

These changes are intended to support debugging the compiler and are
only available with DEBUG builds.
@xermicus xermicus merged commit bb4a4dd into paritytech:main Aug 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants