Issue 310700043: i#1569 AArch64: Handle self-modifying programs. (Closed)
|
Created:
7 years, 5 months ago by Edmund.Grimley.Evans Modified:
7 years, 4 months ago CC:
dynamorio-devs_googlegroups.com Visibility:
Public. |
DescriptionCommit log for first patchset:
---------------
i#1569 AArch64: Handle self-modifying programs.
IC IVAU, Xt is mangled into 7-14 instructions including a call to
icache_op_ic_ivau_asm, which records contiguous invalidated icache
lines in icache_op_struct.
ISB is mangled into 11-20 instructions including a conditional branch
to icache_op_isb_asm, which calls dr_flush_region, then
dr_redirect_execution.
---------------
Patch Set 1 #Patch Set 2 : Use fcache_return #
Total comments: 50
Patch Set 3 : Addressed everything #
Total comments: 14
Patch Set 4 : Code moved and removed #Patch Set 5 : Committed #
MessagesTotal messages: 36
Sign in to reply to this message.
> ISB is mangled into 11-20 instructions including a conditional branch > to icache_op_isb_asm From the code cache to DR .text is unusual and not normally how DR operates. There are no reachability guarantees for the DR lib vs the code cache. Generally gencode is used, which does have reachability guarantees. (Reachability is all set up for x86-64 for 32-bit-disp reachability and is sthg that should be discussed for A64 too). > which calls dr_flush_region That's a tool API routine: the internal routines are generally used by DR. Today there is halfhearted support for non-CLIENT_INTERFACE builds, which should perhaps be refactored at some point, but this would fail there. > dr_redirect_execution Also an API routine. This will thwart traces so the perf implications may need consideration.
Sign in to reply to this message.
On 2016/11/02 17:13:28, bruening wrote: > > ISB is mangled into 11-20 instructions including a conditional branch > > to icache_op_isb_asm > > From the code cache to DR .text is unusual and not normally how DR operates. > There are no reachability guarantees for the DR lib vs the code cache. > Generally gencode is used, which does have reachability guarantees. > (Reachability is all set up for x86-64 for 32-bit-disp reachability and is sthg > that should be discussed for A64 too). For AArch64 there are no reachability guarantees at all, except for within a single fragment, right? However, the way I branch from the fragment cache to DR .text definitely will work because it's a register branch. > > which calls dr_flush_region > > That's a tool API routine: the internal routines are generally used by DR. > Today there is halfhearted support for non-CLIENT_INTERFACE builds, which should > perhaps be refactored at some point, but this would fail there. So what's the equivalent for internal use? > > dr_redirect_execution > > Also an API routine. This will thwart traces so the perf implications may need > consideration. How does this thwart traces?
Sign in to reply to this message.
On 2016/11/02 17:13:28, bruening wrote: > > ISB is mangled into 11-20 instructions including a conditional branch > > to icache_op_isb_asm > > From the code cache to DR .text is unusual and not normally how DR operates. > There are no reachability guarantees for the DR lib vs the code cache. > Generally gencode is used, which does have reachability guarantees. > (Reachability is all set up for x86-64 for 32-bit-disp reachability and is sthg > that should be discussed for A64 too). The other issue is that various operations from other threads are better off when control is located in gencode, where those threads can know there are no DR locks held, than when control is "somewhere in the DR library" where all bets are off for locks. Extra work may still need to be done for things like pending signals, but that may also be easier for non-DR-library code.
Sign in to reply to this message.
Mostly I'm just talking out loud here, and have not looked at the details, but the main question is: why is this being handled so differently from other architectures? Why drift from the usual model of going back to dispatch and DR to handle complex, rare situations, where we'll have signal pending checks and other things for free by following the normal mechanisms?
Sign in to reply to this message.
On 2016/11/04 13:54:07, bruening wrote: > Mostly I'm just talking out loud here, and have not looked at the details, but > the main question is: why is this being handled so differently from other > architectures? Why drift from the usual model of going back to dispatch and DR > to handle complex, rare situations, where we'll have signal pending checks and > other things for free by following the normal mechanisms? ISB can occur very frequently. We certainly don't want to return to dispatch on every ISB. (I did this previously and the slowdown on some versions of Debian was obvious and annoying even for just running "make test".) What I'd like is for it to incur the cost of saving all the registers only when there is an ISB preceded by at least one IC IVAU. The cache lines specified by the IC IVAUs will typically be contiguous. Collecting them into a single region isn't too hard to implement in assembler but the resulting code (icache_op_ic_ivau_asm) is too big to be reasonably inlined. Yes, signals are an issue. There's probably also an issue with the global lock: several threads might be executing IC IVAU. As it is, if a thread gets killed while it has the lock then other threads may spin forever, but fixing that may be easier with the code in aarch64.asm than with the lock-handling code being inlined into the fragment cache.
Sign in to reply to this message.
On 2016/11/04 14:20:11, Edmund.Grimley.Evans wrote: > On 2016/11/04 13:54:07, bruening wrote: > > Mostly I'm just talking out loud here, and have not looked at the details, but > > the main question is: why is this being handled so differently from other > > architectures? Why drift from the usual model of going back to dispatch and > DR > > to handle complex, rare situations, where we'll have signal pending checks and > > other things for free by following the normal mechanisms? > > ISB can occur very frequently. We certainly don't want to return to dispatch on > every ISB. (I did this previously and the slowdown on some versions of Debian > was obvious and annoying even for just running "make test".) This is good to know. Is there a design doc or somewhere where what you tried is documented? Maybe adding something to the wiki would help. There's no reason the part that is doing the flush shouldn't go back to dispatch to do its work, I assume. > There's probably also an issue with the global lock: several threads might be > executing IC IVAU. As it is, if a thread gets killed while it has the lock then > other threads may spin forever, but fixing that may be easier with the code in > aarch64.asm than with the lock-handling code being inlined into the fragment > cache. Holding a DR lock in the code cache is not allowed: it violates key invariants and will lead to deadlocks (it is listed on https://github.com/DynamoRIO/dynamorio/wiki/Code-Content)
Sign in to reply to this message.
> There's no reason the part that is doing the flush shouldn't go back to dispatch > to do its work, I assume. Yes, presumably that would work too. What are the advantages?
Sign in to reply to this message.
On 2016/11/09 14:37:27, Edmund.Grimley.Evans wrote: > > There's no reason the part that is doing the flush shouldn't go back to > dispatch > > to do its work, I assume. > > Yes, presumably that would work too. What are the advantages? It matches how everything else works in DR today, which means it is simpler and cleaner in its interactions with signals, thread synch, reset, accounting statistics, etc. It will work regardless of CLIENT_INTERFACE. It will be more maintainable code in C rather than asm.
Sign in to reply to this message.
> It matches how everything else works in DR today, which means it is simpler and > cleaner in its interactions with signals, thread synch, reset, accounting > statistics, etc. It will work regardless of CLIENT_INTERFACE. It will be more > maintainable code in C rather than asm. The rest may be true, but not that last point, I think. It will be more a matter of expanding and complicating the part that is already in C, while the part in asm will be rewritten without becoming any simpler. Obviously I already go to C as soon as I know that I have to. The calls to dr_flush_region and dr_redirect_execution are already written in C: icache_op_isb.
Sign in to reply to this message.
On 2016/11/11 09:12:18, Edmund.Grimley.Evans wrote: > > It matches how everything else works in DR today, which means it is simpler > and > > cleaner in its interactions with signals, thread synch, reset, accounting > > statistics, etc. It will work regardless of CLIENT_INTERFACE. It will be > more > > maintainable code in C rather than asm. > > The rest may be true, but not that last point, I think. It will be more a matter > of expanding and complicating the part that is already in C, while the part in > asm will be rewritten without becoming any simpler. Obviously I already go to C > as soon as I know that I have to. The calls to dr_flush_region and > dr_redirect_execution are already written in C: icache_op_isb. As already pointed out, dr_flush_region and dr_redirect_execution are tool API's and should not be used here. They obfuscate the DR internal accounting: things like stats on cache exits from core DR will not be properly attributed. They will also not work with non-tool-enabled DR builds. dr_redirect_execution ends up going back to dispatch anyway, so better to go there first. In general we do not want to add new mechanisms for executing DR code: it complicates an already complicated system and makes reasoning about thread synch and security complicated. Self-protection features and all of the other operations (signals, etc.) rely on control flowing through well-defined and enumerated control points. Adding new routes that call key DR operations like flushing yet don't go through fcache_return are not a good idea if there's no advantage to doing so.
Sign in to reply to this message.
Yes, I accept your other points and hope to update this patch some time next week. I'm just warning you that the next version will probably be more complex rather than less complex.
Sign in to reply to this message.
I'm not sure how to do a conditional return to dispatch via DR's .text. Any suggestions? Is there a precedent?
Sign in to reply to this message.
All returns from app state in the cache or gencode go through an exit stub that targets fcache_return and also tells dispatch *why* it exited via a special linkstub pointer.
Sign in to reply to this message.
On 2016/11/14 16:14:35, bruening wrote: > All returns from app state in the cache or gencode go through an exit stub that > targets fcache_return and also tells dispatch *why* it exited via a special > linkstub pointer. So I could mangle ISB into something that branches around some kind of a struct (what's its type?) and enter .text with a pointer to that struct, which I put into the appropriate register or numbered SLOT before branching to fcache_return? I'm not sure what trace optimisation would make of that... Perhaps the closest precedent for what I want to do is a conditional indirect branch, except that instead of trying first to find the target in a hash table I always go back to dispatch, and the evaluation of the condition involves calling some code in .text. I presume ARM's conditional indirect branches are handled efficiently in the case where the branch is not taken?
Sign in to reply to this message.
On Mon, Nov 14, 2016 at 11:45 AM, <Edmund.Grimley.Evans@gmail.com> wrote: > On 2016/11/14 16:14:35, bruening wrote: > >> All returns from app state in the cache or gencode go through an exit >> > stub that > >> targets fcache_return and also tells dispatch *why* it exited via a >> > special > >> linkstub pointer. >> > > So I could mangle ISB into something that branches around some kind of a > struct (what's its type?) and enter .text with a pointer to that struct, which I put into the appropriate register or numbered SLOT before > branching to fcache_return? > I'm not sure what you mean by "branches around a struct" but the last part matches what is generally done: emulate a stub by placing a pointer to the special linkstub in the appropriate register before targeting fcache_return. E.g., see get_selfmod_linkstub(), get_ibl_deleted_linkstub(), get_reset_linkstub(), etc. etc. I presume ARM's > conditional indirect branches are handled efficiently in the case where > the branch is not taken? > IIRC it was easiest to send through the IBL so it is not a first-class link but considered good enough until shown to happen often enough on app critical paths to matter.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1569 AArch64: Handle self-modifying programs. IC IVAU, Xt is mangled into 7-14 instructions including a call to icache_op_ic_ivau_asm, which records contiguous invalidated icache lines in icache_op_struct. ISB is mangled into 11-20 instructions including a conditional branch to icache_op_isb_asm, which returns to dispatch using linkstub_aarch64_selfmod. Review-URL: https://codereview.appspot.com/310700043 ---------------
Sign in to reply to this message.
Ping.
Sign in to reply to this message.
Sorry, will get to it tomorrow.
Sign in to reply to this message.
Having some kind of A64 design doc (maybe in wiki) that discusses tradeoffs and the basic approach here would be useful. https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... File core/arch/aarch64/aarch64.asm (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:637: * been invalidated we tailcall icache_op_isb instead of returning. I don't see any reference to icache_op_isb https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:650: * FIXME: We do not correctly handle the case where the set of contiguous cache i# https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:678: ldr x3, [x0, #16] Why #16 -- unclear: named constant would help https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:681: str x3, [x0, #16] Ditto, and at multiple points below https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:776: * switch stack, write dr_mcontext_t to new stack, then tailcall to icache_op_isb, I don't see any reference to icache_op_isb https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/instr.c File core/arch/aarch64/instr.c (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/instr.c... core/arch/aarch64/instr.c:265: if (opc == OP_sys && opnd_get_immed_int(instr_get_src(instr, 0)) == 0x1ba9) Use a named constant https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h File core/arch/arch_exports.h (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1206: * members alignment guarantees that bit 2 of the address of "lock" is set. Add an assert to some arch init routine https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1220: } icache_op_struct_t; This is very A64-specific and is only referenced in aarch dirs -- so it does not need to be exported from arch/ and can be in an arch-private header instead. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode156 core/dispatch.c:156: char *begin = (char *)dcontext->local_state->spill_space.r2; style: we never use char* for non-strings: we use byte* for general addresses, but these should be of type app_pc https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode157 core/dispatch.c:157: char *end = (char *)dcontext->local_state->spill_space.r3; ditto https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode159 core/dispatch.c:159: dr_flush_region((app_pc)begin, end - begin); flush_fragments_from_region() should be used instead: 1) dr_flush_region() is a client API routine which is not available in some DR builds and as such is only used by clients, not by DR internally. 2) dr_flush_region() is a heavyweight, synchronous operation that suspends all other threads, and should be avoided. The DR cache consistency model uses an unlink flush instead for cache consistency flushes (see "Maintaining Consistency and Bounding Capacity of Software Code Caches" CGO '05). Also see what the ARM SYS_cacheflush handler does. For x86, executable_areas needs to be updated as well, but that may not apply here. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode163 core/dispatch.c:163: dispatch_enter_dynamorio(dcontext); The above code shouldn't be before this is called: it will have the wrong whereami, among other things. Better to be inside dispatch_enter_dynamorio along with similar code. https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c File core/dynamo.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode145 core/dynamo.c:145: void icache_op_isb(byte *begin, byte *end, dr_mcontext_t *mcontext) style violation https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode145 core/dynamo.c:145: void icache_op_isb(byte *begin, byte *end, dr_mcontext_t *mcontext) use app_pc as these are app instruction addresses https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode145 core/dynamo.c:145: void icache_op_isb(byte *begin, byte *end, dr_mcontext_t *mcontext) shouldn't this be priv_mcontext_t? https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode152 core/dynamo.c:152: reg_t *stolen = &mcontext->r0 + (dr_reg_stolen - DR_REG_X0); use reg_set_value https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode155 core/dynamo.c:155: dr_flush_region((app_pc)begin, end - begin); See flushing comments on earlier use https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode156 core/dynamo.c:156: dr_redirect_execution(mcontext); This is a client-only routine that won't work in some builds of DR. Better to -- ok, where is this called from? I can't find any reference to it. https://codereview.appspot.com/310700043/diff/20001/core/lib/statsx.h File core/lib/statsx.h (right): https://codereview.appspot.com/310700043/diff/20001/core/lib/statsx.h#newcode714 core/lib/statsx.h:714: STATS_DEF("Fcache exits, AArch64 selfmod", num_exits_aarch64_selfmod) Can the existing selfmod stat be used? https://codereview.appspot.com/310700043/diff/20001/core/link.c File core/link.c (right): https://codereview.appspot.com/310700043/diff/20001/core/link.c#newcode156 core/link.c:156: const linkstub_t linkstub_aarch64_selfmod = { LINK_FAKE, 0 }; Again, linkstub_selfmod should serve just fine: no need to add something new. https://codereview.appspot.com/310700043/diff/20001/core/link.h File core/link.h (right): https://codereview.appspot.com/310700043/diff/20001/core/link.h#newcode482 core/link.h:482: extern const linkstub_t linkstub_aarch64_selfmod; Why are separate A64-only linkstubs needed? I would think that the existing selfmod linkstub would work just fine.
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode159 core/dispatch.c:159: dr_flush_region((app_pc)begin, end - begin); On 2016/12/07 05:24:29, bruening wrote: > flush_fragments_from_region() should be used instead: > > 1) dr_flush_region() is a client API routine which is not available in some DR > builds and as such is only used by clients, not by DR internally. > > 2) dr_flush_region() is a heavyweight, synchronous operation that suspends all > other threads, and should be avoided. The DR cache consistency model uses an > unlink flush instead for cache consistency flushes (see "Maintaining Consistency > and Bounding Capacity of Software Code Caches" CGO '05). Also see what the ARM > SYS_cacheflush handler does. > > For x86, executable_areas needs to be updated as well, but that may not apply > here. Be sure to terminate a fragment on creation or exit it dynamically for the unlink flush.
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... File core/arch/aarch64/aarch64.asm (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:637: * been invalidated we tailcall icache_op_isb instead of returning. On 2016/12/07 05:24:29, bruening wrote: > I don't see any reference to icache_op_isb I'll remove it. https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:650: * FIXME: We do not correctly handle the case where the set of contiguous cache On 2016/12/07 05:24:29, bruening wrote: > i# s/FIXME/XXX/. This isn't worth worrying about. https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:678: ldr x3, [x0, #16] On 2016/12/07 05:24:29, bruening wrote: > Why #16 -- unclear: named constant would help It's just r0, r1, r2, ... in spill_state_t. What do you want to call them, spill_state_r0_OFFSET and so on, or TLS_REG0_SLOT_OFFSET et cetera? Fortunately my keyboard has a working Caps Lock... https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/instr.c File core/arch/aarch64/instr.c (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/instr.c... core/arch/aarch64/instr.c:265: if (opc == OP_sys && opnd_get_immed_int(instr_get_src(instr, 0)) == 0x1ba9) On 2016/12/07 05:24:29, bruening wrote: > Use a named constant We can call that SYS_ARG_IC_IVAU or something.
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h File core/arch/arch_exports.h (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1206: * members alignment guarantees that bit 2 of the address of "lock" is set. On 2016/12/07 05:24:29, bruening wrote: > Add an assert to some arch init routine Any idea which one? https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1220: } icache_op_struct_t; On 2016/12/07 05:24:29, bruening wrote: > This is very A64-specific and is only referenced in aarch dirs -- so it does not > need to be exported from arch/ and can be in an arch-private header instead. I think the type is only referenced in core/dynamo.c, but the definition of icache_op_struct could live anywhere. Shall I put both the type and the variable in aarchxx/mangle.c? Or create a new file just for that? I could define the variable in aarch64.asm, but that would be unhelpful, I think, even though it would be logical, in a way. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode156 core/dispatch.c:156: char *begin = (char *)dcontext->local_state->spill_space.r2; On 2016/12/07 05:24:29, bruening wrote: > style: we never use char* for non-strings: we use byte* for general addresses, > but these should be of type app_pc Changed. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode159 core/dispatch.c:159: dr_flush_region((app_pc)begin, end - begin); On 2016/12/07 05:24:29, bruening wrote: > flush_fragments_from_region() should be used instead: Yes. I've changed that. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode159 core/dispatch.c:159: dr_flush_region((app_pc)begin, end - begin); On 2016/12/07 14:50:32, bruening wrote: > Be sure to terminate a fragment on creation or exit it dynamically for the > unlink flush. I'm not sure what you mean by that... https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode163 core/dispatch.c:163: dispatch_enter_dynamorio(dcontext); On 2016/12/07 05:24:29, bruening wrote: > The above code shouldn't be before this is called: it will have the wrong > whereami, among other things. Better to be inside dispatch_enter_dynamorio > along with similar code. Moving the code to dispatch_enter_dynamorio stops it from working...
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h File core/arch/arch_exports.h (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1206: * members alignment guarantees that bit 2 of the address of "lock" is set. On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > On 2016/12/07 05:24:29, bruening wrote: > > Add an assert to some arch init routine > > Any idea which one? Maybe mangle_init should call mangle_arch_init() https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1220: } icache_op_struct_t; On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > On 2016/12/07 05:24:29, bruening wrote: > > This is very A64-specific and is only referenced in aarch dirs -- so it does > not > > need to be exported from arch/ and can be in an arch-private header instead. > > I think the type is only referenced in core/dynamo.c I thought that routine was being removed? If it remains I'm not sure dynamo.c is the best place for it. https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1220: } icache_op_struct_t; On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > On 2016/12/07 05:24:29, bruening wrote: > > This is very A64-specific and is only referenced in aarch dirs -- so it does > not > > need to be exported from arch/ and can be in an arch-private header instead. > > I think the type is only referenced in core/dynamo.c, but the definition of > icache_op_struct could live anywhere. Shall I put both the type and the variable > in aarchxx/mangle.c? Or create a new file just for that? > > I could define the variable in aarch64.asm, but that would be unhelpful, I > think, even though it would be logical, in a way. mangle.c seems good to me. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode159 core/dispatch.c:159: dr_flush_region((app_pc)begin, end - begin); On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > On 2016/12/07 14:50:32, bruening wrote: > > Be sure to terminate a fragment on creation or exit it dynamically for the > > unlink flush. > > I'm not sure what you mean by that... unlink flushes are not synchronous and will not remove a fragment until all threads exit it, so you do not want to return and continue on in the same fragment: either the cache flush instructions should always terminate a fragment or a special exit has to be added. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode163 core/dispatch.c:163: dispatch_enter_dynamorio(dcontext); On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > On 2016/12/07 05:24:29, bruening wrote: > > The above code shouldn't be before this is called: it will have the wrong > > whereami, among other things. Better to be inside dispatch_enter_dynamorio > > along with similar code. > > Moving the code to dispatch_enter_dynamorio stops it from working... In what way? Why?
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c File core/dynamo.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dynamo.c#newcode156 core/dynamo.c:156: dr_redirect_execution(mcontext); On 2016/12/07 05:24:29, bruening wrote: > This is a client-only routine that won't work in some builds of DR. Better to > -- ok, where is this called from? I can't find any reference to it. It's left over from a previous version of this patch. Sorry about that! https://codereview.appspot.com/310700043/diff/20001/core/lib/statsx.h File core/lib/statsx.h (right): https://codereview.appspot.com/310700043/diff/20001/core/lib/statsx.h#newcode714 core/lib/statsx.h:714: STATS_DEF("Fcache exits, AArch64 selfmod", num_exits_aarch64_selfmod) On 2016/12/07 05:24:29, bruening wrote: > Can the existing selfmod stat be used? Yes, I can probably combine this with num_ibt_exit_src_sandboxed. https://codereview.appspot.com/310700043/diff/20001/core/link.h File core/link.h (right): https://codereview.appspot.com/310700043/diff/20001/core/link.h#newcode482 core/link.h:482: extern const linkstub_t linkstub_aarch64_selfmod; On 2016/12/07 05:24:29, bruening wrote: > Why are separate A64-only linkstubs needed? I would think that the existing > selfmod linkstub would work just fine. Yes, but I have to make linkstub_selfmod extern rather than static, at least on AArch64, so that it can be referred to in aarch64.asm. I'll do that.
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h File core/arch/arch_exports.h (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1220: } icache_op_struct_t; On 2016/12/07 17:44:19, bruening wrote: > On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > > On 2016/12/07 05:24:29, bruening wrote: > > > This is very A64-specific and is only referenced in aarch dirs -- so it does > > not > > > need to be exported from arch/ and can be in an arch-private header instead. > > > > I think the type is only referenced in core/dynamo.c > > I thought that routine was being removed? If it remains I'm not sure dynamo.c > is the best place for it. Yes, the routine's going, but there's still the variable "icache_op_struct_t icache_op_struct", which is only accessed from assembler, but it's nicer for documentation purposes to define it in C. I'll move it to mangle.c, as you suggested. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode159 core/dispatch.c:159: dr_flush_region((app_pc)begin, end - begin); On 2016/12/07 17:44:19, bruening wrote: > On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > > On 2016/12/07 14:50:32, bruening wrote: > > > Be sure to terminate a fragment on creation or exit it dynamically for the > > > unlink flush. > > > > I'm not sure what you mean by that... > > unlink flushes are not synchronous and will not remove a fragment until all > threads exit it, so you do not want to return and continue on in the same > fragment: either the cache flush instructions should always terminate a fragment > or a special exit has to be added. Thanks. I think I don't return to the fragment so it should be OK. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode163 core/dispatch.c:163: dispatch_enter_dynamorio(dcontext); On 2016/12/07 17:44:19, bruening wrote: > On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > > On 2016/12/07 05:24:29, bruening wrote: > > > The above code shouldn't be before this is called: it will have the wrong > > > whereami, among other things. Better to be inside dispatch_enter_dynamorio > > > along with similar code. > > > > Moving the code to dispatch_enter_dynamorio stops it from working... > > In what way? Why? To be investigated, tomorrow!
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/link.h File core/link.h (right): https://codereview.appspot.com/310700043/diff/20001/core/link.h#newcode482 core/link.h:482: extern const linkstub_t linkstub_aarch64_selfmod; On 2016/12/07 17:57:46, Edmund.Grimley.Evans wrote: > On 2016/12/07 05:24:29, bruening wrote: > > Why are separate A64-only linkstubs needed? I would think that the existing > > selfmod linkstub would work just fine. > > Yes, but I have to make linkstub_selfmod extern rather than static, at least on > AArch64, so that it can be referred to in aarch64.asm. I'll do that. Can the asm call the get_ accessor like C code would do?
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/link.h File core/link.h (right): https://codereview.appspot.com/310700043/diff/20001/core/link.h#newcode482 core/link.h:482: extern const linkstub_t linkstub_aarch64_selfmod; On 2016/12/07 18:40:15, bruening wrote: > On 2016/12/07 17:57:46, Edmund.Grimley.Evans wrote: > > On 2016/12/07 05:24:29, bruening wrote: > > > Why are separate A64-only linkstubs needed? I would think that the existing > > > selfmod linkstub would work just fine. > > > > Yes, but I have to make linkstub_selfmod extern rather than static, at least > on > > AArch64, so that it can be referred to in aarch64.asm. I'll do that. > > Can the asm call the get_ accessor like C code would do? The caller has live values in nearly all the registers and no stack so calling a C function would be difficult.
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/link.h File core/link.h (right): https://codereview.appspot.com/310700043/diff/20001/core/link.h#newcode482 core/link.h:482: extern const linkstub_t linkstub_aarch64_selfmod; On 2016/12/07 21:35:44, Edmund.Grimley.Evans wrote: > On 2016/12/07 18:40:15, bruening wrote: > > On 2016/12/07 17:57:46, Edmund.Grimley.Evans wrote: > > > On 2016/12/07 05:24:29, bruening wrote: > > > > Why are separate A64-only linkstubs needed? I would think that the > existing > > > > selfmod linkstub would work just fine. > > > > > > Yes, but I have to make linkstub_selfmod extern rather than static, at least > > on > > > AArch64, so that it can be referred to in aarch64.asm. I'll do that. > > > > Can the asm call the get_ accessor like C code would do? > > The caller has live values in nearly all the registers and no stack so calling a > C function would be difficult. Note that the typical way to do this in DR would be to generate the code, where the linkstub get_ accessor would be used to produce a constant in the code. Using asm from the cache is not done anywhere else in DR these days, for several reasons including thread-private vs shared and (on x86) reachability.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1569 AArch64: Handle self-modifying programs. IC IVAU, Xt is mangled into 7-14 instructions including a call to icache_op_ic_ivau_asm, which records contiguous invalidated icache lines in icache_op_struct. ISB is mangled into 11-20 instructions including a conditional branch to icache_op_isb_asm, which returns to dispatch using linkstub_aarch64_selfmod. Review-URL: https://codereview.appspot.com/310700043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... File core/arch/aarch64/aarch64.asm (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:678: ldr x3, [x0, #16] On 2016/12/07 16:48:52, Edmund.Grimley.Evans wrote: > On 2016/12/07 05:24:29, bruening wrote: > > Why #16 -- unclear: named constant would help > > It's just r0, r1, r2, ... in spill_state_t. What do you want to call them, > spill_state_r0_OFFSET and so on, or TLS_REG0_SLOT_OFFSET et cetera? > > Fortunately my keyboard has a working Caps Lock... I'm using spill_state_r0_OFFSET for consistency. https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h File core/arch/arch_exports.h (right): https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1206: * members alignment guarantees that bit 2 of the address of "lock" is set. On 2016/12/07 17:44:19, bruening wrote: > On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > > On 2016/12/07 05:24:29, bruening wrote: > > > Add an assert to some arch init routine > > > > Any idea which one? > > Maybe mangle_init should call mangle_arch_init() Done. https://codereview.appspot.com/310700043/diff/20001/core/arch/arch_exports.h#... core/arch/arch_exports.h:1220: } icache_op_struct_t; On 2016/12/07 17:44:19, bruening wrote: > On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > > On 2016/12/07 05:24:29, bruening wrote: > > > This is very A64-specific and is only referenced in aarch dirs -- so it does > > not > > > need to be exported from arch/ and can be in an arch-private header instead. > > > > I think the type is only referenced in core/dynamo.c, but the definition of > > icache_op_struct could live anywhere. Shall I put both the type and the > variable > > in aarchxx/mangle.c? Or create a new file just for that? > > > > I could define the variable in aarch64.asm, but that would be unhelpful, I > > think, even though it would be logical, in a way. > > mangle.c seems good to me. Done. https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/20001/core/dispatch.c#newcode163 core/dispatch.c:163: dispatch_enter_dynamorio(dcontext); On 2016/12/07 18:23:51, Edmund.Grimley.Evans wrote: > On 2016/12/07 17:44:19, bruening wrote: > > On 2016/12/07 17:37:19, Edmund.Grimley.Evans wrote: > > > On 2016/12/07 05:24:29, bruening wrote: > > > > The above code shouldn't be before this is called: it will have the wrong > > > > whereami, among other things. Better to be inside > dispatch_enter_dynamorio > > > > along with similar code. > > > > > > Moving the code to dispatch_enter_dynamorio stops it from working... > > > > In what way? Why? > > To be investigated, tomorrow! Strange, no problem when I try it today...
Sign in to reply to this message.
Looks good except I don't understand the last_exit_deleted() change? https://codereview.appspot.com/310700043/diff/40001/core/arch/aarch64/aarch64... File core/arch/aarch64/aarch64.asm (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:789: * where we will call dr_flush_region, then dr_redirect_execution. what is called is no longer accurate https://codereview.appspot.com/310700043/diff/40001/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/aarchxx/mangle.... core/arch/aarchxx/mangle.c:87: ASSERT(((ptr_uint_t)&icache_op_struct.lock & 0xf) != 0); Use !ALIGNED https://codereview.appspot.com/310700043/diff/40001/core/arch/mangle_shared.c File core/arch/mangle_shared.c (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/mangle_shared.c... core/arch/mangle_shared.c:1235: mangle_arch_init(); Wrong place: the above comment applies to the lines below! Please put above the comment. https://codereview.appspot.com/310700043/diff/40001/core/arch/x86/mangle.c File core/arch/x86/mangle.c (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/x86/mangle.c#ne... core/arch/x86/mangle.c:83: { style: we typically put /* Nothing yet. */ or sthg https://codereview.appspot.com/310700043/diff/40001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/40001/core/dispatch.c#newcode747 core/dispatch.c:747: if (dcontext->last_exit == get_selfmod_linkstub()) { This is still too early: we still haven't done general "entering DR from cache" things like os_enter_dynamorio() and KSTOP_NOT_MATCHING(dispatch_num_exits) -- doing it here will mess up the kstats timing data and other features. It should go down where we start acting on from-cache exit reasons, maybe in the area of handle_callback_return() on line 873. https://codereview.appspot.com/310700043/diff/40001/core/dispatch.c#newcode1372 core/dispatch.c:1372: else if (dcontext->last_exit == get_selfmod_linkstub()) { This will never be executed because the earlier check for the selfmod linkstub will be hit. It seems like this "else if" here should be removed and instead the message for the earlier one should perhaps be tweaked as A64 is not always a self-flush. https://codereview.appspot.com/310700043/diff/40001/core/link.c File core/link.c (right): https://codereview.appspot.com/310700043/diff/40001/core/link.c#newcode637 core/link.c:637: } else if (dcontext->last_exit == get_selfmod_linkstub()) { Why is this code needed? A selfmod linkstub is fake, so it should hit the FRAG_FAKE if() above and this here should be dead code -- right?
Sign in to reply to this message.
https://codereview.appspot.com/310700043/diff/40001/core/arch/aarch64/aarch64... File core/arch/aarch64/aarch64.asm (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/aarch64/aarch64... core/arch/aarch64/aarch64.asm:789: * where we will call dr_flush_region, then dr_redirect_execution. On 2016/12/11 16:50:22, bruening wrote: > what is called is no longer accurate Fixed. https://codereview.appspot.com/310700043/diff/40001/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/aarchxx/mangle.... core/arch/aarchxx/mangle.c:87: ASSERT(((ptr_uint_t)&icache_op_struct.lock & 0xf) != 0); On 2016/12/11 16:50:22, bruening wrote: > Use !ALIGNED Done. https://codereview.appspot.com/310700043/diff/40001/core/arch/mangle_shared.c File core/arch/mangle_shared.c (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/mangle_shared.c... core/arch/mangle_shared.c:1235: mangle_arch_init(); On 2016/12/11 16:50:22, bruening wrote: > Wrong place: the above comment applies to the lines below! Please put above the > comment. Done. https://codereview.appspot.com/310700043/diff/40001/core/arch/x86/mangle.c File core/arch/x86/mangle.c (right): https://codereview.appspot.com/310700043/diff/40001/core/arch/x86/mangle.c#ne... core/arch/x86/mangle.c:83: { On 2016/12/11 16:50:22, bruening wrote: > style: we typically put /* Nothing yet. */ or sthg Done. https://codereview.appspot.com/310700043/diff/40001/core/dispatch.c File core/dispatch.c (right): https://codereview.appspot.com/310700043/diff/40001/core/dispatch.c#newcode747 core/dispatch.c:747: if (dcontext->last_exit == get_selfmod_linkstub()) { On 2016/12/11 16:50:22, bruening wrote: > This is still too early: we still haven't done general "entering DR from cache" > things like os_enter_dynamorio() and KSTOP_NOT_MATCHING(dispatch_num_exits) -- > doing it here will mess up the kstats timing data and other features. It should > go down where we start acting on from-cache exit reasons, maybe in the area of > handle_callback_return() on line 873. Done. https://codereview.appspot.com/310700043/diff/40001/core/dispatch.c#newcode1372 core/dispatch.c:1372: else if (dcontext->last_exit == get_selfmod_linkstub()) { On 2016/12/11 16:50:22, bruening wrote: > This will never be executed because the earlier check for the selfmod linkstub > will be hit. It seems like this "else if" here should be removed and instead > the message for the earlier one should perhaps be tweaked as A64 is not always a > self-flush. Done. https://codereview.appspot.com/310700043/diff/40001/core/link.c File core/link.c (right): https://codereview.appspot.com/310700043/diff/40001/core/link.c#newcode637 core/link.c:637: } else if (dcontext->last_exit == get_selfmod_linkstub()) { On 2016/12/11 16:50:22, bruening wrote: > Why is this code needed? A selfmod linkstub is fake, so it should hit the > FRAG_FAKE if() above and this here should be dead code -- right? Yes, it seems to be dead. I've removed it.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1569 AArch64: Handle self-modifying programs. IC IVAU, Xt is mangled into 7-14 instructions including a call to icache_op_ic_ivau_asm, which records contiguous invalidated icache lines in icache_op_struct. ISB is mangled into 11-20 instructions including a conditional branch to icache_op_isb_asm, which returns to dispatch using linkstub_selfmod. Review-URL: https://codereview.appspot.com/310700043 ---------------
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/dynamorio/commit/f2a666946b666a296cc4746c1ab24ee... Final commit log: --------------- i#1569 AArch64: Handle self-modifying programs. IC IVAU, Xt is mangled into 7-14 instructions including a call to icache_op_ic_ivau_asm, which records contiguous invalidated icache lines in icache_op_struct. ISB is mangled into 11-20 instructions including a conditional branch to icache_op_isb_asm, which returns to dispatch using linkstub_selfmod. Review-URL: https://codereview.appspot.com/310700043 ---------------
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
