Skip to content

Conversation

@Oppen
Copy link
Contributor

@Oppen Oppen commented Nov 12, 2025

Motivation

Our previous solution to build errors on Linux ARM was a "make it work" approach
that duplicated all the assembly code. Making the variable parts into parameters
seems cleaner.

Description

  • Revert "fix(l1,l2): build on ARM Linux (fix(l1,l2): build on ARM Linux #5285)";
  • Use a parameter to signal which kind of object file we're creating;
  • Add a macro to use the proper symbol resolution syntax based on this parameter;
  • Use parameters for the exported symbols, so the name mangling always matches
    the platform.

Linux and Mac linkers need different syntax to resolve `adr` (actually
`adrp+add`) arguments, and name exported symbols differently.
This patch makes a few minor changes to the ARM assembly and mod.rs for
Keccak to:
- Pass a flag indicating which kind of object file will be created;
- Use that flag in a macro to use the right symbol resolution syntax;
- Pass the exported function symbols to ensure they are correctly found
  in both OSes.
Copilot AI review requested due to automatic review settings November 12, 2025 14:19
@Oppen Oppen requested a review from a team as a code owner November 12, 2025 14:19
@github-actions github-actions bot added L1 Ethereum client L2 Rollup client labels Nov 12, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the ARM assembly code for the Keccak cryptographic hash implementation to be more portable across different platforms (Linux ELF vs macOS Mach-O). Instead of maintaining duplicate assembly files for each platform, the solution uses template parameters and conditional macros to handle platform-specific differences in a single file.

Key changes:

  • Consolidates duplicate ARM assembly files into a single parameterized file
  • Introduces an adr macro in assembly to handle different label resolution syntax between ELF (:lo12:) and Mach-O (@PAGE/@PAGEOFF)
  • Uses Rust's global_asm! template parameters to inject platform-specific symbol names

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
crates/common/crypto/keccak/mod.rs Updates global_asm! invocation to pass parameters for ELF detection and symbol names; changes visibility of State and extern functions to pub(super)
crates/common/crypto/keccak/keccak1600-armv8.s Adds adr macro for platform-specific label resolution; replaces hardcoded symbol names with template parameters; escapes curly braces in vector register syntax
crates/common/crypto/keccak/keccak1600-armv8-macho.s Removes entire duplicate Mach-O specific assembly file
Comments suppressed due to low confidence (1)

crates/common/crypto/keccak/keccak1600-armv8.s:21

  • [nitpick] The macro parameter {elf} is used in a .if conditional, but assembly .if directives typically expect numeric values. Since elf=const cfg!(target_os=\"linux\") as u32 passes either 0 or 1, this should work, but the comment in lines 7-9 describing the macro's purpose could be clearer about expecting a numeric value (0 for Mach-O, 1 for ELF) rather than just saying 'depending on the target OS'.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

#[cfg(all(target_arch = "aarch64", target_os = "macos"))]
std::arch::global_asm!(include_str!("keccak1600-armv8-macho.s"), options(raw));
#[cfg(target_arch = "aarch64")]
std::arch::global_asm!(include_str!("keccak1600-armv8.s"), elf=const cfg!(target_os="linux") as u32, SHA3_absorb=sym SHA3_absorb, SHA3_squeeze=sym SHA3_squeeze);
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The global_asm! syntax mixes a const parameter (elf) with sym parameters, but symbols passed with sym need to match the assembly template syntax. The assembly uses {SHA3_absorb} and {SHA3_squeeze} as labels, which expects the actual symbol names (potentially with platform-specific mangling like _SHA3_absorb on macOS). However, sym SHA3_absorb provides a symbol reference that may not include the platform's name mangling. Consider verifying that Rust's sym directive properly handles the platform-specific name mangling (underscore prefix on macOS) that the assembly code expects, or explicitly handle the mangling in the template parameters.

Suggested change
std::arch::global_asm!(include_str!("keccak1600-armv8.s"), elf=const cfg!(target_os="linux") as u32, SHA3_absorb=sym SHA3_absorb, SHA3_squeeze=sym SHA3_squeeze);
#[cfg(target_os = "macos")]
std::arch::global_asm!(
include_str!("keccak1600-armv8.s"),
elf = const cfg!(target_os = "linux") as u32,
SHA3_absorb = const "_SHA3_absorb",
SHA3_squeeze = const "_SHA3_squeeze"
);
#[cfg(all(target_arch = "aarch64", not(target_os = "macos")))]
std::arch::global_asm!(
include_str!("keccak1600-armv8.s"),
elf = const cfg!(target_os = "linux") as u32,
SHA3_absorb = const "SHA3_absorb",
SHA3_squeeze = const "SHA3_squeeze"
);

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

Lines of code report

Total lines added: 2
Total lines removed: 0
Total lines changed: 2

Detailed view
+-------------------------------------------+-------+------+
| File                                      | Lines | Diff |
+-------------------------------------------+-------+------+
| ethrex/crates/common/crypto/keccak/mod.rs | 305   | +2   |
+-------------------------------------------+-------+------+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client L2 Rollup client

Projects

Status: No status
Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants