From 4f1d2309550003b5a6725188f7cb8ec2d6dc0267 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Tue, 7 Sep 2021 20:36:30 +0200 Subject: [PATCH 1/6] Use LLVM objdump on Macos ARM64 because it is not possible to enable TME support with otool --- crates/stdarch-test/src/disassembly.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/stdarch-test/src/disassembly.rs b/crates/stdarch-test/src/disassembly.rs index 38ebce75f7..dd80b4ad83 100644 --- a/crates/stdarch-test/src/disassembly.rs +++ b/crates/stdarch-test/src/disassembly.rs @@ -67,6 +67,24 @@ pub(crate) fn disassemble_myself() -> HashSet { String::from_utf8_lossy(Vec::leak(output.stdout)) } else if cfg!(target_os = "windows") { panic!("disassembly unimplemented") + } else if cfg!(target_os = "macos") && cfg!(target_arch = "aarch64") { + // use LLVM objdump because it is not possible to enable TME support with otool + let objdump = env::var("OBJDUMP").unwrap_or_else(|_| "objdump".to_string()); + let output = Command::new(objdump.clone()) + .arg("--disassemble") + .arg("--no-show-raw-insn") + .arg("--mattr=+crc,+crypto,+tme") + .arg(&me) + .output() + .unwrap_or_else(|_| panic!("failed to execute objdump. OBJDUMP={}", objdump)); + println!( + "{}\n{}", + output.status, + String::from_utf8_lossy(&output.stderr) + ); + assert!(output.status.success()); + + String::from_utf8_lossy(Vec::leak(output.stdout)) } else if cfg!(target_os = "macos") { let output = Command::new("otool") .arg("-vt") From c256c813ad6f7c5065e04dba68d09eaf303376d2 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Wed, 8 Sep 2021 06:51:09 +0200 Subject: [PATCH 2/6] Use objdump on Macos x86_64 as well. --- crates/stdarch-test/src/disassembly.rs | 39 +++++--------------------- 1 file changed, 7 insertions(+), 32 deletions(-) diff --git a/crates/stdarch-test/src/disassembly.rs b/crates/stdarch-test/src/disassembly.rs index dd80b4ad83..aef68e12e2 100644 --- a/crates/stdarch-test/src/disassembly.rs +++ b/crates/stdarch-test/src/disassembly.rs @@ -67,43 +67,18 @@ pub(crate) fn disassemble_myself() -> HashSet { String::from_utf8_lossy(Vec::leak(output.stdout)) } else if cfg!(target_os = "windows") { panic!("disassembly unimplemented") - } else if cfg!(target_os = "macos") && cfg!(target_arch = "aarch64") { - // use LLVM objdump because it is not possible to enable TME support with otool - let objdump = env::var("OBJDUMP").unwrap_or_else(|_| "objdump".to_string()); - let output = Command::new(objdump.clone()) - .arg("--disassemble") - .arg("--no-show-raw-insn") - .arg("--mattr=+crc,+crypto,+tme") - .arg(&me) - .output() - .unwrap_or_else(|_| panic!("failed to execute objdump. OBJDUMP={}", objdump)); - println!( - "{}\n{}", - output.status, - String::from_utf8_lossy(&output.stderr) - ); - assert!(output.status.success()); - - String::from_utf8_lossy(Vec::leak(output.stdout)) - } else if cfg!(target_os = "macos") { - let output = Command::new("otool") - .arg("-vt") - .arg(&me) - .output() - .expect("failed to execute otool"); - println!( - "{}\n{}", - output.status, - String::from_utf8_lossy(&output.stderr) - ); - assert!(output.status.success()); - - String::from_utf8_lossy(Vec::leak(output.stdout)) } else { let objdump = env::var("OBJDUMP").unwrap_or_else(|_| "objdump".to_string()); + let add_args = if cfg!(target_os = "macos") && cfg!(target_arch = "aarch64") { + // Target features need to be enabled for LLVM objdump on Macos ARM64 + vec!["--mattr=+crc,+crypto,+tme"] + } else { + vec![] + }; let output = Command::new(objdump.clone()) .arg("--disassemble") .arg("--no-show-raw-insn") + .args(add_args) .arg(&me) .output() .unwrap_or_else(|_| panic!("failed to execute objdump. OBJDUMP={}", objdump)); From 0cc4f0b7f40ffbf3d824727c2a74d816dce21804 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Wed, 8 Sep 2021 10:03:14 +0200 Subject: [PATCH 3/6] using v8.6a target feature to cover more instructions --- crates/stdarch-test/src/disassembly.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stdarch-test/src/disassembly.rs b/crates/stdarch-test/src/disassembly.rs index aef68e12e2..6164e286e2 100644 --- a/crates/stdarch-test/src/disassembly.rs +++ b/crates/stdarch-test/src/disassembly.rs @@ -71,7 +71,7 @@ pub(crate) fn disassemble_myself() -> HashSet { let objdump = env::var("OBJDUMP").unwrap_or_else(|_| "objdump".to_string()); let add_args = if cfg!(target_os = "macos") && cfg!(target_arch = "aarch64") { // Target features need to be enabled for LLVM objdump on Macos ARM64 - vec!["--mattr=+crc,+crypto,+tme"] + vec!["--mattr=+v8.6a,+crypto,+tme"] } else { vec![] }; From 87b8c76f8affa7663fc9a6abd1f62f1e9005ff16 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Wed, 8 Sep 2021 07:04:56 +0200 Subject: [PATCH 4/6] Run CI on Macos (Big Sur) again. Since otool is no longer used the OOM should be gone. --- .github/workflows/main.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c7cec5a858..516ddd49f4 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -138,9 +138,8 @@ jobs: norun: true - target: aarch64-unknown-linux-gnu os: ubuntu-latest - # Temporarily disabled because otool crashes with "Out of memory", seems Github CI issue - #- target: x86_64-apple-darwin - # os: macos-latest + - target: x86_64-apple-darwin + os: macos-11 - target: x86_64-pc-windows-msvc os: windows-latest - target: i686-pc-windows-msvc From 4ff03bab7f15706f216a8fb902666a71db8e1fe8 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Wed, 8 Sep 2021 09:55:12 +0200 Subject: [PATCH 5/6] remove assembly parsing special case for otool output (no longer needed) --- crates/stdarch-test/src/disassembly.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/crates/stdarch-test/src/disassembly.rs b/crates/stdarch-test/src/disassembly.rs index 6164e286e2..29e09ae6d7 100644 --- a/crates/stdarch-test/src/disassembly.rs +++ b/crates/stdarch-test/src/disassembly.rs @@ -125,16 +125,7 @@ fn parse(output: &str) -> HashSet { cached_header = None; break; } - let parts = if cfg!(target_os = "macos") { - // Each line of instructions should look like: - // - // $addr $instruction... - instruction - .split_whitespace() - .skip(1) - .map(std::string::ToString::to_string) - .collect::>() - } else if cfg!(target_env = "msvc") { + let parts = if cfg!(target_env = "msvc") { // Each line looks like: // // > $addr: ab cd ef $instr.. From eca3bdd7f9efdee63809bf62a3eb3ef90f94be68 Mon Sep 17 00:00:00 2001 From: Hans Kratz Date: Wed, 8 Sep 2021 15:27:42 +0200 Subject: [PATCH 6/6] Implement proper subroutine call detection for x86, x86_64, aarch64 and wasm32. --- crates/stdarch-test/src/lib.rs | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/crates/stdarch-test/src/lib.rs b/crates/stdarch-test/src/lib.rs index e5ccec216f..71920e3876 100644 --- a/crates/stdarch-test/src/lib.rs +++ b/crates/stdarch-test/src/lib.rs @@ -90,17 +90,25 @@ pub fn assert(shim_addr: usize, fnname: &str, expected: &str) { // function, e.g., tzcntl in tzcntl %rax,%rax. let found = instrs.iter().any(|s| s.starts_with(expected)); - // Look for `call` instructions in the disassembly to detect whether - // inlining failed: all intrinsics are `#[inline(always)]`, so - // calling one intrinsic from another should not generate `call` - // instructions. - let inlining_failed = instrs.windows(2).any(|s| { - // On 32-bit x86 position independent code will call itself and be - // immediately followed by a `pop` to learn about the current address. - // Let's not take that into account when considering whether a function - // failed inlining something. - s[0].contains("call") && (!cfg!(target_arch = "x86") || s[1].contains("pop")) - }); + // Look for subroutine call instructions in the disassembly to detect whether + // inlining failed: all intrinsics are `#[inline(always)]`, so calling one + // intrinsic from another should not generate subroutine call instructions. + let inlining_failed = if cfg!(target_arch = "x86_64") || cfg!(target_arch = "wasm32") { + instrs.iter().any(|s| s.starts_with("call ")) + } else if cfg!(target_arch = "x86") { + instrs.windows(2).any(|s| { + // On 32-bit x86 position independent code will call itself and be + // immediately followed by a `pop` to learn about the current address. + // Let's not take that into account when considering whether a function + // failed inlining something. + s[0].starts_with("call ") && s[1].starts_with("pop") // FIXME: original logic but does not match comment + }) + } else if cfg!(target_arch = "aarch64") { + instrs.iter().any(|s| s.starts_with("bl ")) + } else { + // FIXME: Add detection for other archs + false + }; let instruction_limit = std::env::var("STDARCH_ASSERT_INSTR_LIMIT") .ok() @@ -167,8 +175,8 @@ pub fn assert(shim_addr: usize, fnname: &str, expected: &str) { ); } else if inlining_failed { panic!( - "instruction found, but the disassembly contains `call` \ - instructions, which hint that inlining failed" + "instruction found, but the disassembly contains subroutine \ + call instructions, which hint that inlining failed" ); } }