i#2016: Port the umbra_64.c class to work on aarch64 #2404

gregcawthorne · 2021-04-28T13:09:56Z

Enables shadow memory tracking on AArch64.

This is a subsection of the larger ongoing AArch64 port of DRMemory.

Issue: #2016

This is a subsection of the larger ongoing aarch64 port of DRMemory.

johnfxgalea

Thank you for the PR! Nice to see support for AARCH64.

I think it is best that we have some tests just to ensure all is working, and avoid regression.

johnfxgalea · 2021-04-28T16:04:46Z

umbra/umbra_64.c

@@ -354,6 +358,37 @@ static ptr_uint_t map_disp_win81[] = {
 * We check conflicts later when creating shadow memory.
 */
 #ifdef LINUX
+#ifdef AARCH64
+static const app_segment_t app_segments_initial[] = {
+    /* We split app3 [0x7F0000000000, 0x800000000000) into two parts:


I see that a big chunk of this comment block was copied from that below. Consider condensing to avoid duplicate text, as future updates would then require double changes.

Removed comment.

johnfxgalea · 2021-04-28T16:07:53Z

umbra/umbra_64.c

@@ -839,7 +875,7 @@ umbra_map_arch_init(umbra_map_t *map, umbra_map_options_t *ops)
    ASSERT(map->shadow_block_size >= ALLOC_UNIT_SIZE &&
           map->app_block_size    >= ALLOC_UNIT_SIZE,
           "block size too small");
-    map->mask = segment_mask(num_seg_bits) | seg_index_mask(num_seg_bits);
+    map->mask = IF_AARCH64_ELSE(0xfffffffffffffffc, segment_mask(num_seg_bits) | seg_index_mask(num_seg_bits));


Avoid magic values in code as this hinders readability.

Is it possible to have segment_mask() return the mask, rather than have the IF_AARCH64_ELSE macro here?

Finally, add a comment on how this mask was derived.

I have now made these changes.

gregcawthorne · 2021-04-28T17:36:40Z

Thank you for the PR! Nice to see support for AARCH64.

I think it is best that we have some tests just to ensure all is working, and avoid regression.

I was hoping the fact that the existing umbra tests pass with the full port, might suffice.

But we could add more specific tests.

johnfxgalea · 2021-04-28T18:11:54Z

@derekbruening Does Dr. Memory's CI run tests for AArch64? IIRC, they used to run prior to the switch to Github Actions?

gregcawthorne · 2021-04-28T18:15:17Z

@derekbruening Does Dr. Memory's CI run tests for AArch64? IIRC, they used to run prior to the switch to Github Actions?

If I recall correctly prior to this body of work DRMemory didn't even compile on AArch64. It only had 32 bit arm support. However this might have changed since I started working on it, or perhaps I misunderstood.

johnfxgalea · 2021-04-28T18:17:14Z

@derekbruening Does Dr. Memory's CI run tests for AArch64? IIRC, they used to run prior to the switch to Github Actions?

If I recall correctly prior to this body of work DRMemory didn't even compile on AArch64. It only had 32 bit arm support. However this might have changed since I started working on it, or perhaps I misunderstood.

Sorry, I meant 32-bit ARM. What we need now is to run AArch64 umbra tests.

derekbruening · 2021-04-28T18:20:54Z

@derekbruening Does Dr. Memory's CI run tests for AArch64? IIRC, they used to run prior to the switch to Github Actions?

If I recall correctly prior to this body of work DRMemory didn't even compile on AArch64. It only had 32 bit arm support. However this might have changed since I started working on it, or perhaps I misunderstood.

Sorry, I meant 32-bit ARM

On Travis there was a 32-bit ARM cross-compile job, but no execution.

What should be done is to add both a32 and a64 QEMU runs (at least some simple tests are likely to run on QEMU given that they do for plain DR) and maybe reuse the DR Jenkins machine and setup to run a64 tests as support is added.

johnfxgalea · 2021-04-28T18:26:26Z

@derekbruening Does Dr. Memory's CI run tests for AArch64? IIRC, they used to run prior to the switch to Github Actions?

If I recall correctly prior to this body of work DRMemory didn't even compile on AArch64. It only had 32 bit arm support. However this might have changed since I started working on it, or perhaps I misunderstood.

Sorry, I meant 32-bit ARM

On Travis there was a 32-bit ARM cross-compile job, but no execution.

What should be done is to add both a32 and a64 QEMU runs (at least some simple tests are likely to run on QEMU given that they do for plain DR) and maybe reuse the DR Jenkins machine and setup to run a64 tests as support is added.

I see... Okay, so in that case, unfortunately we will punt test for this PR.

@derekbruening Is there an issue page regarding the above? I couldn't find it.

johnfxgalea · 2021-04-28T18:29:35Z

@gregcawthorne No rush, just let me know when ready for me to take another look at the PR.

derekbruening · 2021-04-28T18:34:54Z

@derekbruening Is there an issue page regarding the above? I couldn't find it.

Probably not yet in the DrM tracker: the DR QEMU issue is DynamoRIO/dynamorio#4719

gregcawthorne · 2021-04-29T12:10:55Z

@gregcawthorne No rush, just let me know when ready for me to take another look at the PR.

I have made the changes you have suggested, but haven't added a new test.

I am currently unsure of the ci-windows/vs2017-32 build failure in CI.

johnfxgalea · 2021-04-29T12:38:20Z

umbra/umbra_64.c

@@ -354,6 +363,18 @@ static ptr_uint_t map_disp_win81[] = {
 * We check conflicts later when creating shadow memory.
 */
 #ifdef LINUX
+#ifdef AARCH64
+static const app_segment_t app_segments_initial[] = {
+    {(app_pc)0x0000ff0000000000,  (app_pc)0x0001000000000000, 0},


Just having another look, and I don't understand why the layout needs to be different from the existing one. Can you explain, or provide a reference, regarding how you defined this app segment layout please?

Okay I will review this section and get back to you.

Okay so I when you start up DRMemory does a scan of all the current memory blocks. I am pretty sure I simply looked at the default ones I was getting every time on my taishan AArch64 system and copied them in.

umbra/umbra_64.c

johnfxgalea · 2021-04-29T12:40:48Z

@gregcawthorne No rush, just let me know when ready for me to take another look at the PR.

I have made the changes you have suggested, but haven't added a new test.

I am currently unsure of the ci-windows/vs2017-32 build failure in CI.

@gregcawthorne Thank you for the changes. Yes, we will skip tests due to limitations of the CI, but I am assuming you tried the code locally, and all works fine, right?

johnfxgalea · 2021-04-29T12:43:39Z

@gregcawthorne Ignore the failure on the Windows machine. It failed due to a known flaky test (ref: #2361)

johnfxgalea · 2021-04-29T12:45:36Z

umbra/umbra_64.c

+    }
+    instru_insert_mov_pc(drcontext, ilist, where, opnd_create_reg(tmp),
+                             OPND_CREATE_INT64(map->mask));
+    PRE(ilist, where, INSTR_CREATE_and(drcontext,


Why did you place the and instruction last here, in contrast to the existing code below?

yes I have done:
add => shift => and

rather than the existing:
and => add => shift

My understanding is that the purpose of the mapping is to shift and scale into some other distinct space range with the correct scale. The order of operations doesn't matter too much as long as 'a' mapping was achieved.

AArch64 doesn't use the same segmentation scheme as x86 as far as I know.

Therefore I am using the mask variable to simply mask off the lower 2 bits of the address in order to secure a valid address (as commented), and this makes sense to be done as the final operation.

yes I have done:
add => shift => and

rather than the existing:
and => add => shift

My understanding is that the purpose of the mapping is to shift and scale into some other distinct space range with the correct scale. The order of operations doesn't matter too much as long as 'a' mapping was achieved.

I think it can make a difference. The and is used to mask the address, the add is used to include a displacement value to reach a shadow region, and the shift is intended for scaling. While it may not make a difference in some cases, having the and last might clear information injected earlier by the displacement/scaling if we make future changes.

In any case, it is much better to have the same code sequence as other platforms if possible, as otherwise it appears we are doing something special and potentially arises confusion.

AArch64 doesn't use the same segmentation scheme as x86 as far as I know.

Therefore I am using the mask variable to simply mask off the lower 2 bits of the address in order to secure a valid address (as commented), and this makes sense to be done as the final operation.

The mask and displacement values are critically set up in order to have shadow memory regions not collide with app regions. These values were carefully crafted, and some recently generated with the help of automated tools (https://github.com/DynamoRIO/drmemory/blob/master/tools/umbra_layout.py).

Maybe I am missing something, but I still don't understand why we can't use the existing segment mapping for Linux?

gregcawthorne · 2021-05-07T10:05:11Z

Hi guys. Just giving this a nudge.

johnfxgalea · 2021-05-07T11:08:17Z

Hi guys. Just giving this a nudge.

Okay I will review this section and get back to you.

Hey thanks for the nudge.

I was under the impression that I am still awaiting my above comments to be addressed. Particularly: #2404 (comment)

gregcawthorne · 2021-05-07T11:27:44Z

Hi guys. Just giving this a nudge.

Okay I will review this section and get back to you.

Hey thanks for the nudge.

I was under the impression that I am still awaiting my above comments to be addressed. Particularly: #2404 (comment)

Yes I will address that. I was thinking too sequentially.

johnfxgalea · 2021-05-07T11:29:41Z

Hi guys. Just giving this a nudge.

Okay I will review this section and get back to you.

Hey thanks for the nudge.
I was under the impression that I am still awaiting my above comments to be addressed. Particularly: #2404 (comment)

Yes I will address that. I was thinking too sequentially.

Sure, no problem! Thank you.

I added another comment (#2404 (comment)) relating to the above.

gregcawthorne · 2021-05-21T11:38:56Z

Hi.

Sorry for my lateness on this, was out for a week and also missed the notifications.

I took onboard what you have said and have reverted the behaviour you were questioning (different initial segment map + incorrect order of the and/add/shift) and re ran the tests we have ported and we are getting the same tests passing/failing.

I will chalk this up to my early brash guess work when I first started orienting myself.

Thank you!

gregcawthorne · 2021-05-26T13:01:54Z

I can't see currently how my changes could necessarily break x86 on only osx.
Possibly the added import?

johnfxgalea · 2021-05-27T13:13:48Z

@gregcawthorne You're right. The failure doesn't concern your code; it concerns this issue: #2379

johnfxgalea

I had another look. Seems like we are progressing well.

Thank you!

umbra/umbra_64.c

johnfxgalea

I think these are the final comments and once they are fixed, we will be very close to merging. Thank you.

umbra/umbra_64.c

gregcawthorne · 2021-05-28T14:12:14Z

Okay, do you guys have any preferences of which part of the larger initial aarch64 port would be 'best' to upstream next?

johnfxgalea

LGTM. Thank you!

johnfxgalea · 2021-05-28T14:30:24Z

Okay, do you guys have any preferences of which part of the larger initial aarch64 port would be 'best' to upstream next?

Not really, just try to keep the PRs as small as possible to facilitate reviewing!

It would be great if we consider adding the testing architecture for aarch64 before adding more code.(See: #2404 (comment)). If you can help with this, it would be really appreciated (but I understand if you are busy).

johnfxgalea · 2021-05-28T14:33:20Z

@gregcawthorne Merged! Thanks again for the PR!

gregcawthorne · 2021-06-02T12:56:57Z

Hi John, I can add any changes I have made to the tests next sure.

The port actually depends on dynamorio patch I have which is to add the ability to use memory references as arguments in clean calls.

I will work to get that upstreamed too.

johnfxgalea · 2021-06-02T22:44:05Z

Hi John, I can add any changes I have made to the tests next sure.

The port actually depends on dynamorio patch I have which is to add the ability to use memory references as arguments in clean calls.

I will work to get that upstreamed too.

Okay, no problem. Feel free to request me as a reviewer for the DR patch if you wish.

Port the umbra_64.c class to work on aarch64

1bcd295

This is a subsection of the larger ongoing aarch64 port of DRMemory.

johnfxgalea requested changes Apr 28, 2021

View reviewed changes

refactor segment code

b732de7

johnfxgalea reviewed Apr 29, 2021

View reviewed changes

umbra/umbra_64.c Show resolved Hide resolved

johnfxgalea reviewed Apr 29, 2021

View reviewed changes

gregcawthorne added 2 commits May 21, 2021 12:27

revert umbra segments and (and, add, shift) mapping order

49b8671

lint

0d4dde2

johnfxgalea reviewed May 27, 2021

View reviewed changes

umbra/umbra_64.c Show resolved Hide resolved

gregcawthorne added 2 commits May 27, 2021 14:47

add umbra_insert_app_to_shadow_arch num_scratch_regs checks

a5b9b0e

lint

b516a4d

johnfxgalea reviewed May 27, 2021

View reviewed changes

umbra/umbra_64.c Outdated Show resolved Hide resolved

johnfxgalea reviewed May 27, 2021

View reviewed changes

umbra/umbra_64.c Show resolved Hide resolved

gregcawthorne added 2 commits May 27, 2021 15:57

fix indentation

fefb255

change assert to return CMF_ERROR_INVALID_PARAMETER

449b98a

johnfxgalea requested changes May 27, 2021

View reviewed changes

umbra/umbra_64.c Outdated Show resolved Hide resolved

umbra/umbra_64.c Outdated Show resolved Hide resolved

gregcawthorne added 2 commits May 28, 2021 09:58

fix umbra_num_scratch_regs equality

539569d

lint

8013b8e

johnfxgalea approved these changes May 28, 2021

View reviewed changes

johnfxgalea changed the title ~~Port the umbra_64.c class to work on aarch64~~ i#2016: Port the umbra_64.c class to work on aarch64 May 28, 2021

johnfxgalea merged commit 78be9a9 into master May 28, 2021

johnfxgalea deleted the aarch64-umbra-port branch May 28, 2021 14:32

i#2016: Port the umbra_64.c class to work on aarch64 #2404

i#2016: Port the umbra_64.c class to work on aarch64 #2404

Conversation

gregcawthorne commented Apr 28, 2021 • edited by johnfxgalea Loading

johnfxgalea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcawthorne commented Apr 28, 2021

johnfxgalea commented Apr 28, 2021

gregcawthorne commented Apr 28, 2021

johnfxgalea commented Apr 28, 2021 • edited Loading

derekbruening commented Apr 28, 2021

johnfxgalea commented Apr 28, 2021

johnfxgalea commented Apr 28, 2021

derekbruening commented Apr 28, 2021

gregcawthorne commented Apr 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnfxgalea commented Apr 29, 2021

johnfxgalea commented Apr 29, 2021

Choose a reason for hiding this comment

gregcawthorne Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

johnfxgalea May 7, 2021 • edited Loading

Choose a reason for hiding this comment

gregcawthorne commented May 7, 2021

johnfxgalea commented May 7, 2021

gregcawthorne commented May 7, 2021

johnfxgalea commented May 7, 2021 • edited Loading

gregcawthorne commented May 21, 2021

gregcawthorne commented May 26, 2021 • edited Loading

johnfxgalea commented May 27, 2021

johnfxgalea left a comment

Choose a reason for hiding this comment

johnfxgalea left a comment

Choose a reason for hiding this comment

gregcawthorne commented May 28, 2021

johnfxgalea left a comment

Choose a reason for hiding this comment

johnfxgalea commented May 28, 2021

johnfxgalea commented May 28, 2021

gregcawthorne commented Jun 2, 2021 • edited Loading

johnfxgalea commented Jun 2, 2021

gregcawthorne commented Apr 28, 2021 •

edited by johnfxgalea

Loading

johnfxgalea commented Apr 28, 2021 •

edited

Loading

gregcawthorne Apr 29, 2021 •

edited

Loading

johnfxgalea May 7, 2021 •

edited

Loading

johnfxgalea commented May 7, 2021 •

edited

Loading

gregcawthorne commented May 26, 2021 •

edited

Loading

gregcawthorne commented Jun 2, 2021 •

edited

Loading