PowerPC: Keep BI out of the CTR-only branch predicate - #3043
Conversation
62d56d1 to
826ecb8
Compare
There was a problem hiding this comment.
Pull request overview
Fixes PowerPC branch-predicate decoding so that CTR-only BO encodings produce the correct pred_ctr regardless of BI (which selects a CR bit and must not influence CTR-only predicates), restoring the documented PPC_get_branch_pred() contract.
Changes:
- Adjust
PPC_get_branch_pred()to derive CTR predicates without mixing inBIwhenget_cr_pred == false. - Add YAML test cases covering CTR-only BO values with non-zero
BIfor bothbcandbclrencodings. - Add missing
PPC_PRED_INVALIDentry to the cstest enum-name map so expected/actual comparisons don’t silently skip that value.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/details/ppc.yaml | Adds regression tests for CTR-only branch predicate decoding with non-zero BI (bc + bclr). |
| suite/cstest/include/test_mapping.h | Adds PPC_PRED_INVALID to cs_enum_map so cstest can compare that enum value by name. |
| include/capstone/ppc.h | Fixes PPC_get_branch_pred() to keep BI out of the CTR-only predicate derivation path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fb4ac6a to
53119d2
Compare
|
Thanks |
|
Did you use an LLM to generate this PR description btw? |
Yes but I typically edit them as they're too verbose. The co-pilot summary is better, I'll aim for that level of detail next time. |
|
For the next PR please be so kind and consider more what is the minimal example showing the problem. A description like below would have saved me a lot of time understanding what was going on. This cstool command doesn't contain the BO, BI bits. Because if the PPC_BO_TEST_CR bit is set, but the BI contains bits as well, it decodes to invalid. E.g. here the Branching details are missing: |
|
Actually, I would kindly request to not write PR descriptions at all with LLMs. They are really hard to read. |
|
Will do, thanks for taking the time to show me the way. |
|
np, thanks for the fix! |
Your checklist for this pull request
Detailed description
For a BO that encodes only the counter condition,
PPC_get_branch_pred()falls through toPPC_get_no_hint_pred(((bi % 4) << 5) | bo). BI selects a CR bit and has no part in that predicate, so mixing it in lands outsideppc_predand the counter test is lost:The combined-condition branch above already gets this right — it computes
(bo | PPC_BO_TEST_CR) & ~PPC_BO_CR_CMPand notes that BI is ignored. Rather than repeat that expression, the!get_cr_predcase is hoisted above theTestCR && DecrCTRblock, so the CTR predicate is derived in one place and thectr_bo_condlocal goes away. The function's doc comment already promisesPPC_PRED_INVALIDonly when no such predicate is encoded, so this restores the documented contract rather than changing it.Test plan
One case in
tests/details/ppc.yamlcovering BO=16 and BO=18 with a non-zero BI onbc, plus abclrfor the XL form, whose BI and BO come out of a different field mask. The BO=16 BI=6bcfails against unpatchednext; the others state the invariant in the fixture rather than in one encoding.Each case also asserts
pred_cr: PPC_PRED_INVALID, so that deriving the CTR predicate in one place cannot start manufacturing a CR one. That needed the one-line map addition:PPC_PRED_INVALIDwas the onlyppc_predmember missing fromcs_enum_map, andcompare_enum_retis wrapped inif (expected), so until the name resolves the field is silently not compared at all. It is asserted only on the new cases — a combined-condition branch legitimately has both predicates, as the existingbdztla 4*cr5+eqcase shows.BO x BIspace (32x32) ofbc,bclrandbcctr— 3072 encodings: those that returnedPPC_PRED_INVALIDnow return the predicate they encode, and no existing value changes.cstest tests/goes 87971/87034/932/5 -> 87972/87035/932/5 — the same five failures, alltests/negative/, and no expectation edited.pred_crassertion does not mask thepred_ctrone it sits beside (a case stops at its first mismatch, andpred_cris compared first): with theppc.hchange reverted, the case still fails onpred_ctr.Closing issues
None