Use shadowstack in SWT cloned module

[Pavel-Durov]

The ShadowStack pass, which transforms allocas into shadow stack pointers, is currently ignoring all unoptimised functions (code reference). However, we require these functions to utilise shadow stack pointers, as tracing is performed within them.

Furthermore, the unoptimised main function must be adjusted to use the same shadow stack instance as the optimised main function. This adjustment is essential because the unoptimised and optimised main functions need to share variables stored on the stack, facilitating the transitions between the versions.

This pull request ensures that both the optimised and unoptimised main functions use the same shadow stack. There is no need to establish a mapping between the allocas of the two functions, as the ordering of allocas remains consistent by definition; the functions are direct copies of one another.

Note: once we have the control point transition implemented, we can remove the shadow stack pointers from the optimised functions completely.

Example of variables in __yk_unopt_main declared before this change:

define dso_local i32 @__yk_unopt_main(i32 noundef %0, ptr noundef %1) local_unnamed_addr #0 !dbg !84 {
  call void @__yk_trace_basicblock(i32 9, i32 0)
  %3 = alloca i32, align 4
  %4 = alloca i32, align 4
  %5 = alloca ptr, align 8
  %6 = alloca ptr, align 8
  %7 = alloca %struct.YkLocation, align 8
  %8 = alloca i32, align 4
  %9 = alloca i32, align 4

Variables in __yk_unopt_main declared after this change:

define dso_local i32 @__yk_unopt_main(i32 noundef %0, ptr noundef %1) local_unnamed_addr #0 !dbg !83 {
  call void @__yk_trace_basicblock(i32 9, i32 0)
  %3 = load ptr, ptr @shadowstack_0, align 8
  %4 = getelementptr i8, ptr %3, i32 0
  %5 = getelementptr i8, ptr %3, i32 4
  %6 = getelementptr i8, ptr %3, i32 8
  %7 = alloca ptr, align 8
  %8 = alloca %struct.YkLocation, align 8
  %9 = getelementptr i8, ptr %3, i32 16
  store i32 0,  %30) #6, !dbg !131, !srcloc !84
  br label %31, !dbg !132

[Pavel-Durov]

We need to change:

if (F.getName() == MAIN || F.getName().startswith(YK_CLONE_PREFIX) == false) {

[Pavel-Durov]

We need to change:

if (F.getName() == MAIN || F.getName().startswith(YK_CLONE_PREFIX) == false) {

We can skip inserting shadowstacks into unopt version once we'll have the cp transition in place.
Current yk SWT is dependent on these shadowstacks.

[ptersilie]

We still use the word clone in a couple of variables and comments here. I think we should get rid of it entirely and always say if we are talking about opt or unopt. Clone is just too confusing, especially as we've changed at least once which one the clone is and I can never remember.

[Pavel-Durov]

Replaced.

[ptersilie]

We need at the very least a comment explaining why we believe that allocas in unopt will match the allocas in opt. But ideally we have asserts here that make sure. We might want to optimise the opt version slightly in the future which could change the variable order and then things will go wrong.

[Pavel-Durov]

Added assertions 👉 fabd8f9

[ltratt]

I'm a dummy, so the updated PR description is still too sparse for me. Can we see why we're doing this (not just how) please?

[Pavel-Durov]

I'm a dummy, so the updated PR description is still too sparse for me. Can we see why we're doing this (not just how) please?

It's been updated, is it clear?

[Pavel-Durov]

Any other values that need to be asserted here?
I noticed that the types are not always the same...
This assertion fails:

assert(optAI->getAllocatedType() == unoptAI->getAllocatedType() &&
               "Alloca types must match");

[ptersilie]

I think that should do. But it's weird that the types don't match. I wasn't expecting that to happen.

[Pavel-Durov]

ok, yeah it's only in on specific yk test ang_tests::sdiv.c

Mismatched alloca types at index 7:
  Optimized alloca type: 0x70
  Unoptimized alloca type: 0x55e49c008e90

[ptersilie]

Yeah that's weird. If you do Instr->dump() it will give your more information which two instructions aren't matching.

[ptersilie]

It's been updated, is it clear?

It will do for now. Though I believe you got the two code blocks the wrong way around. Personally, I probably would have described the change more, rather than showing the code blocks, to give a little more context what is going on. Although a small example certainly doesn't hurt. For example, I might have written something like:

The shadowstack pass, which turns allocas into shadowstack pointers, ignores all optimised functions, since these aren't traced and thus don't require a shadow stack. We forgot, however, that the optimised main function is an exception since it needs to share variables stored on the stack with __yk_unopt_main so that we can transition between the two.

With this PR, we use the shadow stack for the optimised main function as well, e.g. allocas like %4 = alloca i32, align 4 get turned into accesses into the shadow stack like %4 = getelementptr i8, ptr %3, i32 0. There is no need to create a mapping between the variables of main and __yk_unopt_main since by definition the ordering is the same because the functions are copies of each other.

[Pavel-Durov]

It's been updated, is it clear?

It will do for now. Though I believe you got the two code blocks the wrong way around. Personally, I probably would have described the change more, rather than showing the code blocks, to give a little more context what is going on. Although a small example certainly doesn't hurt. For example, I might have written something like:

The shadowstack pass, which turns allocas into shadowstack pointers, ignores all optimised functions, since these aren't traced and thus don't require a shadow stack. We forgot, however, that the optimised main function is an exception since it needs to share variables stored on the stack with __yk_unopt_main so that we can transition between the two.

With this PR, we use the shadow stack for the optimised main function as well, e.g. allocas like %4 = alloca i32, align 4 get turned into accesses into the shadow stack like %4 = getelementptr i8, ptr %3, i32 0. There is no need to create a mapping between the variables of main and __yk_unopt_main since by definition the ordering is the same because the functions are copies of each other.

Thank you, updated.

[ptersilie]

Thank you, updated.

Excellent. This looks good now.