Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions tests/c/guard_body_no_args.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Run-time:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS this test doesn't prove it's testing what it claims. I think we would need to have IR in here and check that there are no arg instructions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current test all interpreter state (g_pc, g_mt, i) are set as globals, so the control point safepoint records no live locals:

  bb0:
    %0_0: ptr = alloca {0: i64}, 1, 8
    %0_1: ptr = call malloc(1000000i64)
    %0_2: ptr = ptr_add %0_1, 16
    *@shadowstack_head = %0_1
    *@shadowstack_0 = %0_2
    %0_5: ptr = ptr_add %0_1, 0
    *%0_5 = 0i32
    br bb1
...
  bb7:
    # guard_body_no_args.c:34: yk_mt_control_point(g_mt, &i);
    %7_0: ptr = load @g_mt
    call llvm.experimental.patchpoint.void(0i64, 13i32, __ykrt_control_point, 3i32, %7_0, @i, 0i64) [safepoint: 0i64, ()]
    # guard_body_no_args.c:35: debug_fn();
    br bb8

In Contrast, if g_pc is moved from a global to a local declared inside main it assigns it a shadow stack slot and it appears as a live variable:

-long g_pc = 0;
+
 YkMT *g_mt = NULL;
 YkLocation i;

   i = yk_location_new();
+  long g_pc = 0;

Produced IR:

  bb0:
    %0_0: ptr = alloca {0: i64}, 1, 8
    %0_1: ptr = call malloc(1000000i64)
    %0_2: ptr = ptr_add %0_1, 16
    *@shadowstack_head = %0_1
    *@shadowstack_0 = %0_2
    %0_5: ptr = ptr_add %0_1, 0
    %0_6: ptr = ptr_add %0_1, 8  ; shadow stack slot for g_pc
    *%0_5 = 0i32
    br bb1
  ...
  bb8:
    # guard_body_no_args.c:35: yk_mt_control_point(g_mt, &i);
    %8_0: ptr = load @g_mt
    call llvm.experimental.patchpoint.void(0i64, 13i32, __ykrt_control_point, 3i32, %8_0, @i, 0i64, %0_6) [safepoint: 0i64, (%0_6)]
    # guard_body_no_args.c:36: debug_fn();
    br bb9

%0_6 is live at the control point and yk emits an Arg instruction.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is: the C code might show this but the lang_tester test doesn't prove this. We'd need YKD_LOG_IR=jit and then guarantee that has no args in it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok got it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated 0699fb1

// env-var: YKD_LOG=4
// env-var: YKD_SERIALISE_COMPILATION=1
// stderr:
// yk-tracing: start-tracing
// ...
// yk-tracing: stop-tracing
// ...
// yk-execution: enter-jit-code
// ...

// Testing that when all interpreter state is in globals the JIT produces
// a trace block with no Arg instructions.
// Previously this caused an unconditional panic in p_block.

#include <stdlib.h>
#include <yk.h>
#include <yk_testing.h>

long g_pc = 0;
YkMT *g_mt = NULL;
YkLocation i;

__attribute__((yk_outline)) static void debug_fn(void) {}

int main(void) {
g_mt = yk_mt_new(NULL);
yk_mt_hot_threshold_set(g_mt, 0);
yk_mt_sidetrace_threshold_set(g_mt, 4);
i = yk_location_new();

for (; g_pc < 20; g_pc++) {
yk_mt_control_point(g_mt, &i);
debug_fn();
if (g_pc > 15) {
break;
}
}

yk_location_drop(i);
yk_mt_shutdown(g_mt);
return EXIT_SUCCESS;
}
8 changes: 3 additions & 5 deletions ykrt/src/compile/j2/hir_to_asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -860,11 +860,9 @@ impl<'a, AB: HirToAsmBackend> HirToAsm<'a, AB> {
break;
}
let Some((iidx, hinst)) = insts_iter.next() else {
// By definition there must be no `Arg` instructions in this trace, which is only
// plausible in testing mode.
#[cfg(not(test))]
panic!();
#[cfg(test)]
// No Arg instructions: Can happen when all live values at this control point are globals.
// Their addresses are baked into the IR, so nothing needs to be passed in
// via registers or stack at trace entry.
break;
};
match hinst {
Expand Down
Loading