search

DynamoRIO / dynamorio

Dynamic Instrumentation Tool Platform
Other
2.63k stars 557 forks source link

Update and extend existing INSTR_CREATE_nop macros #3877

Open hgreving2304 opened 5 years ago

hgreving2304 commented 5 years ago

xref #3828

This issue covers and requests updating the multi-byte nops INSTR_CREATE macros with the byte sequences as recently added by #3868

e.g. update instr_create_nbyte_nop(), INSTR_CREATE_RAW_nop2byte() etc, and add versions up to INSTR_CREATE_RAW_nop9byte()

This issue also covers adding a test to api.ir for the new macros.

ZehMatt commented 4 years ago

It appears that there are separate nops for x86 and x64 as seen below:

#ifdef X64
#    define INSTR_CREATE_RAW_nop2byte(dc) instr_create_raw_2bytes(dc, 0x66, 0x90)
#    define INSTR_CREATE_RAW_nop3byte(dc) instr_create_raw_3bytes(dc, 0x48, 0x8d, 0x3f)
#else
#    define INSTR_CREATE_RAW_nop2byte(dc) instr_create_raw_2bytes(dc, 0x8b, 0xff)
#    define INSTR_CREATE_RAW_nop3byte(dc) instr_create_raw_3bytes(dc, 0x8d, 0x7f, 0x00)
#endif

Should I just use the nops from which have been introduced lately and eliminate the condition? The comment above also states that WinDbg is apparently not happy with nops that use modrm.

hgreving2304 commented 4 years ago

I am not sure where they are coming from. We should change them into the encodings that Intel has in the Spec. See "Table 4-12. Recommended Multi-Byte Sequence of NOP Instruction" Vol. 2B 4-167.

Before doing that, could you please cross-check AMD?

ZehMatt commented 4 years ago

Ref 5.8 on https://www.amd.com/system/files/TechDocs/47414_15h_sw_opt_guide.pdf says multibyte nops are available for 32 bit and 64 bit, but it also states that CPUs older than AMD Athlon should use 90.

hgreving2304 commented 4 years ago

Thanks for looking this up! I think this is ok, I would add a comment. @derekbruening , do you think we need an extra check for old processors?

derekbruening commented 4 years ago

I filed #3897: we should add a missing up-front check for the oldest x86 processor we want to bother supporting.

hgreving2304 commented 4 years ago

@ZehMatt , you can reference the bug above in the comment. with XXX or prob. even with FIXME.

ZehMatt commented 4 years ago

Alright, so get rid of the condition and use the nops we already have in the fill function and leave a comment with the issue number. I'll start working on it the next couple days or so.

derekbruening commented 4 years ago

Let's not conflate different processor features: the plan for #3897 is to require SSE. If some of these nops were added after SSE we should either have a runtime check, or reconsider the SSE baseline.

hgreving2304 commented 4 years ago

From the AMD Spec. above it sounds like for AMD, they were added after SSE. Athlon wasn't the first one that introduced SSE...