Skip to content

Conversation

@Oppen
Copy link
Contributor

@Oppen Oppen commented Nov 11, 2025

ELF assemblers require a different syntax than MACH-O ones for labels.
This was breaking the Linux build for ARM.

ELF assemblers require a different syntax than MACH-O ones for labels.
This was breaking the Linux build for ARM.
Copilot AI review requested due to automatic review settings November 11, 2025 23:04
@Oppen Oppen requested a review from a team as a code owner November 11, 2025 23:04
@github-actions github-actions bot added L1 Ethereum client L2 Rollup client labels Nov 11, 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 fixes the ARM Linux build by introducing separate assembly files for ELF (Linux) and MACH-O (macOS) assemblers, which require different label reference syntax.

Key Changes:

  • Split the single ARM assembly file into platform-specific versions using conditional compilation
  • MACH-O version uses @PAGE/@PAGEOFF syntax for label references
  • ELF version uses :lo12: syntax for label references

Reviewed Changes

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

File Description
crates/common/crypto/keccak/mod.rs Added conditional compilation to include ELF assembly for Linux and MACH-O assembly for macOS
crates/common/crypto/keccak/keccak1600-armv8-macho.s New MACH-O-specific assembly file with @PAGE/@PAGEOFF label syntax for macOS
crates/common/crypto/keccak/keccak1600-armv8-elf.s New ELF-specific assembly file with :lo12: label syntax for Linux

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

Comment on lines +5 to +6
// - Removed dots from all local labels for correct detection in the frontend.
// Reason: `.L` local labels are ELF-specific.
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

The comment states "Removed dots from all local labels for correct detection in the frontend. Reason: .L local labels are ELF-specific." This is misleading in the ELF assembly file context. .L labels are indeed ELF-specific, so they should be acceptable in ELF assembly. This comment appears to be copied from the MACH-O file where it makes sense (removing ELF-specific features from MACH-O). Consider clarifying that the dots were removed for consistency or compatibility reasons, not because they're problematic in ELF.

Suggested change
// - Removed dots from all local labels for correct detection in the frontend.
// Reason: `.L` local labels are ELF-specific.
// - Removed dots from all local labels for consistent detection in the frontend and cross-platform compatibility.
// Note: `.L` local labels are ELF-specific and valid in ELF, but dots were removed for consistency across platforms and tooling.

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 | 303   | +2   |
+-------------------------------------------+-------+------+

@github-project-automation github-project-automation bot moved this to In Review in ethrex_l1 Nov 12, 2025
@ilitteri ilitteri enabled auto-merge November 12, 2025 00:40
@ilitteri ilitteri added this pull request to the merge queue Nov 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 12, 2025
@ilitteri ilitteri added this pull request to the merge queue Nov 12, 2025
Merged via the queue into main with commit f4905c2 Nov 12, 2025
38 checks passed
@ilitteri ilitteri deleted the fix/keccak-asm-arm-linux branch November 12, 2025 12:49
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Nov 12, 2025
Oppen added a commit that referenced this pull request Nov 12, 2025
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: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants