Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x86 decoding table contains a number of incorrect categories #7410

Closed
derekbruening opened this issue Mar 28, 2025 · 1 comment · Fixed by #7411
Closed

x86 decoding table contains a number of incorrect categories #7410

derekbruening opened this issue Mar 28, 2025 · 1 comment · Fixed by #7411
Assignees
@derekbruening
Labels
Component-IR Google-Affecting

Comments

@derekbruening
Copy link
Contributor

#6238 added initial categorization to a bunch of x86 opcodes in decode_table, with load/store added dynamically to those that vary.

But later I noticed a number of categories that are incorrect:

  • OP_lea is listed as catLoad: but it does not do a load; seems like it should be catMath.
  • OP_nop_modrm is listed as catSIMD: it is unrelated to SIMD.
  • The OP_fld and OP_fst* variants that do not touch memory are still labeled catLoad or catStore.

I'm going to add a check to api.ir that the load and store categories are not present if the instruction does not do a load or store.
@derekbruening derekbruening self-assigned this Mar 28, 2025
@derekbruening
Copy link
Contributor Author

This affects the Public v2 Google Workload Traces.

derekbruening added a commit that referenced this issue Mar 28, 2025
@derekbruening
i#7410: Fix incorrect x86 ISA categories and movntdqa operands
6fb821b 
Adds a check that the load and store categories strictly match the
instr_{reads,writes}_memory() on every x86 opcode. This revealed a
number of incorrect catLoad and catStore labels which are now removed
from:
+ OP_lea (see also below)
+ OP_lahf (see also below)
+ OP_vbroadcastf32x2
+ OP_vbroadcasti32x2
+ OP_fld with only register operands
+ OP_fst{,p} with only register operands
+ OP_fld* which insert literals

Fixes {v,}movntdqa which the above check detected had its operands in
the wrong order: it was listed as a store in the table when it is in
fact a load.  Also adds catSIMD to some variants that were missing it.

Fixes the following x86 ISA category errors:
+ OP_lea is now catMath instead of catLoad
+ OP_nop_modrm is now catOther instead of catSIMD
+ OP_lahf is now catState instead of catLoad

Categorizes the following previously uncategorized opcodes:
+ OP_xchg and OP_fxchg are now catMove
+ OP_pushf is now catStore|catState
+ OP_popf is now catLoad|catState
+ OP_sahf is now catState
+ OP_cmov* is now catMove
+ OP_nop is now catOther

Fixes #7410
@derekbruening derekbruening mentioned this issue Mar 28, 2025
Merged
derekbruening added a commit that referenced this issue Mar 31, 2025
@derekbruening
i#7410: Fix incorrect x86 ISA categories and movntdqa operands (#7411)
814070f 
Adds a check that the load and store categories strictly match the
instr_{reads,writes}_memory() on every x86 opcode. This revealed a
number of incorrect catLoad and catStore labels which are now removed
from:
+ OP_lea (see also below)
+ OP_lahf (see also below)
+ OP_vbroadcastf32x2
+ OP_vbroadcasti32x2
+ OP_fld with only register operands
+ OP_fst{,p} with only register operands
+ OP_fld* which insert literals

Splits the ir_x86_{3,4}args_avx512_evex_mask files up further to avoid
OOM in 32-bit VS (xref #3992, #4610).

Fixes {v,}movntdqa which the above check detected had its operands in
the wrong order: it was listed as a store in the table when it is in
fact a load. Also adds catSIMD to some variants that were missing it.

Fixes the following x86 ISA category errors:
+ OP_lea is now catMath instead of catLoad
+ OP_nop_modrm is now catOther instead of catSIMD
+ OP_lahf is now catState instead of catLoad

Categorizes the following previously uncategorized opcodes:
+ OP_xchg and OP_fxchg are now catMove
+ OP_pushf is now catStore|catState
+ OP_popf is now catLoad|catState
+ OP_sahf is now catState
+ OP_cmov* is now catMove
+ OP_nop is now catOther

None of these changes affect other IR interfaces like
instr_{reads,write}_memory().
There is no compatibility issue with drmemtrace address traces: no raw
trace impact; the final trace is impacted for movntdqa and (for regdeps)
the categories but there's no compatibility change: just an increase in
accuracy from raw2trace or record_filter output.

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

@derekbruening derekbruening

Labels
Component-IR Google-Affecting
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

i#7410: Fix incorrect x86 ISA categories and movntdqa operands DynamoRIO/dynamorio
1 participant
@derekbruening