synctools: generalize asmwriter.py's AArch64-only alias/mnemonic logic - #3053
Open
StilesCrisis wants to merge 1 commit into
Open
synctools: generalize asmwriter.py's AArch64-only alias/mnemonic logic#3053StilesCrisis wants to merge 1 commit into
StilesCrisis wants to merge 1 commit into
Conversation
While attempting to regenerate PPC's AsmWriter output end-to-end (see
previous commit), hit several real bugs in this script - all in logic
that was hand-written specifically for AArch64 in 2022 and never
exercised against any other target since:
- getMnemonic/getRegisterName's "const char *AArch64InstPrinter::"
skip rule was hardcoded to the literal string "AArch64InstPrinter::",
so it silently swallowed PPC's real getRegisterName definition line
too (both match "const char *PPCInstPrinter::getRegisterName(...)"),
producing a getRegisterName body with no function signature at all.
Now excludes getRegisterName explicitly.
- getMnemonic's std::pair-returning raw signature had no matching
early-out translation for the "if (Bits == 0) return {nullptr,
Bits};" guard that newer LLVM adds - fixed to fold into the same
uint64_t-returning convention already used for the main return path.
- Newer LLVM wraps register handles in MCRegister, so getRegisterName's
raw definition now reads "getRegisterName(MCRegister Reg) { unsigned
RegNo = Reg.id(); ... }" - this leftover redeclaration line conflicted
with our hardcoded plain-unsigned-parameter signature. Now stripped.
- The injected alias-matching block's AArch64InstPrinterValidateMCOperand
call and its isSME/set_sme_index handling (AArch64 SME tile-slice
indexing syntax, e.g. "za[w8, 0]") were both hardcoded literals.
Parameterized the validator name by target; SME handling now only
applies for AArch64/ARM64, with a plain always-set_mem_access fallback
for every other target (which never has SME bracket syntax).
Each fix was confirmed necessary by direct compiler error (not
speculative) while iterating toward a from-scratch PPC rebuild. See the
previous commit for what remains unverified about that full rebuild.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
suite/synctools/asmwriter.py(the script that converts rawllvm-tblgen -gen-asm-writeroutput into capstone's C style) has AArch64-only logic hardcoded into several places, dating to when the script was written for AArch64 in 2022 and never exercised against another target since. Running it against a non-AArch64 target's raw tblgen output produced by a reasonably modern LLVM breaks in several ways - found while regenerating PowerPC's asm-writer output for #3052.Bugs fixed
getRegisterName/getMnemonic's"const char *AArch64InstPrinter::"skip rule was a literal string match, so for any other target it also matched (and silently swallowed) that target's own realgetRegisterNamedefinition line - e.g. PPC's"const char *PPCInstPrinter::getRegisterName(...)"matches the same substring pattern. Result: agetRegisterNamebody with no function signature at all. Now excludesgetRegisterNameexplicitly, and matches the target's own class name instead of a hardcodedAArch64InstPrinter.getMnemonichad no translation for newer LLVM's early-out guard (if (Bits == 0) return {nullptr, Bits};), which doesn't exist in the older LLVM output this script was originally written against.MCRegister, sogetRegisterName's raw definition now includes aRegNo = Reg.id();redeclaration line that conflicts with this script's hardcoded plain-unsigned-parameter signature.printAliasInstralias-matching block hardcodedAArch64InstPrinterValidateMCOperandand AArch64's SME tile-slice-indexing bracket syntax (za[w8, 0]) unconditionally. Validator name is now parameterized by target; SME bracket handling now only applies for AArch64/ARM64, with a plainset_mem_access()-only fallback for every other target (none of which have SME syntax).Each fix was confirmed necessary by an actual compiler/parse error while iterating toward a from-scratch PPC rebuild, not speculative.
Scope note
This is a synctools script fix, not a PPC-specific change - it generalizes logic that only worked for AArch64 to work for any target regenerated against a comparably modern LLVM. It's independent of #3052, which uses an older (LLVM 7.0.1-vintage) toolchain that doesn't hit any of these code paths at all.
One related note for anyone regenerating a target against modern LLVM specifically: if that target has zero
K_CustomAliasPatternConds (true for PPC, as of writing), the injected<Target>InstPrinterValidateMCOperandcall in the alias-matching block is unreachable but still needs a stub of that name in the target'sInstPrinter.cto link - this script doesn't generate that stub itself, since it's C source, not template output.