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

mcount: Restore/reset rstack only when the return address is hijacked #1841

Merged
merged 1 commit into from Nov 6, 2023

Conversation

jyf111
Copy link
Contributor

@jyf111 jyf111 commented Oct 30, 2023

When specifying estimate_return, the return address is not hijacked. This patch ensures that mcount_rstack_reset_exception() is not called in this case, to avoid recording incorrect end times.

Before:
I got an inverted time bug in estimate_return mode (it only occurs when using dynamic tracing).
I found that mcount_rstack_reset_exception() was called unexpectedly, which sets the end time using mcount_gettime(), but the end time should be estimated by mcount_rstack_inject_return().
This PR should fix this small problem.

$ clang++ tests/s-exception3.cpp -o test/exp
$ uftrace -P. -e test/exp
# DURATION     TID     FUNCTION
            [117574] | main() {
            [117574] |   A::A() {
            [117574] |     foo1() {
            [117574] |       foo2() {
            [117574] |         foo4() {
   5.050 us [117574] |           C::C();
            [117574] |           foo5() {
 129.894 us [117574] |             __cxa_allocate_exception();
  81.546 us [117574] |           } /* foo5 */
  36.698 us [117574] |           C::~C();
 515.376 us [117574] |         } /* foo4 */
 547.074 us [117574] |       } /* foo2 */
                     |          /* inverted time: broken data? */
 289.136 us [117574] |     } /* foo1 */
  13.199 us [117574] |     B::B();
  17.199 us [117574] |     B::~B();
                     |        /* inverted time: broken data? */
 335.734 us [117574] |   } /* A::A */
   5.700 us [117574] |   A::~A();
            [117574] |   catch_exc() {
            [117574] |     bar() {
   3.450 us [117574] |       B::B();
            [117574] |       bar1() {
            [117574] |         bar2() {
   3.549 us [117574] |           C::C();
   7.199 us [117574] |           __cxa_allocate_exception();
  10.399 us [117574] |           C::~C();
  64.897 us [117574] |         } /* bar2 */
                     |            /* inverted time: broken data? */
  42.348 us [117574] |       } /* bar1 */
   3.749 us [117574] |       B::~B();
            [117574] |       catch_exc() {
  73.298 us [117574] |         baz();
  86.498 us [117574] |       } /* catch_exc */
 235.792 us [117574] |     } /* bar */
 261.492 us [117574] |   } /* catch_exc */
 988.659 us [117574] | } /* main */

@@ -1356,7 +1356,7 @@ static int __mcount_entry(unsigned long *parent_loc, unsigned long child, struct
return -1;
}

if (unlikely(mtdp->in_exception)) {
if (!mcount_estimate_return && unlikely(mtdp->in_exception)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the fix. I think it's better to move this check to mcount_rstack_reset_exception() to minimize the change and to be in sync with mcount_rstack_reset().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I also found that if I don't do this, mtdp->in_exception will not be reset to false, which may cause other issues.
Similarly, I removed the check for mcount_estimate_return in __cxa_begin_catch().

Copy link
Owner

Choose a reason for hiding this comment

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

Actually I don't remember why I put it in __cxa_begin_catch() but it seems ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is introduced by 27b0f44

When specifying estimate_return, the return address is not hijacked.
This patch ensures that mcount_rstack_reset_exception() is not called
in this case, to avoid recording incorrect end times.

Signed-off-by: Yufeng Jin <jinyufeng2000@gmail.com>
Copy link
Collaborator

@honggyukim honggyukim left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. The change looks correct and I've confirmed that the inverted time: broken data? message got disappeared by this change.

@namhyung namhyung merged commit 567648d into namhyung:master Nov 6, 2023
3 checks passed
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.

None yet

3 participants