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.

Sign up for GitHub

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

Jump to bottom

i#6417 x86-32 client.flush: Fix tag for client events #6480

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

i#6417 x86-32 client.flush: Fix tag for client events #6480

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
21791cc
i#6417 x86-32 flakiness: debug client.flush
abhinav92003 Nov 29, 2023
7c449a2
Use dr_fragment_app_pc with increment/decrement
abhinav92003 Dec 13, 2023
2f304ad
Revert "i#6417 x86-32 flakiness: debug client.flush"
abhinav92003 Dec 13, 2023
7dd94e1
Reapply "i#6417 x86-32 flakiness: debug client.flush"
abhinav92003 Dec 13, 2023
3a68ba3
Revert "Use dr_fragment_app_pc with increment/decrement"
abhinav92003 Dec 13, 2023
ee1eb05
Replace displaced_pc with return_pc for the tag
abhinav92003 Jan 24, 2024
1989ac3
Revert "i#6417 x86-32 flakiness: debug client.flush"
abhinav92003 Jan 25, 2024
2e26f41
Merge branch 'master' into i6417-debug-tmate
abhinav92003 Jan 25, 2024
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
12 changes: 6 additions & 6 deletions suite/tests/client-interface/flush.dll.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* **********************************************************
* Copyright (c) 2011-2021 Google, Inc. All rights reserved.
* Copyright (c) 2011-2023 Google, Inc. All rights reserved.
* Copyright (c) 2007-2010 VMware, Inc. All rights reserved.
* **********************************************************/

Expand Down Expand Up @@ -79,7 +79,7 @@ find(void *tag)
}

static void
increment(void *tag)
increment(app_pc tag)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a deeper issue with the DR interface. The tag is supposed to be the canonical identifier and should track the fragment from creation to deletion. I thought the tag was already tweaked for hooks and I was disturbed when you described it varying: that is not supposed to happen. I think we have to fix that underlying issue. The alternative of changing the docs to say "ignore the tag" seems too disruptive and I suspect quite a few tools, both our own code and 3rd-party, would have to be rewritten. Some very large and complex tools like Dr. Memory use tags for this type of tracking -- and they have been run extensively on Windows which has many more hooks than just this 32-bit-only vsyscall on Linux. That's what's so confusing: if the tag is really wrong at creation time, why didn't all these other tools break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought you mentioned that clients should be using dr_fragment_app_pc and not just using the tag directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

If clients want to get a real PC (e.g., to attribute to a library/executable) they should use dr_fragment_app_pc: they should not assume a tag is anything but a random number.

There are numerous other interfaces which take tags: dr_replace_fragment, dr_fragment_exists_at, dr_mark_trace_head, etc. If tags really aren't reliable how did all of those work all this time? If we say no one should use tags, we have to change all those interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we document anywhere that the fragment tag can be used directly? On Windows, dr_fragment_app_pc seems to be doing more work and perhaps is more important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. you mean clients should still be able to uniquely identify fragments using tags. They shouldn't need to go through dr_fragment_app_pc for just identifying fragments. That I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two directions of conversion, tag to pc and pc to tag. The underlying code is get_app_pc_from_intercept_pc and get_intercept_pc_from_app_pc. I think we need to add the this Linux hok to the latter to solve this and get the right tag. See the call to that function -- I guess guarded by could_be_hook_occluded_pc() so that needs a look too. That's where bb->start_pc is updated. bb->start_pc becomes the tag. Err...later there is another game played with selfmod....I guess ignore that for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we document anywhere that the fragment tag can be used directly?

bb event says " * - \p tag is a unique identifier for the basic block fragment.

  • Use dr_fragment_app_pc() to translate it to an application address."

{
elem_t *elem;

Expand All @@ -106,7 +106,7 @@ increment(void *tag)
}

static void
decrement(void *tag)
decrement(app_pc tag)
{
elem_t *elem;

Expand Down Expand Up @@ -161,14 +161,14 @@ static dr_emit_flags_t
trace_event(void *drcontext, void *tag, instrlist_t *trace, bool translating)
{
if (!translating)
increment(tag);
increment(dr_fragment_app_pc(tag));
return DR_EMIT_DEFAULT;
}

static void
deleted_event(void *dcontext, void *tag)
{
decrement(tag);
decrement(dr_fragment_app_pc(tag));
}

void
Expand Down Expand Up @@ -246,7 +246,7 @@ bb_event(void *drcontext, void *tag, instrlist_t *bb, bool for_trace, bool trans
{
instr_t *instr;
if (!translating)
increment(tag);
increment(dr_fragment_app_pc(tag));

/* I'm looking for a specific BB in the test .exe. I've marked
* it with a couple nops.
Expand Down
Loading