Issue 6405056: i#751: giant CL for x86_to_x64 translation.
|
Created:
11 years, 9 months ago by yangt Modified:
5 months, 2 weeks ago CC:
dynamorio-devs_googlegroups.com Visibility:
Public. |
DescriptionReviewer: bruening@google.com
i#751: giant CL for x86_to_x64 translation.
i#751: giant CL for x86_to_x64 translation.
Add 'x86_to_x64' option for the 32-to-64 translation.
Invariant: dcontext->x86_mode always reflects the x86 mode of the
application's code.
When x86_to_x64 option is enabled, full_decode is assumed. Besides,
instr_create() always creates 64-bit instructions, instr_create_nbyte_nop()
always creates raw nops, get_shared_gencode() always returns the 64-bit
version, and far ibl always updates dcontext->x86_mode and always uses
64-bit ibl. Some assertions are relaxed to allow 32-bit dcontext and 64-bit
fragment.
The translate_x86_to_x64() function emulates known invalid/non-encodable
32-bit instructions in 64-bit. It also sets raw bits invalid for all
instructions to enforce re-encoding. The translator uses %r8.
Export reg_is_16bit(), opnd_is_reg_16bit(), get_call_return_address(), and
insert_push_retaddr() for convenience.
Add stats 'num_32bit_instrs_translated' that counts the number of 32-bit
instructions translated to 64-bit.
Add win32.x86_to_x64 into the test suite.
Patch Set 1 #
Total comments: 52
Patch Set 2 : i#751: revision based on the comments. #
Total comments: 20
Patch Set 3 : i#751: target 2nd-round review comments. #
Total comments: 4
Patch Set 4 : i#751: wrap is_wow64_process with IF_WINDOWS_ELSE and pass it to Derek. #
Total comments: 67
Patch Set 5 : i#751: giant CL for x86_to_x64 translation. #
Total comments: 16
MessagesTotal messages: 20
These are my comments. The plan we discussed in person was for me to one round of review, and then let Derek do the rest of the review. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/arch.h#newcode693 trunk/core/x86/arch.h:693: if (DYNAMO_OPTION(x86_to_x64)) The "shared_code" name doesn't indicate that it's x64 code, so it's worth a comment like, "For x86_to_x64, we always use x64 gencode." http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode1795 trunk/core/x86/mangle.c:1795: #ifdef X64 /* if X64, then this function is also used in x86_to_x64.c */ Seems like extra ifdef complexity just to limit the scope. I'd just drop the static and the corresponding arch.h ifdef around the prototypes. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode1907 trunk/core/x86/mangle.c:1907: #ifdef X64 /* if X64, then this function is also used in x86_to_x64.c */ ditto http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode2529 trunk/core/x86/mangle.c:2529: if (get_x86_mode(dcontext) && retsz == OPSZ_4 && What about retsz == 2? Does that never happen in x86_32? I don't really understand why the pop below works. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode3858 trunk/core/x86/mangle.c:3858: if (is_wow64_process(NT_CURRENT_PROCESS) && instr_get_x86_mode(instr) && This will likely break the build on Linux. I can't spot all these issues, so it's worth running the suite on a Linux machine before uploading the next diff. I'll help with that. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:16: * * Neither the name of VMware, Inc. nor the names of its contributors may be I change VMWare to Google for new files. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:61: if (instr_is_call(instr)) { My preference would be to have a translate_indirect_call() which does the push_retaddr and then calls translate_indirect_branch() to handle the rest. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:73: instr_free(dcontext, instr); For many of these instr_free, instr_set... sequences, I would try to write some kind of replace_app_instr_with(dcontext, old_instr, new_instr) so you can work with the INSTR_CREATE_* macros. It'll cost one extra temporary heap allocation, but I think it's worth it for readability. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:90: * mov r8d -> (rsp) It's kind of a corner case, but these stores can potentially fault, which means they: - need to go first (means storing to -4(%rsp)) - need to be non-meta - need to have the original translation For the register pushes, you should be able to just replace the original instruction. IIRC this is something we talked about when Derek visited. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:150: INSTR_CREATE_mov_st(dcontext, OPND_CREATE_MEM32(REG_RSP, 0), This is another special case, because we have a load and a store. Because we're using r8d, we don't need to do any state restore if the store faults but not the load. I think we'll be safe if we just move the store before the lea. I don't know how to write a test for this, but it's good to be on the safe side. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:191: INSTR_CREATE_mov_st(dcontext, OPND_CREATE_MEM32(REG_RSP, 0), ditto, store goes first http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:268: INSTR_CREATE_mov_st(dcontext, OPND_CREATE_MEM16(REG_RSP, 14), Rather than copy pasting all this instr creation, I'd prefer to have a pusha_registers[] array that we iterate over, and create stores at the appropriate offsets. You'll have to use the opnd_create_base_disp() instead of OPND_CREATE_MEMNN() in order to pass the right OPSZ. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:330: /* translate: popa/popad => mov 14(rsp)/28(rsp) -> r8w/r8d This first instr is to probe the stack? The result is immediately killed by the next load from (rsp) to r8. If it's necessary, it deserves a comment. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:422: INSTR_CREATE_jcc_short(dcontext, OP_jno_short, Derek, are there any complications internal to DR if a meta-instruction reads the app flags? I think this is OK. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:498: /* translate: pushfd => mov (rsp) -> r8d Worth a comment that this sort of violates app transparency, since we're assuming we can read and write the top-of-stack. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:577: case OP_leave: Leave brings to mind the enter instruction. Don't implement it in this CL, but put in an ASSERT_NOT_IMPLEMENTED() for it. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... File trunk/suite/tests/win32/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:1: /* ********************************************************** This is just mixed_mode.c, right? I'd really rather not duplicate this file just to remove the daa instruction tests. Can we add a command line flag to mixed_mode.c and have that control the daa tests? Also, I can't see the true diff vs mixed_mode.c. There are args to git diff to have it do copy detection, which will show the proper base file. Maybe we should add those to Derek's code review bash function. My threshold is set too low, so I often get this accidentally.
Sign in to reply to this message.
On Wed, Jul 18, 2012 at 12:45 PM, <rnk@google.com> wrote: > The plan we discussed in person was for me to one round of review, and > then let Derek do the rest of the review. > Noone discussed that with me (!) and I've already started reviewing. I will stop and publish my comments at this point and wait for 2nd round then.
Sign in to reply to this message.
started review (with the test) before your plan to ask me to review a later round and not this round was communicated so sending out these test comments http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:33: /* file "x86_to_x64.c" */ add brief description http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... File trunk/suite/tests/win32/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:33: /* Built as 32-bit and then run on WOW64 with 64-bit DR to test mixed-mode update comment http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:54: test_top_bits(); this is identical to mixedmode.c. better to run mixedmode test with x86_to_x64 option than to duplicate its code here. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:55: print("r8 was 0x%I64x\n", *(__int64*)global_data); %I is windows-only. could use PFMT from tools.h http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:58: print("edx was 0x%I32x\n", *(__int32*)global_data); %I is windows-only. ditto below. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:72: print("test_iret() returned %d\n", test_iret()); also identical: see earlier comment http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:74: print("test_far_calls() returned %d\n", test_far_calls()); also identical: see earlier comment http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:100: # define SWITCH_32_TO_64(label) \ all this is duplicated from mixedmode.c. if test_top_bits and the far call and iret routines are removed and we run mixedmode itself then the duplication should go away.
Sign in to reply to this message.
On Wed, Jul 18, 2012 at 12:48 PM, Derek Bruening <bruening@google.com>wrote: > On Wed, Jul 18, 2012 at 12:45 PM, <rnk@google.com> wrote: > >> The plan we discussed in person was for me to one round of review, and >> then let Derek do the rest of the review. >> > > Noone discussed that with me (!) and I've already started reviewing. I > will stop and publish my comments at this point and wait for 2nd round then. > Yeah, that was partly my bad, this went out at the end of the day, and this morning I just jumped into reviewing instead of making that clear.
Sign in to reply to this message.
Oops I'm sorry I misunderstood your intention and made this confusion. I'll take a look at your comments. On Wed, Jul 18, 2012 at 12:50 PM, Reid Kleckner <rnk@google.com> wrote: > On Wed, Jul 18, 2012 at 12:48 PM, Derek Bruening <bruening@google.com>wrote: > >> On Wed, Jul 18, 2012 at 12:45 PM, <rnk@google.com> wrote: >> >>> The plan we discussed in person was for me to one round of review, and >>> then let Derek do the rest of the review. >>> >> >> Noone discussed that with me (!) and I've already started reviewing. I >> will stop and publish my comments at this point and wait for 2nd round then. >> > > Yeah, that was partly my bad, this went out at the end of the day, and > this morning I just jumped into reviewing instead of making that clear. > -- Yang Tang | Software Engineering Intern | yangt@google.com | US-NYC-9TH 12F104F
Sign in to reply to this message.
http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/arch.h#newcode693 trunk/core/x86/arch.h:693: if (DYNAMO_OPTION(x86_to_x64)) On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > The "shared_code" name doesn't indicate that it's x64 code, so it's worth a > comment like, "For x86_to_x64, we always use x64 gencode." Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode1795 trunk/core/x86/mangle.c:1795: #ifdef X64 /* if X64, then this function is also used in x86_to_x64.c */ On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > Seems like extra ifdef complexity just to limit the scope. I'd just drop the > static and the corresponding arch.h ifdef around the prototypes. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode1907 trunk/core/x86/mangle.c:1907: #ifdef X64 /* if X64, then this function is also used in x86_to_x64.c */ On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > ditto Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode2529 trunk/core/x86/mangle.c:2529: if (get_x86_mode(dcontext) && retsz == OPSZ_4 && On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > What about retsz == 2? Does that never happen in x86_32? > > I don't really understand why the pop below works. According to Intel's manual, it seems to me that x64 is able to do a 2-byte pop, which adds RSP by 2 as a result. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode3858 trunk/core/x86/mangle.c:3858: if (is_wow64_process(NT_CURRENT_PROCESS) && instr_get_x86_mode(instr) && On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > This will likely break the build on Linux. I can't spot all these issues, so > it's worth running the suite on a Linux machine before uploading the next diff. > I'll help with that. I think on Linux there's no test case with x86_to_x64 option turned on? http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:16: * * Neither the name of VMware, Inc. nor the names of its contributors may be On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > I change VMWare to Google for new files. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:33: /* file "x86_to_x64.c" */ On 2012/07/18 16:50:16, bruening wrote: > add brief description Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:61: if (instr_is_call(instr)) { On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > My preference would be to have a translate_indirect_call() which does the > push_retaddr and then calls translate_indirect_branch() to handle the rest. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:73: instr_free(dcontext, instr); On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > For many of these instr_free, instr_set... sequences, I would try to write some > kind of replace_app_instr_with(dcontext, old_instr, new_instr) so you can work > with the INSTR_CREATE_* macros. > > It'll cost one extra temporary heap allocation, but I think it's worth it for > readability. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:90: * mov r8d -> (rsp) On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > It's kind of a corner case, but these stores can potentially fault, which means > they: > - need to go first (means storing to -4(%rsp)) > - need to be non-meta > - need to have the original translation > > For the register pushes, you should be able to just replace the original > instruction. > > IIRC this is something we talked about when Derek visited. Since there's no stack red zone for 32-bit, is there any potential danger of writing beyond the stack pointer? Also, I changed the PRE and POST macros to non-meta. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:150: INSTR_CREATE_mov_st(dcontext, OPND_CREATE_MEM32(REG_RSP, 0), On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > This is another special case, because we have a load and a store. Because we're > using r8d, we don't need to do any state restore if the store faults but not the > load. I think we'll be safe if we just move the store before the lea. > > I don't know how to write a test for this, but it's good to be on the safe side. Ditto. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:191: INSTR_CREATE_mov_st(dcontext, OPND_CREATE_MEM32(REG_RSP, 0), On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > ditto, store goes first Ditto. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:268: INSTR_CREATE_mov_st(dcontext, OPND_CREATE_MEM16(REG_RSP, 14), On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > Rather than copy pasting all this instr creation, I'd prefer to have a > pusha_registers[] array that we iterate over, and create stores at the > appropriate offsets. You'll have to use the opnd_create_base_disp() instead of > OPND_CREATE_MEMNN() in order to pass the right OPSZ. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:330: /* translate: popa/popad => mov 14(rsp)/28(rsp) -> r8w/r8d On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > This first instr is to probe the stack? The result is immediately killed by the > next load from (rsp) to r8. > > If it's necessary, it deserves a comment. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:422: INSTR_CREATE_jcc_short(dcontext, OP_jno_short, On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > Derek, are there any complications internal to DR if a meta-instruction reads > the app flags? I think this is OK. I changed PRE and POST macros to non-meta. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:498: /* translate: pushfd => mov (rsp) -> r8d On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > Worth a comment that this sort of violates app transparency, since we're > assuming we can read and write the top-of-stack. Done. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:577: case OP_leave: On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > Leave brings to mind the enter instruction. Don't implement it in this CL, but > put in an ASSERT_NOT_IMPLEMENTED() for it. Done. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... File trunk/suite/tests/win32/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:1: /* ********************************************************** On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > This is just mixed_mode.c, right? I'd really rather not duplicate this file > just to remove the daa instruction tests. > > Can we add a command line flag to mixed_mode.c and have that control the daa > tests? > > Also, I can't see the true diff vs mixed_mode.c. There are args to git diff to > have it do copy detection, which will show the proper base file. Maybe we > should add those to Derek's code review bash function. My threshold is set too > low, so I often get this accidentally. I merged this file into mixedmode.c and removed this file. If cmdline arg is "x86_to_x64", then we skip the daa. Also added test_push_word and test_pop_word. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:33: /* Built as 32-bit and then run on WOW64 with 64-bit DR to test mixed-mode On 2012/07/18 16:50:16, bruening wrote: > update comment Done. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:54: test_top_bits(); On 2012/07/18 16:50:16, bruening wrote: > this is identical to mixedmode.c. better to run mixedmode test with x86_to_x64 > option than to duplicate its code here. Done. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:55: print("r8 was 0x%I64x\n", *(__int64*)global_data); On 2012/07/18 16:50:16, bruening wrote: > %I is windows-only. could use PFMT from tools.h Cannot directly use PFMT since it's 32-bit. I used 0x%016"INT64_FORMAT"x instead. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:58: print("edx was 0x%I32x\n", *(__int32*)global_data); On 2012/07/18 16:50:16, bruening wrote: > %I is windows-only. ditto below. Done. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:72: print("test_iret() returned %d\n", test_iret()); On 2012/07/18 16:50:16, bruening wrote: > also identical: see earlier comment Done. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:74: print("test_far_calls() returned %d\n", test_far_calls()); On 2012/07/18 16:50:16, bruening wrote: > also identical: see earlier comment Done. http://codereview.appspot.com/6405056/diff/1/trunk/suite/tests/win32/x86_to_x... trunk/suite/tests/win32/x86_to_x64.c:100: # define SWITCH_32_TO_64(label) \ On 2012/07/18 16:50:16, bruening wrote: > all this is duplicated from mixedmode.c. if test_top_bits and the far call and > iret routines are removed and we run mixedmode itself then the duplication > should go away. Done.
Sign in to reply to this message.
Pretty good. Let's do one more iteration, and then I think it'll be ready for Derek to take a look. http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/mangle.c#newcode3858 trunk/core/x86/mangle.c:3858: if (is_wow64_process(NT_CURRENT_PROCESS) && instr_get_x86_mode(instr) && On 2012/07/19 20:23:11, yangt wrote: > On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > > This will likely break the build on Linux. I can't spot all these issues, so > > it's worth running the suite on a Linux machine before uploading the next > diff. > > I'll help with that. > > I think on Linux there's no test case with x86_to_x64 option turned on? I meant, is_wow64_process() is not available on Linux, so if will not be able to compile at all. You have to do something like IF_WINDOWS_ELSE(win_expr, lin_expr). http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... trunk/core/x86/x86_to_x64.c:90: * mov r8d -> (rsp) On 2012/07/19 20:23:11, yangt wrote: > On 2012/07/18 16:45:45, Reid Kleckner (google) wrote: > > It's kind of a corner case, but these stores can potentially fault, which > means > > they: > > - need to go first (means storing to -4(%rsp)) > > - need to be non-meta > > - need to have the original translation > > > > For the register pushes, you should be able to just replace the original > > instruction. > > > > IIRC this is something we talked about when Derek visited. > > Since there's no stack red zone for 32-bit, is there any potential danger of > writing beyond the stack pointer? There is existing code in Windows dlls that writes -4 bytes past the top of the stack and there is no pre-emption, so this should be safe. On Linux, we use an alternate stack to receive signals, and then we delay them until a bb boundary if we can. If the signal can't be delayed (SIGSEGV etc), we look up the current app PC create a new fragment, which will start on an app instr boundary. Therefore we don't have to worry about being pre-empted between these two instructions. > Also, I changed the PRE and POST macros to non-meta. I'll let Derek confirm whether all of these should be app or meta. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/arch.h#newcode941 trunk/core/x86/arch.h:941: #ifdef X64 Can move this ifdef down to just before x86_to_x64.c. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:23: * ARE DISCLAIMED. IN NO EVENT SHALL VMWARE, INC. OR CONTRIBUTORS BE LIABLE I should have pointed out the other instance of VMWARE here. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:106: /* 4-byte push */ Worth an assertion that opnd_get_size(mem) == OPSZ_4. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:295: for (i = 7; i >= 0; --i) { You don't need to have two for loops. Before the loop, call opnd_get_size_in_bytes() and use that instead of "* 2" and "* 4". You can use reg_32_to_opsz(pushad_registers[i], opsz) to get the right register. Similarly, the "lea -32/-16" can be "lea -8 * opsz_bytes" http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:328: INSTR_CREATE_mov_ld(dcontext, opnd_create_reg(REG_R8W), These two instructions are: mov (rsp) -> r8w mov r8w -> di Why do we do that instead of mov (rsp) -> di? Then you can just start the loop at 0. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:334: if (i == 3) /* skip sp */ I'd rather write "pushad_registers[i] == REG_ESP", since that's readable and guaranteed to be correct regardless of the order of pushad. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:357: for (i = 1; i < 8; ++i) { Same here about combining the two for loops. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:450: * which may violate app transparency. I'd go further and outline the specific case that might break: "this may fault or create a race if esp is pointing to the base of an empty stack." http://codereview.appspot.com/6405056/diff/8002/trunk/suite/tests/win32/mixed... File trunk/suite/tests/win32/mixedmode.c (right): http://codereview.appspot.com/6405056/diff/8002/trunk/suite/tests/win32/mixed... trunk/suite/tests/win32/mixedmode.c:36: * If cmdline arg is "x86_to_x64", we avoid using daa. "avoid using instructions we can't translate, such as daa." http://codereview.appspot.com/6405056/diff/8002/trunk/suite/tests/win32/mixed... trunk/suite/tests/win32/mixedmode.c:84: global_data[0] = 0; This gets overwritten, so I wouldn't use it. You can create another global and read it in the asm using DECL_EXTERN.
Sign in to reply to this message.
http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/arch.h#newcode941 trunk/core/x86/arch.h:941: #ifdef X64 On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > Can move this ifdef down to just before x86_to_x64.c. Done. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:23: * ARE DISCLAIMED. IN NO EVENT SHALL VMWARE, INC. OR CONTRIBUTORS BE LIABLE On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > I should have pointed out the other instance of VMWARE here. Done. Sorry about my careless. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:106: /* 4-byte push */ On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > Worth an assertion that opnd_get_size(mem) == OPSZ_4. Done. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:295: for (i = 7; i >= 0; --i) { On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > You don't need to have two for loops. > > Before the loop, call opnd_get_size_in_bytes() and use that instead of "* 2" and > "* 4". > > You can use reg_32_to_opsz(pushad_registers[i], opsz) to get the right register. > > Similarly, the "lea -32/-16" can be "lea -8 * opsz_bytes" Done. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:328: INSTR_CREATE_mov_ld(dcontext, opnd_create_reg(REG_R8W), On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > These two instructions are: > mov (rsp) -> r8w > mov r8w -> di > > Why do we do that instead of mov (rsp) -> di? > > Then you can just start the loop at 0. Done. You're right. It was a unnecessary residue of refactoring. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:334: if (i == 3) /* skip sp */ On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > I'd rather write "pushad_registers[i] == REG_ESP", since that's readable and > guaranteed to be correct regardless of the order of pushad. Done. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:357: for (i = 1; i < 8; ++i) { On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > Same here about combining the two for loops. Done. http://codereview.appspot.com/6405056/diff/8002/trunk/core/x86/x86_to_x64.c#n... trunk/core/x86/x86_to_x64.c:450: * which may violate app transparency. On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > I'd go further and outline the specific case that might break: > "this may fault or create a race if esp is pointing to the base of an empty > stack." Done. http://codereview.appspot.com/6405056/diff/8002/trunk/suite/tests/win32/mixed... File trunk/suite/tests/win32/mixedmode.c (right): http://codereview.appspot.com/6405056/diff/8002/trunk/suite/tests/win32/mixed... trunk/suite/tests/win32/mixedmode.c:36: * If cmdline arg is "x86_to_x64", we avoid using daa. On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > "avoid using instructions we can't translate, such as daa." Done. http://codereview.appspot.com/6405056/diff/8002/trunk/suite/tests/win32/mixed... trunk/suite/tests/win32/mixedmode.c:84: global_data[0] = 0; On 2012/07/20 15:23:47, Reid Kleckner (google) wrote: > This gets overwritten, so I wouldn't use it. You can create another global and > read it in the asm using DECL_EXTERN. Done.
Sign in to reply to this message.
LGTM, but let Derek should take a look. You missed my two reply comments on the first patchset. Unfortunately, Rietveld's interface isn't that good and it's easy to miss them. If you click the "1" link instead of "view", it will show the diff between the two patchsets and you can see reply comments. http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:3851: if (is_wow64_process(NT_CURRENT_PROCESS) && instr_get_x86_mode(instr) && This still won't build on Linux. http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:110: /* translate: push reg32 => lea -4(rsp) -> rsp I still think the store needs to go first. See this comment: http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... You can take it up with Derek, though.
Sign in to reply to this message.
http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:3851: if (is_wow64_process(NT_CURRENT_PROCESS) && instr_get_x86_mode(instr) && On 2012/07/20 19:05:06, Reid Kleckner (google) wrote: > This still won't build on Linux. Done. http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/11004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:110: /* translate: push reg32 => lea -4(rsp) -> rsp On 2012/07/20 19:05:06, Reid Kleckner (google) wrote: > I still think the store needs to go first. See this comment: > http://codereview.appspot.com/6405056/diff/1/trunk/core/x86/x86_to_x64.c#newc... > > You can take it up with Derek, though. I'm not very familiar with the fault-handling stuff. I'd let Derek decide.
Sign in to reply to this message.
The current code translates "push esp" incorrectly. See inline comments. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:112: */ Just found that there's a severe flaw here. It handles "push esp" incorrectly. One way to solve it is to swap the two instructions. Another way is: mov esp -> r8d; lea -4(rsp) -> rsp; mov r8d -> (rsp).
Sign in to reply to this message.
main concern is that in some places you assume that absolutely everything will be translated. what about the BCD instrs and other challenges? are you ruling out any solution for those that leaves them as 32-bit, perhaps in an isolated one-instr bb? http://codereview.appspot.com/6405056/diff/14004/trunk/core/CMakeLists.txt File trunk/core/CMakeLists.txt (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/CMakeLists.txt#ne... trunk/core/CMakeLists.txt:122: list(APPEND ARCH_SRCS x86/x86_to_x64.c) not the idiom used in rest of our cmake code: our style is "set(ARCH_SRCS ${ARCH_SRCS} x86/x86_to_x64.c)" http://codereview.appspot.com/6405056/diff/14004/trunk/core/optionsx.h File trunk/core/optionsx.h (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/optionsx.h#newcod... trunk/core/optionsx.h:508: OPTION_DEFAULT_INTERNAL(bool, x86_to_x64, false, bug: you're using DYNAMO_OPTION yet you declared it as an internal option which requires INTERNAL_OPTION. simplest to just making it an external option (remove _INTERNAL). http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:693: /* For x86_to_x64, we always use x64 gencode. */ how are you going to handle instrs that you can't easily translate, like BCD instrs? if the plan is to leave their containing fragments as 32-bit, won't you need x86 gencode? or do you have a different plan? http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:952: translate_x86_to_x64(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, label instr as OUT (assuming it is OUT) http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c File trunk/core/x86/emit_utils.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:5967: * For x86_to_x64, we should always update dcontext->x86_mode and I don't understand what you mean: why would you change x86_mode if the actual mode didn't change? http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:6003: /* x86_to_x64: always update dcontext->x86_mode */ again, I don't understand why you need to do this? the app mode hasn't changed http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:6040: /* x86_to_x64: always use 64-bit ibl */ and do not change modes. though once again, what is your plan for BCD instrs? truncate bb after them, so never have far cti in 32-bit fragment? http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c File trunk/core/x86/instr.c (left): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c#oldco... trunk/core/x86/instr.c:61: # undef ASSERT /* N.B.: if have issues w/ DYNAMO_OPTION, re-instate */ add comment like in decode.c: /* we can't undef ASSERT b/c of DYNAMO_OPTION */ http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c File trunk/core/x86/instr.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c#newco... trunk/core/x86/instr.c:1677: : get_x86_mode(dcontext))); this needs to be clearly documented for clients to avoid confusion http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c#newco... trunk/core/x86/instr.c:5026: if (raw || DYNAMO_OPTION(x86_to_x64)) { this needs a comment explaining http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.h File trunk/core/x86/instr.h (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.h#newco... trunk/core/x86/instr.h:1270: reg_is_16bit(reg_id_t reg); add to feature list in api/docs/release.dox http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.h#newco... trunk/core/x86/instr.h:1278: opnd_is_reg_16bit(opnd_t opnd); ditto http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/interp.c File trunk/core/x86/interp.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/interp.c#newc... trunk/core/x86/interp.c:2723: IF_X64(|| DYNAMO_OPTION(coarse_split_riprel) || DYNAMO_OPTION(x86_to_x64)) add comment explaining why http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/interp.c#newc... trunk/core/x86/interp.c:4716: if (!X64_MODE_DC(dcontext) && !DYNAMO_OPTION(x86_to_x64)) { hmmm, how are you going to handle a 32-bit fragment (from BCD)? http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:1811: !DYNAMO_OPTION(x86_to_x64)))) { maybe there should be a notion of "code cache mode" to avoid having option checks at all the callers: X64_CACHE_MODE_DC() or sthg http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:2523: DYNAMO_OPTION(x86_to_x64)) { check option first to avoid work when off http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:2529: OPND_CREATE_MEM_lea(REG_RSP, REG_NULL, 0, 4))); this code is equivalent to the iret/lret above. it should be a smaller code change to add to the if() above and leverage its code rather than having extra code here and extra indent below. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:2617: if ((X64_MODE_DC(dcontext) || DYNAMO_OPTION(x86_to_x64)) && retsz == OPSZ_4) { a downside of having mode not change is all these "|| option" changes. see prior proposal of X64_CACHE_MODE_DC(). once nice thing about that is that if BCD ends up w/ 32-bit frags for -x86_to_x64 you can hide whatever complexity (3-way mode?) inside there. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:3852: instr_get_x86_mode(instr) && DYNAMO_OPTION(x86_to_x64)) put the option first, for two reasons: one, to avoid extra work when it's off; and two, to label this as being specific to that option. when last it looks like this code is not owned by that option. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:33: /* file "x86_to_x64.c" -- translate 32-bit instr to 64-bit can you add comments here on the general tradeoff of fault handling vs beyond-TOS and your approach here for replacing one app instr with multiple http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:33: /* file "x86_to_x64.c" -- translate 32-bit instr to 64-bit also add comments on how you assume you can freely use r8, but not any other registers http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:33: /* file "x86_to_x64.c" -- translate 32-bit instr to 64-bit plus you haven't yet added fault translation support: can you add a comment here, and add to the list under "Current status" for "FAULT TRANSLATION" in arch.c http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:52: replace(dcontext_t *dcontext, instrlist_t *ilist, instr_t **old, instr_t *new) mark as INOUT http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:61: translate_indirect_branch(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, mark as INOUT http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:61: translate_indirect_branch(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, _jump would be clearer (more consistent w/ DR names) http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:70: INSTR_CREATE_movzx(dcontext, opnd_create_reg(REG_R8D), target)); you're not setting the translation. do you assume that instrlist_set_translation_target() has been called? can you add a top-level comment to that effect? http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:104: /* 4-byte push */ add FIXME comment about fault handling http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:112: */ On 2012/07/30 15:01:16, yangt wrote: > Just found that there's a severe flaw here. It handles "push esp" incorrectly. > One way to solve it is to swap the two instructions. Another way is: mov esp -> > r8d; lea -4(rsp) -> rsp; mov r8d -> (rsp). ties into fault handling vs beyond-TOS for which one to pick http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:170: */ add FIXME comment about fault handling http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:273: */ add FIXME about fault handling http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:461: translate_x86_to_x64(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, INOUT http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:461: translate_x86_to_x64(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, add comment that you assume that the caller will call instrlist_set_translation_target() when translations are needed. do you also assume instrlist_set_our_mangling() has been called? can you add an assert for that? http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... File trunk/suite/tests/win32/mixedmode.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... trunk/suite/tests/win32/mixedmode.c:170: push byte ptr HEX(ff) you can do "pushw" and thus not need the RAW(66) http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... trunk/suite/tests/win32/mixedmode.c:293: /* skip daa if global_data[0] == 0 */ you mean if is_x86_to_x64 == 0 http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... trunk/suite/tests/win32/mixedmode.c:363: /* skip daa if global_data[0] == 0 */ ditto
Sign in to reply to this message.
http://codereview.appspot.com/6405056/diff/14004/trunk/core/CMakeLists.txt File trunk/core/CMakeLists.txt (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/CMakeLists.txt#ne... trunk/core/CMakeLists.txt:122: list(APPEND ARCH_SRCS x86/x86_to_x64.c) On 2012/07/31 03:21:05, bruening wrote: > not the idiom used in rest of our cmake code: our style is "set(ARCH_SRCS > ${ARCH_SRCS} x86/x86_to_x64.c)" Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/optionsx.h File trunk/core/optionsx.h (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/optionsx.h#newcod... trunk/core/optionsx.h:508: OPTION_DEFAULT_INTERNAL(bool, x86_to_x64, false, On 2012/07/31 03:21:05, bruening wrote: > bug: you're using DYNAMO_OPTION yet you declared it as an internal option which > requires INTERNAL_OPTION. simplest to just making it an external option (remove > _INTERNAL). Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:693: /* For x86_to_x64, we always use x64 gencode. */ On 2012/07/31 03:21:05, bruening wrote: > how are you going to handle instrs that you can't easily translate, like BCD > instrs? if the plan is to leave their containing fragments as 32-bit, won't you > need x86 gencode? or do you have a different plan? Added a comment. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:952: translate_x86_to_x64(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, On 2012/07/31 03:21:05, bruening wrote: > label instr as OUT (assuming it is OUT) should it be INOUT? http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c File trunk/core/x86/emit_utils.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:5967: * For x86_to_x64, we should always update dcontext->x86_mode and On 2012/07/31 03:21:05, bruening wrote: > I don't understand what you mean: why would you change x86_mode if the actual > mode didn't change? Updated my reason in comments. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:6003: /* x86_to_x64: always update dcontext->x86_mode */ On 2012/07/31 03:21:05, bruening wrote: > again, I don't understand why you need to do this? the app mode hasn't changed see previous comment. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:6040: /* x86_to_x64: always use 64-bit ibl */ On 2012/07/31 03:21:05, bruening wrote: > and do not change modes. though once again, what is your plan for BCD instrs? > truncate bb after them, so never have far cti in 32-bit fragment? see previous comment. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c File trunk/core/x86/instr.c (left): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c#oldco... trunk/core/x86/instr.c:61: # undef ASSERT /* N.B.: if have issues w/ DYNAMO_OPTION, re-instate */ On 2012/07/31 03:21:05, bruening wrote: > add comment like in decode.c: > /* we can't undef ASSERT b/c of DYNAMO_OPTION */ Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c File trunk/core/x86/instr.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c#newco... trunk/core/x86/instr.c:1677: : get_x86_mode(dcontext))); On 2012/07/31 03:21:05, bruening wrote: > this needs to be clearly documented for clients to avoid confusion Done in arch_exports.h. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.c#newco... trunk/core/x86/instr.c:5026: if (raw || DYNAMO_OPTION(x86_to_x64)) { On 2012/07/31 03:21:05, bruening wrote: > this needs a comment explaining Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.h File trunk/core/x86/instr.h (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.h#newco... trunk/core/x86/instr.h:1270: reg_is_16bit(reg_id_t reg); On 2012/07/31 03:21:05, bruening wrote: > add to feature list in api/docs/release.dox I find that reg_is_16bit and opnd_is_reg_16bit are no longer needed due to a previous refactoring of x86_to_x64.c. So I just removed these two functions. Also removed the corresponding implementations in instr.c. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/instr.h#newco... trunk/core/x86/instr.h:1278: opnd_is_reg_16bit(opnd_t opnd); On 2012/07/31 03:21:05, bruening wrote: > ditto ditto http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/interp.c File trunk/core/x86/interp.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/interp.c#newc... trunk/core/x86/interp.c:2723: IF_X64(|| DYNAMO_OPTION(coarse_split_riprel) || DYNAMO_OPTION(x86_to_x64)) On 2012/07/31 03:21:05, bruening wrote: > add comment explaining why Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/interp.c#newc... trunk/core/x86/interp.c:4716: if (!X64_MODE_DC(dcontext) && !DYNAMO_OPTION(x86_to_x64)) { On 2012/07/31 03:21:05, bruening wrote: > hmmm, how are you going to handle a 32-bit fragment (from BCD)? Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c File trunk/core/x86/mangle.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:1811: !DYNAMO_OPTION(x86_to_x64)))) { On 2012/07/31 03:21:05, bruening wrote: > maybe there should be a notion of "code cache mode" to avoid having option > checks at all the callers: X64_CACHE_MODE_DC() or sthg Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:2523: DYNAMO_OPTION(x86_to_x64)) { On 2012/07/31 03:21:05, bruening wrote: > check option first to avoid work when off n/a after refactoring. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:2529: OPND_CREATE_MEM_lea(REG_RSP, REG_NULL, 0, 4))); On 2012/07/31 03:21:05, bruening wrote: > this code is equivalent to the iret/lret above. it should be a smaller code > change to add to the if() above and leverage its code rather than having extra > code here and extra indent below. Done. I think it's unnecessary to check if the opcode is OP_iret or OP_ret_far. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:2617: if ((X64_MODE_DC(dcontext) || DYNAMO_OPTION(x86_to_x64)) && retsz == OPSZ_4) { On 2012/07/31 03:21:05, bruening wrote: > a downside of having mode not change is all these "|| option" changes. see > prior proposal of X64_CACHE_MODE_DC(). once nice thing about that is that if > BCD ends up w/ 32-bit frags for -x86_to_x64 you can hide whatever complexity > (3-way mode?) inside there. Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/mangle.c#newc... trunk/core/x86/mangle.c:3852: instr_get_x86_mode(instr) && DYNAMO_OPTION(x86_to_x64)) On 2012/07/31 03:21:05, bruening wrote: > put the option first, for two reasons: one, to avoid extra work when it's off; > and two, to label this as being specific to that option. when last it looks > like this code is not owned by that option. Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:33: /* file "x86_to_x64.c" -- translate 32-bit instr to 64-bit On 2012/07/31 03:21:05, bruening wrote: > also add comments on how you assume you can freely use r8, but not any other > registers Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:52: replace(dcontext_t *dcontext, instrlist_t *ilist, instr_t **old, instr_t *new) On 2012/07/31 03:21:05, bruening wrote: > mark as INOUT Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:61: translate_indirect_branch(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, On 2012/07/31 03:21:05, bruening wrote: > mark as INOUT Done. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:70: INSTR_CREATE_movzx(dcontext, opnd_create_reg(REG_R8D), target)); On 2012/07/31 03:21:05, bruening wrote: > you're not setting the translation. do you assume that > instrlist_set_translation_target() has been called? can you add a top-level > comment to that effect? Modified "pre" to set translation. This involves changing a lot of places in this file. Please kindly review them. Thanks:) http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:104: /* 4-byte push */ On 2012/07/31 03:21:05, bruening wrote: > add FIXME comment about fault handling See comments at the beginning of this file. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:112: */ On 2012/07/31 03:21:05, bruening wrote: > On 2012/07/30 15:01:16, yangt wrote: > > Just found that there's a severe flaw here. It handles "push esp" incorrectly. > > One way to solve it is to swap the two instructions. Another way is: mov esp > -> > > r8d; lea -4(rsp) -> rsp; mov r8d -> (rsp). > > ties into fault handling vs beyond-TOS for which one to pick Done. Also fixed similar bugs for "pop esp" and "pop [esp]". Also added a test case in win32.mixedmode. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:170: */ On 2012/07/31 03:21:05, bruening wrote: > add FIXME comment about fault handling See comments at the beginning of this file. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:273: */ On 2012/07/31 03:21:05, bruening wrote: > add FIXME about fault handling See comments at the beginning of this file. http://codereview.appspot.com/6405056/diff/14004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:461: translate_x86_to_x64(dcontext_t *dcontext, instrlist_t *ilist, instr_t **instr, On 2012/07/31 03:21:05, bruening wrote: > INOUT After refactoring, now I don't assume that the caller calls instrlist_set_translation_target(). This function is only called in mangle(), where instrlist_set_our_mangling() has been called. I added an assertion. http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... File trunk/suite/tests/win32/mixedmode.c (right): http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... trunk/suite/tests/win32/mixedmode.c:170: push byte ptr HEX(ff) On 2012/07/31 03:21:05, bruening wrote: > you can do "pushw" and thus not need the RAW(66) I can't find a way. PUSHW byte ptr HEX(ff) still pushs word 0x00ff, not byte 0xff sign-extended to word. http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... trunk/suite/tests/win32/mixedmode.c:293: /* skip daa if global_data[0] == 0 */ On 2012/07/31 03:21:05, bruening wrote: > you mean if is_x86_to_x64 == 0 Done. Sorry that I forgot to change the comment. I mean if is_x86_to_x64 == 1. http://codereview.appspot.com/6405056/diff/14004/trunk/suite/tests/win32/mixe... trunk/suite/tests/win32/mixedmode.c:363: /* skip daa if global_data[0] == 0 */ On 2012/07/31 03:21:05, bruening wrote: > ditto Done.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/6405056/diff/18004/trunk/core/optionsx.h File trunk/core/optionsx.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/optionsx.h#newcod... trunk/core/optionsx.h:508: OPTION_DEFAULT(bool, x86_to_x64, false, "translate x86 code to x64.") can you expand the string: explain it's when on a 64-bit kernel http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.c File trunk/core/x86/arch.c (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.c#newcod... trunk/core/x86/arch.c:2335: * - x86_to_x64: fault translation has not been tested add i# http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:706: * This may not work if we have 32-bit fragments, in which case we may may not? it definitely won't work http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:707: * refer to X64_CACHE_MODE_DC(). why not just add this to the code? if not, this comment should be prefixed with "FIXME i#nnnn:" b/c it's a correctness issue http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch_exports.h File trunk/core/x86/arch_exports.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch_exports.... trunk/core/x86/arch_exports.h:1391: * In x86_to_x64, sets to the mode of code cache instead. this is an externally-visible comment (see the double star? that's a doxygen comment). users don't know what "In x86_to_x64" means, nor "mode of code cache". we could go update bt.dox and the runtime option list in the .dox files to explain it, but maybe x86_to_x64 is not ready for prime-time yet. I would suggest keeping it un-documented for now. thus, make this a separate, normal comment. minor: comment could be clearer if expanded a little. something like: "For -x86_to_x64, sets the mode of the instr to the code cache mode instead of the app mode." http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/decode.h File trunk/core/x86/decode.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/decode.h#newc... trunk/core/x86/decode.h:637: * Later, if needed, we can introduce a new field in dcontext_t. add a xref to the i# for this http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/emit_utils.c File trunk/core/x86/emit_utils.c (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:5989: * NOTE: this may not work if we have 32-bit fragments. not "may", it won't work. should have "FIXME i#". the "may" part is whether there are 32-bit fragments. if they're there, it won't work. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:152: replace(dcontext, ilist, instr, ok so with this order you don't need next_instr.
Sign in to reply to this message.
http://codereview.appspot.com/6405056/diff/18004/trunk/core/optionsx.h File trunk/core/optionsx.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/optionsx.h#newcod... trunk/core/optionsx.h:508: OPTION_DEFAULT(bool, x86_to_x64, false, "translate x86 code to x64.") On 2012/08/06 16:58:46, bruening wrote: > can you expand the string: explain it's when on a 64-bit kernel Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.c File trunk/core/x86/arch.c (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.c#newcod... trunk/core/x86/arch.c:2335: * - x86_to_x64: fault translation has not been tested On 2012/08/06 16:58:46, bruening wrote: > add i# Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.h File trunk/core/x86/arch.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:706: * This may not work if we have 32-bit fragments, in which case we may On 2012/08/06 16:58:46, bruening wrote: > may not? it definitely won't work Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch.h#newcod... trunk/core/x86/arch.h:707: * refer to X64_CACHE_MODE_DC(). On 2012/08/06 16:58:46, bruening wrote: > why not just add this to the code? if not, this comment should be prefixed with > "FIXME i#nnnn:" b/c it's a correctness issue Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch_exports.h File trunk/core/x86/arch_exports.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/arch_exports.... trunk/core/x86/arch_exports.h:1391: * In x86_to_x64, sets to the mode of code cache instead. On 2012/08/06 16:58:46, bruening wrote: > this is an externally-visible comment (see the double star? that's a doxygen > comment). users don't know what "In x86_to_x64" means, nor "mode of code > cache". we could go update bt.dox and the runtime option list in the .dox files > to explain it, but maybe x86_to_x64 is not ready for prime-time yet. I would > suggest keeping it un-documented for now. thus, make this a separate, normal > comment. > > minor: comment could be clearer if expanded a little. something like: "For > -x86_to_x64, sets the mode of the instr to the code cache mode instead of the > app mode." Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/decode.h File trunk/core/x86/decode.h (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/decode.h#newc... trunk/core/x86/decode.h:637: * Later, if needed, we can introduce a new field in dcontext_t. On 2012/08/06 16:58:46, bruening wrote: > add a xref to the i# for this Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/emit_utils.c File trunk/core/x86/emit_utils.c (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/emit_utils.c#... trunk/core/x86/emit_utils.c:5989: * NOTE: this may not work if we have 32-bit fragments. On 2012/08/06 16:58:46, bruening wrote: > not "may", it won't work. should have "FIXME i#". the "may" part is whether > there are 32-bit fragments. if they're there, it won't work. Done. http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/x86_to_x64.c File trunk/core/x86/x86_to_x64.c (right): http://codereview.appspot.com/6405056/diff/18004/trunk/core/x86/x86_to_x64.c#... trunk/core/x86/x86_to_x64.c:152: replace(dcontext, ilist, instr, On 2012/08/06 16:58:46, bruening wrote: > ok so with this order you don't need next_instr. Any problem?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
