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

feat: add additional valgrind arguments support #48

Merged
merged 2 commits into from
Feb 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .github/actions/build_and_test/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ runs:
set -x
cargo install cargo2junit
[ "${RUNNER_OS}" = "Windows" ] && ADD_FLAGS="--skip debug_build_with_tests_coverage --skip build_with_sanitizers"
[ "${RUNNER_OS}" = "macOS" ] && export PATH="/opt/homebrew/opt/llvm/bin:${PATH}"
cargo test --no-fail-fast -- ${ADD_FLAGS} ${TEST_THREADS} -Z unstable-options --format json \
| cargo2junit | tee "${UNIT_TESTS_RESULTS_XML}"

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/build-and-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ jobs:
- name: Prepare MacOS environment
shell: bash
if: runner.os == 'macOS'
run: brew install cmake ninja
run: brew install cmake ninja llvm

- name: Build and test
uses: ./.github/actions/build_and_test
Expand Down
5 changes: 5 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ pub fn build(
enable_assertions: bool,
sanitizer: Option<sanitizer::Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
std::fs::create_dir_all(LLVMPath::DIRECTORY_LLVM_TARGET)?;

Expand All @@ -180,6 +181,7 @@ pub fn build(
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;
} else if target_env == target_env::TargetEnv::GNU {
platforms::x86_64_linux_gnu::build(
Expand All @@ -195,6 +197,7 @@ pub fn build(
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;
} else {
anyhow::bail!("Unsupported target environment for x86_64 and Linux");
Expand Down Expand Up @@ -246,6 +249,7 @@ pub fn build(
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;
} else if target_env == target_env::TargetEnv::GNU {
platforms::aarch64_linux_gnu::build(
Expand All @@ -261,6 +265,7 @@ pub fn build(
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;
} else {
anyhow::bail!("Unsupported target environment for aarch64 and Linux");
Expand Down
2 changes: 2 additions & 0 deletions src/platforms/aarch64_linux_gnu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub fn build(
enable_assertions: bool,
sanitizer: Option<Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
crate::utils::check_presence("cmake")?;
crate::utils::check_presence("clang")?;
Expand Down Expand Up @@ -105,6 +106,7 @@ pub fn build(
))
.args(crate::platforms::shared::shared_build_opts_valgrind(
enable_valgrind,
valgrind_options,
)),
"LLVM building cmake",
)?;
Expand Down
4 changes: 4 additions & 0 deletions src/platforms/aarch64_linux_musl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub fn build(
enable_assertions: bool,
sanitizer: Option<Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
crate::utils::check_presence("cmake")?;
crate::utils::check_presence("clang")?;
Expand Down Expand Up @@ -90,6 +91,7 @@ pub fn build(
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;

Ok(())
Expand Down Expand Up @@ -281,6 +283,7 @@ fn build_target(
enable_assertions: bool,
sanitizer: Option<Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
let mut clang_path = host_target_directory.to_path_buf();
clang_path.push("bin/clang");
Expand Down Expand Up @@ -360,6 +363,7 @@ fn build_target(
))
.args(crate::platforms::shared::shared_build_opts_valgrind(
enable_valgrind,
valgrind_options,
)),
"LLVM target building cmake",
)?;
Expand Down
16 changes: 11 additions & 5 deletions src/platforms/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,18 @@ pub fn shared_build_opts_sanitizers(sanitizer: Option<Sanitizer>) -> Vec<String>
///
/// The build options to enable Valgrind for LLVM regression tests.
///
pub fn shared_build_opts_valgrind(enabled: bool) -> Vec<String> {
if enabled {
vec!["-DLLVM_LIT_ARGS='-sv --vg --vg-leak'".to_owned()]
} else {
vec![]
pub fn shared_build_opts_valgrind(enabled: bool, valgrind_options: Vec<String>) -> Vec<String> {
if !enabled {
return vec![];
}

let vg_args = valgrind_options
.iter()
.map(|opt| format!("--vg-arg='{}'", opt))
.collect::<Vec<_>>()
.join(" ");

vec![format!("-DLLVM_LIT_ARGS='-sv --vg --vg-leak {}'", vg_args)]
}

///
Expand Down
2 changes: 2 additions & 0 deletions src/platforms/x86_64_linux_gnu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub fn build(
enable_assertions: bool,
sanitizer: Option<Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
crate::utils::check_presence("cmake")?;
crate::utils::check_presence("clang")?;
Expand Down Expand Up @@ -105,6 +106,7 @@ pub fn build(
))
.args(crate::platforms::shared::shared_build_opts_valgrind(
enable_valgrind,
valgrind_options,
)),
"LLVM building cmake",
)?;
Expand Down
4 changes: 4 additions & 0 deletions src/platforms/x86_64_linux_musl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub fn build(
enable_assertions: bool,
sanitizer: Option<Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
crate::utils::check_presence("cmake")?;
crate::utils::check_presence("clang")?;
Expand Down Expand Up @@ -91,6 +92,7 @@ pub fn build(
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;

Ok(())
Expand Down Expand Up @@ -281,6 +283,7 @@ fn build_target(
enable_assertions: bool,
sanitizer: Option<Sanitizer>,
enable_valgrind: bool,
valgrind_options: Vec<String>,
) -> anyhow::Result<()> {
let mut clang_path = host_target_directory.to_path_buf();
clang_path.push("bin/clang");
Expand Down Expand Up @@ -360,6 +363,7 @@ fn build_target(
))
.args(crate::platforms::shared::shared_build_opts_valgrind(
enable_valgrind,
valgrind_options,
)),
"LLVM target building cmake",
)?;
Expand Down
4 changes: 4 additions & 0 deletions src/zksync_llvm/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ pub enum Arguments {
/// Whether to run LLVM unit tests under valgrind or not.
#[arg(long)]
enable_valgrind: bool,

/// Additional valgrind options to pass to the valgrind command.
#[arg(long)]
valgrind_options: Vec<String>,
},

/// Checkout the branch specified in `LLVM.lock`.
Expand Down
2 changes: 2 additions & 0 deletions src/zksync_llvm/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ fn main_inner() -> anyhow::Result<()> {
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
} => {
let mut targets = targets
.into_iter()
Expand Down Expand Up @@ -103,6 +104,7 @@ fn main_inner() -> anyhow::Result<()> {
enable_assertions,
sanitizer,
enable_valgrind,
valgrind_options,
)?;
}
Arguments::Checkout { force } => {
Expand Down
Loading