wmjb wrote: Looking through this patch, the overall structure is sound — it follows the same WinEH lowering model used by the other Windows targets — but it’s clear that the implementation is patterned after the ARM64 SEH pipeline rather than being built from the ARM32 ABI outward. That’s not inherently wrong, but it does mean a few ARM32‑specific details need closer scrutiny.
1. The lowering flow is structurally ARM64‑derived The use of CLEANUPRET, CATCHRET, and the WinEH preparation pass mirrors the ARM64 implementation almost exactly. That’s fine — LLVM’s Windows EH model is intentionally unified — but it also means the patch inherits ARM64 assumptions that don’t always map cleanly to ARM32. 2. ARM32 unwind encoding is different enough that copying the ARM64 pattern is risky The patch emits ARM32‑style .pdata/.xdata, which is correct, but the logic around prolog/epilog recognition and opcode emission still feels like a back‑port rather than a native ARM32 implementation. ARM32 unwind opcodes are more constrained, and ARM‑mode prologs differ from Thumb‑2 prologs in ways that aren’t visible in this diff. I’d want to see explicit validation that the emitter handles both modes correctly. 3. The return‑sequence selection is correct but too tightly tied to isThumb() Choosing between tBX_RET and BX_RET is the right idea, but the decision is currently keyed directly off the function’s Thumb flag. Windows ARM32 supports both ARM and Thumb code, and MSVC does generate ARM‑mode functions in real binaries. If the goal is full compatibility, the return‑opcode logic should be centralized and mode‑aware rather than scattered behind isThumb() checks. 4. No obvious ABI violations, but the personality entry path needs explicit confirmation The personality routine for ARM32 has different expectations than ARM64 — especially around saved registers and entry state. The patch doesn’t appear to violate anything, but it also doesn’t explicitly document the assumptions. Given how easy it is to accidentally inherit ARM64 semantics, I’d prefer to see a comment clarifying the expected entry conditions for ARM32. 5. The overall design is reasonable, but it still reads like an ARM64 back‑port Not in the sense of copying the wrong ABI — the unwind format is correct — but in the sense that the structure, pass ordering, and pseudo‑instruction expansion are clearly derived from the ARM64 implementation. That’s fine as long as the ARM32‑specific differences are fully accounted for. Right now, some of those differences feel implied rather than explicitly handled. https://github.com/llvm/llvm-project/pull/184953 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
