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

WIP - Method call expressions #269

Merged
merged 3 commits into from
Oct 23, 2019
Merged

Conversation

camchenry
Copy link
Contributor

@camchenry camchenry commented Oct 11, 2019

This is a work in progress branch. I want to share my code so that I can get feedback from others. I started out by adding tests for method calls but stumbled upon some repeatable examples which do not get covered. I hope that maybe we can fix them in this branch in addition to testing them.

Here are the results from running tarpaulin on these tests:
image

It seems that the last line of a chain of method calls is counted as uncovered sometimes.

@xd009642
Copy link
Owner

So this is a known issue #136 if you want to tackle it at the same time (though it might be big or worth another pr) your can use my minitarp repo and the instrumentation points from a debug printout to start testing on the binary.

Then also objdump -WL should show other potential addresses to instrument for each line and try to figure out from if alternative choices improve coverage how the instrumentation choices made in SRC/test_loadrr.rs can be improved.

Disclaimer: this information is from memory as I'm on my phone on my commute home

@xd009642
Copy link
Owner

Alternatively, we can mark the test as one to ignore and remove that attribute when the other issue is fixed

@camchenry
Copy link
Contributor Author

Thanks for the advice, I downloaded minitarp and I'm looking at the dumped assembly now for the examples. If I can't figure anything out after a bit I'll just revise the PR to be regression tests only.

@camchenry
Copy link
Contributor Author

I decompiled the output for this simple example:

#[test]
fn simple_method_call() {
    // ...

    let is_4_in_list = // line 30
        Vec::new() // line 31
        .contains(&4); // line 32
}

into this (with demangled symbols) and marked the lines which had breakpoints added for the lines

<method_calls::simple_method_call>:
    405bf0:    sub    $0x48,%rsp
L31 405bf4:    lea    0x20(%rsp),%rdi
    405bf9:    callq  405790 <alloc::vec::Vec<T>::new>
    405bfe:    jmp    405c0c <method_calls::simple_method_call+0x1c>
    405c00:    mov    0x38(%rsp),%rdi
    405c05:    callq  4040e0 <_Unwind_Resume@plt>
    405c0a:    ud2          # undefined instruction?
    405c0c:    lea    0x20(%rsp),%rdi
    405c11:    callq  405850 <<alloc::vec::Vec<T> as core::ops::deref::Deref>::deref>
    405c16:    mov    %rdx,0x10(%rsp)
    405c1b:    mov    %rax,0x8(%rsp)
    405c20:    jmp    405c22 <method_calls::simple_method_call+0x32>
    405c22:    lea    0xb44e3(%rip),%rdx        # 4ba10c <str.1+0xac>
    405c29:    mov    0x8(%rsp),%rdi
    405c2e:    mov    0x10(%rsp),%rsi
    405c33:    callq  404730 <core::slice::<impl [T]>::contains>
    405c38:    mov    %al,0x7(%rsp)
    405c3c:    jmp    405c4a <method_calls::simple_method_call+0x5a>
L32 405c3e:    lea    0x20(%rsp),%rdi
    405c43:    callq  405710 <core::ptr::real_drop_in_place>
    405c48:    jmp    405c00 <method_calls::simple_method_call+0x10>
    405c4a:    mov    0x7(%rsp),%al
    405c4e:    and    $0x1,%al
    405c50:    mov    %al,0x1f(%rsp)
    405c54:    lea    0x20(%rsp),%rdi
    405c59:    callq  405710 <core::ptr::real_drop_in_place>
    405c5e:    add    $0x48,%rsp
    405c62:    retq
    405c63:    mov    %edx,%ecx
    405c65:    mov    %rax,0x38(%rsp)
    405c6a:    mov    %ecx,0x40(%rsp)
    405c6e:    jmp    405c3e <method_calls::simple_method_call+0x4e>

However, that line is never hit, hence why it is not counted as covered.
image

I'm not an x86 expert, but it seems like the correct line for coverage would be 0x405c33? That line is executed, but I don't know why the DWARF format has 0x405c3e as the instruction for line 31. Maybe you could give some advice here?

@xd009642
Copy link
Owner

I'm not going to have time to properly try this out on my pc and figure things out. But if you get the decoded lines output objdump -WL you can see the line rows in sequence from DWARF. You'll see multiple instructions mapped to a single line. One of the things that tarpaulin needs is a smarter choosing of which address to pick and handling the fact that one line might be present a ton of times

@xd009642
Copy link
Owner

Oh one thing I forgot about, breakpoint addresses are aligned to words, so mentally zero out the 3 least significant bits to get the aligned address for the breakpoint

@xd009642
Copy link
Owner

Okay I have an idea that seems to have legs after some plays with minitarp. If the prior instruction is a jump, instrument that as well as it may cause the instruction to be skipped. However, I need to find out if that would cause false positives in things like if statements...

@camchenry
Copy link
Contributor Author

This might be a stupid question, but what's the way to know if a line is an if statement in tarpaulin? That is, what changes are you making and where to answer something like that?

@camchenry
Copy link
Contributor Author

Also, let me know if there are some tests that I can write to help with that. I feel like it might be much easier to make these changes after we add some regression tests for them. Or at least, we will have more confidence in making them.

@xd009642
Copy link
Owner

For coverage the existing tests and I have some larger projects I run it on and check the results of for sanity. Test wise it just needs tests that are <100% covered and matching the lines covered/uncovered.

Thinking of it I don't think jump and branches are too massive a problem I'll have to experiment though

@camchenry
Copy link
Contributor Author

@xd009642 In order to not let this stagnate, I'm going to just add the #[ignore] attribute to this test for now. But we should remove it whenever #136 gets updated to fix some of these issues. These can be used as regression tests and also as a guide while fixing the problem.

@camchenry camchenry marked this pull request as ready for review October 23, 2019 13:24
@xd009642
Copy link
Owner

yeah good call, I was working on the trace point improvements but there's a lot of analysis work that needs to be done as the naive solution didn't work

@xd009642 xd009642 merged commit 33637cc into xd009642:develop Oct 23, 2019
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.

2 participants