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.

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

tool.scheduler.unit_tests broken on ARM/AArch32 #7173

Open
egrimley-arm opened this issue Jan 6, 2025 · 2 comments
Open

tool.scheduler.unit_tests broken on ARM/AArch32 #7173

egrimley-arm opened this issue Jan 6, 2025 · 2 comments

Comments

@egrimley-arm
Copy link
Contributor

tool.scheduler.unit_tests seems to be broken on ARM/AArch32 since bd7f3eb (@derekbruening, May 2023).

The problem seems to be that memref_is_nop_instr calls decode in a way that always decodes as A32 so a T32 NOP instruction is not recognised. So I can make it pass by inserting if (*(uint *)record.instr.encoding == 0xe320f000) return true; into that function.

Would it make sense to rewrite memref_is_nop_instr so that instead of using the decoder it just recognises the encodings, like how next_record works? Or is there a better way?

@derekbruening
Copy link
Contributor

derekbruening commented Jan 6, 2025

There will be many usage patterns that call decode on record.instr.encoding so we need to solve that usage pattern.

E.g., the drmemtrace view tool does the exact same thing: so won't it get all the Thumb-mode instructions wrong? The same thing with the opcode_mix tool.

This affects PR #7114 which is making a library for all analyzers for decoding: @abhinav92003

The tracer makes sure to call dr_app_pc_as_jump_target on recording PC values so that raw2trace decodes them properly. What about the instruction addresses in the trace records: do those have LSB set for thumb?

Since record.instr.encoding is a field reference, we would have to add some wrapper users have to call: if the LSB is set in the record.instr.addr this wrapper could use that to also set the LSB in .encoding. We'd have to add this to memref.h and document that it should be used instead of the .encoding directly, and update all our uses.

(The module mapper decode PC would also need to be adjusted.)

(This all assumes setting the LSB is the way to go; that seems better than explicitly setting the ISA mode somehow on every single record, right?)

@derekbruening
Copy link
Contributor

If decode_from_copy() is what is already used, and record.instr.addr has LSB=1 set (does it today? seems like it should), we could make sure decode_from_copy() uses the LSB-mode-setting logic on the original PC and then not need a wrapper. We would document that anyone decoding the record.instr.encoding should use decode_from_copy().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants