i#1726: Port Umbra to ARM by toshipiazza · Pull Request #2044 · DynamoRIO/drmemory

toshipiazza · 2017-08-30T17:35:57Z

Pulls out shadow_table_insert_app_to_shadow() into 2 arch-specific
methods for x86 and for ARM. Also implements this method for ARM.

Issue: #1726

Pulls out shadow_table_insert_app_to_shadow() into 2 arch-specific methods for x86 and for ARM. Also implements this method for ARM. Issue: #1726

toshipiazza · 2017-08-30T17:37:29Z

+    disp_hi >>= 0x10;
+
+    /* %reg_idx = table */
+    PRE(ilist, where, INSTR_CREATE_movw(drcontext,


INSTR_CREATE_load_int() does not load 32-bit ints, so I resorted to using movw and movt. Is there a shorthand for this in the repo already? If not, should there be one?

There is instrlist_insert_mov_immed_ptrsz()

Whoops, forgot about that method. Done.

toshipiazza · 2017-08-30T17:40:43Z

Some questions:

Should we split the files up even more? We're basically using all the same code between x86 and ARM, so maybe it's not worth.
Is the new code sequence alright? It looks non-optimal because it requires a second scratch register, but I haven't been able to get it to use a single scratch register.

I think Umbra on x86 and ARM should now be functionally equivalent, but there are no tests in the repo for it; should I add some tests?

derekbruening · 2017-08-30T17:50:06Z

I think Umbra on x86 and ARM should now be functionally equivalent, but there are no tests in the repo for it; should I add some tests?

This is a hole is the test suite: see #1468. We would love to have some tests added.

- Fixes build error/typo in CMakeLists.txt - Replaces movw/movt sequence with instrlist_insert_mov_immed_ptrsz()

toshipiazza · 2017-08-30T18:07:12Z

I'll add some tests in with this CL then.

derekbruening

Should we split the files up even more? We're basically using all the same code between x86 and ARM, so maybe it's not worth.

If we did split I would suggest renaming umbra_x86.c to umbra_32bit.c (or just umbra_32.c?) and having umbra_x86.c and umbra_arm.c contain the differing code. It does seem weird to have ARM use a file named "umbra_x86.c". We could rename that file and not add any others (keeping the ifdef inside it for that one routine)?

Is the new code sequence alright? It looks non-optimal because it requires a second scratch register, but I haven't been able to get it to use a single scratch register.

I put a suggestion that I think gets rid of the 2nd scratch.

derekbruening · 2017-09-06T01:58:58Z

+                                       reg_id_t reg_addr, reg_id_t *regs)
+{
+    reg_id_t reg_idx = regs[0];
+    reg_id_t reg_tmp = regs[1];


Hmm, this just looks risky: maybe pass in "int num_scratch_regs" and assert on it or sthg. At least a comment next to the regs (I like scratch_regs better) param.

Since we can do this with 1 reg, the array is no longer passed in.

derekbruening · 2017-09-06T01:59:33Z

-#ifdef X86
+shadow_table_insert_app_to_shadow_arch(void *drcontext, umbra_map_t *map,
+                                       instrlist_t *ilist, instr_t *where,
+                                       reg_id_t reg_addr, reg_id_t *regs)


s/regs/scratch_regs/

Since we can do this with 1 reg, the array is no longer passed in.

derekbruening · 2017-09-06T02:03:14Z

+/* code sequence:
+ * %reg_idx   = table
+ * %reg_tmp   = (%reg_addr >> 0x10)
+ * %reg_idx  += (%reg_tmp  << 0x02)


You could get rid of reg_tmp by adding (%reg_addr >> 14) here and then zeroing the bottom 2 bits, right?

Sorry about the delay. I think this would only work if the static table already has the bottom 2 bits cleared:

reg_idx = 0x41414141 reg_idx += (reg_addr >> 14) reg_idx &= 0xfffffffc

If we simply clear the last two bits of reg_idx, we might overwrite the bottom 2 bits of the address of the shadow table. I guess it's ok to assume that the static table is 4 byte aligned though, so maybe this is fine.

Unrelated: there's a confusing variable called static_shadow_table_is_used which is statically set to false, though really the variable should be named static_shadow_table_unused or sthg. Can update this.

Hmm yeah that seems to never be written to?

Yup looks like it's only being used once to determine whether to allocate the shadow table using nonheap_alloc() (if static_shadow_table_is_used is true), but uses the static shadow table if static_shadow_table_is_used is false, hence the confusion.

- Reduces number of regs required for Umbra on ARM - Renames umbra_x86 to umbra_32, and umbra_x64 to umbra_64

derekbruening · 2017-09-25T18:40:34Z

I think most of the failures are the same ones that hit DR when Travis updated their VM image about 2 weeks ago: DynamoRIO/dynamorio#2632, DynamoRIO/dynamorio#2634, DynamoRIO/dynamorio#2641

I fixed the first two but am still working on the last one: if anyone can reproduce it locally that would help a great deal as it is slow going trying to diagnose through just what I can print out on Travis.

Xref #2050 where @mxmssh is also hitting failures and about to update DR and I think hitting some further warnings from the clang upgrade.

toshipiazza · 2017-10-04T15:46:31Z

@@ -1,4 +1,4 @@
-#!/bin/sh


On my box, /bin/sh -> /bin/dash, and I believe that this script uses a bash-ism. Not sure why I only saw this recently though. Should I change this back?

Making it bash seems fine: we did the same thing with git_review.sh

toshipiazza · 2017-10-04T15:47:09Z

 # a function scope, we must set the global vars ahead of time:
 configure_DynamoRIO_global(OFF ON)

-# We save pdb+dll space by not using libc (xref DRi#714).


Umbra pulls in libc, so we can't do this anymore.

On Windows too?

I think so, as of dddcfc6 right?

That one may have unnecessarily changed Windows too. If I re-instate set(DynamoRIO_USE_LIBC OFF) today and build umbra it builds fine and there are no unusual imports. Missing symbols at runtime are usually a UNIX thing for ELF and !LD_BIND_NOW: on Windows the linker complains at link time.

Gotcha, should I file an issue and fix in a later CL? Or should I just fix it in this CL (i.e. revert effects of dddcfc6 only for Windows)

toshipiazza · 2017-10-04T16:12:17Z

I only added 2 tests, one of which tests umbra_{read,write}_shadow_memory(). I'd also like to test umbra_insert_app_to_shadow() (of course). How would I go about doing this? It was mentioned here that "DrMem detects whether [a pointer from the page table points to] a special block and replaces it with a writable block at that time," but does DrMem do this in a fault handler, or is the only logic for this in umbra_{read,write}_shadow_memory()?

derekbruening · 2017-10-05T05:17:17Z

@@ -1,4 +1,4 @@
-#!/bin/sh


Making it bash seems fine: we did the same thing with git_review.sh

derekbruening · 2017-10-05T05:19:07Z

 # a function scope, we must set the global vars ahead of time:
 configure_DynamoRIO_global(OFF ON)

-# We save pdb+dll space by not using libc (xref DRi#714).


On Windows too?

derekbruening · 2017-10-05T05:53:39Z

+        PUSH_SEH(REG_XDI)
+        sub      REG_XSP, FRAME_PADDING /* align */
+        mov 	 REG_XBP, REG_XSP
+        sub		 REG_XSP, 4


The operand alignment looks off here and on the line above. The style guide says "place opcodes on column 8 and start operands on column 17" -- hard to tell from this view which one of lines 78 and 79 is correct.

Done. Looks like this was a tabs vs spaces confusion issue.

derekbruening · 2017-10-05T05:53:50Z

+     test1:
+        mov      REG_XDI, UMBRA_TEST_1_ASM
+        mov      REG_XDI, UMBRA_TEST_1_ASM
+        mov		 [REG_XBP], REG_XDI


Alignment again

derekbruening · 2017-10-05T05:54:00Z

+     test2:
+        mov      REG_XDI, UMBRA_TEST_2_ASM
+        mov      REG_XDI, UMBRA_TEST_2_ASM
+        mov		 REG_XDI, [REG_XBP]


derekbruening · 2017-10-05T05:54:09Z

+        jmp      epilog
+
+    epilog:
+        mov 	 REG_XSP, REG_XBP


derekbruening · 2017-10-05T14:21:19Z

I only added 2 tests, one of which tests umbra_{read,write}shadow_memory(). I'd also like to test umbra_insert_app_to_shadow() (of course). How would I go about doing this? It was mentioned here that "DrMem detects whether [a pointer from the page table points to] a special block and replaces it with a writable block at that time," but does DrMem do this in a fault handler, or is the only logic for this in umbra{read,write}_shadow_memory()?

For inlined instrumentation, it is a fault handler: e.g., https://github.com/DynamoRIO/drmemory/blob/master/drmemory/fastpath.c#L350. For C code it's sthg like this https://github.com/DynamoRIO/drmemory/blob/master/drmemory/shadow.c#L482

toshipiazza · 2017-10-05T18:42:36Z

For inlined instrumentation, it is a fault handler: e.g., https://github.com/DynamoRIO/drmemory/blob/master/drmemory/fastpath.c#L350. For C code it's sthg like this https://github.com/DynamoRIO/drmemory/blob/master/drmemory/shadow.c#L482

Noted. Is there a reason 64-bit Umbra abstracts away the fault handler (i.e. the fault handler appears in umbra_64.c), but 32-bit Umbra does not? Is it due to poor interaction with DrMemory?

derekbruening · 2017-10-05T18:45:37Z

For inlined instrumentation, it is a fault handler: e.g., https://github.com/DynamoRIO/drmemory/blob/master/drmemory/fastpath.c#L350. For C code it's sthg like this https://github.com/DynamoRIO/drmemory/blob/master/drmemory/shadow.c#L482

Noted. Is there a reason 64-bit Umbra abstracts away the fault handler (i.e. the fault handler appears in umbra_64.c), but 32-bit Umbra does not? Is it due to poor interaction with DrMemory?

I don't think there's a good reason: it may just be historical, that DrM has a fault handler there for other reasons. Paging @zhaoqin who wrote umbra -- is there a reason?

- (Hopefully) fixes Windows cmake build error - Fixes whitespace errors in umbra_app.c - Re-adds building without libc on Windows

- tests umbra_insert_app_to_shadow() - tests consistency between umbra_insert_app_to_shadow() and umbra_get_shadow_memory()

toshipiazza · 2017-10-23T00:02:18Z

The travis Mac build seems to be erroring out with the following message:

/usr/local/Homebrew/Library/Homebrew/brew.rb:12:in `<main>': Homebrew must be run under Ruby 2.3! You're running 2.0.0. (RuntimeError)

derekbruening · 2017-10-23T15:13:22Z

The travis Mac build seems to be erroring out with the following message:
/usr/local/Homebrew/Library/Homebrew/brew.rb:12:in `<main>': Homebrew must be run under Ruby 2.3! You're running 2.0.0. (RuntimeError)

DR hit this last week and I was hoping Travis would fix it by now. Searching shows this Travis issue which lists some workarounds (didn't read it all yet):
travis-ci/travis-ci#8552

Suggested in travis-ci/travis-ci#8552

derekbruening · 2017-10-27T15:44:50Z

Thanks for trying this here, I'm doing the same thing for DR: DynamoRIO/dynamorio#2672

toshipiazza · 2017-10-28T01:30:11Z

Is this ready for merge? Were the last few tests I added reviewed?

derekbruening

The tests look good to me. Thanks for adding them!

i#1726: Port Umbra to ARM

37c1960

Pulls out shadow_table_insert_app_to_shadow() into 2 arch-specific methods for x86 and for ARM. Also implements this method for ARM. Issue: #1726

toshipiazza requested a review from derekbruening August 30, 2017 17:35

toshipiazza commented Aug 30, 2017

View reviewed changes

Addresses reviewer concerns

bd4cecd

- Fixes build error/typo in CMakeLists.txt - Replaces movw/movt sequence with instrlist_insert_mov_immed_ptrsz()

derekbruening reviewed Sep 6, 2017

View reviewed changes

Addresses reviewer concerns

8fa6b9e

- Reduces number of regs required for Umbra on ARM - Renames umbra_x86 to umbra_32, and umbra_x64 to umbra_64

derekbruening approved these changes Sep 25, 2017

View reviewed changes

toshipiazza added 2 commits October 2, 2017 15:27

Merge branch 'master' into i1726-port-umbra-arm

506cf8c

Adds Umbra tests for umbra_{read,write}_shadow_memory()

2da3869

toshipiazza commented Oct 4, 2017

View reviewed changes

Re-add /Zi /Ob0 to compile flags to prevent test failures on Windows

89120fe

Fixes clang build (hopefully)

8a2fd10

derekbruening approved these changes Oct 5, 2017

View reviewed changes

toshipiazza added 4 commits October 5, 2017 14:50

Addresses reviewer concerns

3aea45a

- (Hopefully) fixes Windows cmake build error - Fixes whitespace errors in umbra_app.c - Re-adds building without libc on Windows

Adds valid SEH header to umbra_app

c02b59e

Adds more tests

9cc4bfb

- tests umbra_insert_app_to_shadow() - tests consistency between umbra_insert_app_to_shadow() and umbra_get_shadow_memory()

Fixes Windows build

214d3e8

Attempts to fix travis build

f75350d

Suggested in travis-ci/travis-ci#8552

Add xref to original travis issue in .travis.yml

6f737bf

derekbruening approved these changes Oct 28, 2017

View reviewed changes

toshipiazza merged commit 3c0c21e into master Oct 28, 2017

toshipiazza deleted the i1726-port-umbra-arm branch October 28, 2017 04:04

derekbruening mentioned this pull request Jun 24, 2019

Fixes multi init Umbra tables #2200

Merged

Conversation

toshipiazza commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toshipiazza commented Aug 30, 2017

Uh oh!

derekbruening commented Aug 30, 2017

Uh oh!

toshipiazza commented Aug 30, 2017

Uh oh!

derekbruening left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toshipiazza Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbruening commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toshipiazza commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbruening commented Oct 5, 2017

Uh oh!

toshipiazza commented Oct 5, 2017

Uh oh!

toshipiazza Sep 22, 2017 •

edited

Loading