Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement local calls in JIT/interpreter. #271

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

hawkinsw
Copy link
Collaborator

No description provided.

@hawkinsw hawkinsw self-assigned this May 26, 2023
@hawkinsw hawkinsw force-pushed the local_call branch 2 times, most recently from c66637c to d2637df Compare May 30, 2023 01:47
@hawkinsw hawkinsw changed the title WIP: Implement local calls in interpreter. WIP: Implement local calls in JIT/interpreter. May 30, 2023
@hawkinsw
Copy link
Collaborator Author

@Alan-Jowett Although I have still labeled it as WIP, I would love your feedback. The JIT and interpreter are now able to handle local function calls. I am sure that there are probably things that I will need to clean up to meet your high standards, but I would really appreciate your input.

Thanks for letting me participate!
Will

vm/ubpf_jit_x86_64.c Outdated Show resolved Hide resolved
@Alan-Jowett
Copy link
Collaborator

This looks like a good approach. You might want to consider adding support for local functions from ELF files? If I recall correctly, LLVM emits relocations for local calls and the ELF loader then needs to stitch re-assemble it into a single contiguous block of byte code. Feel free to punt this and file an issue to track it for later.

@hawkinsw
Copy link
Collaborator Author

This looks like a good approach. You might want to consider adding support for local functions from ELF files? If I recall correctly, LLVM emits relocations for local calls and the ELF loader then needs to stitch re-assemble it into a single contiguous block of byte code. Feel free to punt this and file an issue to track it for later.

You are, obviously, absolutely correct! I have a WIP for that (which I dropped when I realized that calls themselves were not even implemented). If it's okay, I will put that in a separate issue to track.

I will track aarch64 separately, too, if that's okay!?

@hawkinsw hawkinsw changed the title WIP: Implement local calls in JIT/interpreter. Implement local calls in JIT/interpreter. May 31, 2023
@hawkinsw
Copy link
Collaborator Author

@Alan-Jowett Thanks for the feedback! I believe that the latest version is ready for full review! I have a few PRs staged that I will fire off once I finalize your incredibly helpful feedback!

Thank you again for everything!
Will

@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 2, 2023

@Alan-Jowett I think that this updated PR will give us support for both x86_64 and aarch64 interpreted/jit'd calls for local functions.

I have tested that all tests pass on aarch64 hardware running Ubuntu 22.04.2 LTS and on x86_64 hardware running Fedora Core 37 (Desktop).

I will test on a local m2-based mac mini in a few.

As always, it's awesome to work on this tool with you!

@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 4, 2023

@Alan-Jowett FYI: I have compiled/executed the tests for local calls on macOS and everything appears to work correctly. Sorry for the delay!

Will

@Alan-Jowett
Copy link
Collaborator

@hawkinsw Do you know why the arm64 and macos changes are failing in this PR?

@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 7, 2023

@hawkinsw Do you know why the arm64 and macos changes are failing in this PR?

I do not, unfortunately! When I run the PR on my mac everything passes. I will dig in and see why that test fails! Sorry!

@Alan-Jowett
Copy link
Collaborator

@hawkinsw The change is looking good. Seems like it's down to a single failure in the MacOS case?

You can debug it by running the byte code directly in the ubpf_plugin under a debugger with:

alanjo@alanjo-dev:~/ubpf/build$ bin/ubpf_plugin --program "b7  01  00  00  61  62  63  78  63  1a  f8  ff  00  00  00  00  b7  06  00  00  00  00  00  00  73  6a  fc  ff  00  00  00  00  73  6a  f4  ff  00  00  00  00  b7  01  00  00  61  62  63  79  63  1a  f0  ff  00  00  00  00  bf  a1  00  00  00  00  00  00  07  01  00  00  f8  ff  ff  ff  bf  12  00  00  00  00  00  00  85  00  00  00  04  00  00  00  bf  01  00  00  00  00  00  00  b7  00  00  00  01  00  00  00  67  01  00  00  20  00  00  00  77  01  00  00  20  00  00  00  55  01  0b  00  00  00  00  00  bf  a1  00  00  00  00  00  00  07  01  00  00  f8  ff  ff  ff  bf  a2  00  00  00  00  00  00  07  02  00  00  f0  ff  ff  ff  85  00  00  00  04  00  00  00  bf  01  00  00  00  00  00  00  67  01  00  00  20  00  00  00  77  01  00  00  20  00  00  00  b7  00  00  00  01  00  00  00  1d  61  01  00  00  00  00  00  b7  00  00  00  00  00  00  00  95  00  00  00  00  00  00  00" --jit
0

I am curious as to what it is returning.

The original C program was:

extern int strcmp_ext(const char *a, const char *b);

int entry(int *mem)
{
    char a[] = "abcx";
    char b[] = "abcy";

    if (strcmp_ext(a, a) != 0) {
        return 1;
    }

    if (strcmp_ext(a, b) == 0) {
        return 1;
    }

    return 0;
}

See: https://github.com/iovisor/ubpf/blob/main/tests/string-stack.data

@hawkinsw hawkinsw force-pushed the local_call branch 2 times, most recently from 21c8867 to 6e62944 Compare June 8, 2023 05:32
@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 8, 2023

I spent additional time on the failure tonight. I spun up an x86-based macos host on AWS (to see if the problem was down to architecture) and couldn't reproduce the error there either. The only major difference that I saw was that the AppleClang compiler in the github runners is a full version behind the ones that I am using to test. I fail to see how that could possibly be the problem but I can't see anything else, either.

I wonder if there is a way to ssh in to a github runner as it is executing and see what is happening. I will do some research and investigate further.

Sorry again for the trouble!
Will

@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 8, 2023

As a follow-up: It looks like debugging may be possible: https://github.com/marketplace/actions/debugging-with-ssh I will look further tomorrow!

@hawkinsw hawkinsw force-pushed the local_call branch 3 times, most recently from d5b2523 to 788b886 Compare June 9, 2023 01:55
@coveralls
Copy link

coveralls commented Jun 9, 2023

Coverage Status

coverage: 80.957% (+0.2%) from 80.72% when pulling f15cfbd on hawkinsw:local_call into 1d20064 on iovisor:main.

@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 9, 2023

@Alan-Jowett We did it!!

@Alan-Jowett
Copy link
Collaborator

@Alan-Jowett We did it!!

Awesome! Thanks for working on this!

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
@hawkinsw
Copy link
Collaborator Author

hawkinsw commented Jun 9, 2023

@Alan-Jowett Made a (final?) update that will remove an additional integer operation from the generated code.

@Alan-Jowett Alan-Jowett merged commit 2e3b039 into iovisor:main Jun 9, 2023
@hawkinsw hawkinsw deleted the local_call branch June 17, 2023 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants