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

Add timestamp marker increase monotonically invariant check. #6037

Conversation

ivankyluk
Copy link
Contributor

Please review the change to add an invariant check to verify timestamp markers are increasing monotonically.

Reference:

From #2039 (comment) and #5704 (comment)

@derekbruening derekbruening self-requested a review May 17, 2023 19:57
@derekbruening
Copy link
Contributor

Thank you for the PR. I will merge it.

@derekbruening
Copy link
Contributor

Well we hit a violation causing test failure in x86-64 tool.drcacheoff.phys-SUDO:
https://github.com/DynamoRIO/dynamorio/actions/runs/5007379198/jobs/8973920770?pr=6037

344:              6           0:       25181 <marker: timestamp 13328827882613812>
344:              7           0:       25181 <marker: tid 25181 on core 1>
344:              8           0:       25181 <marker: physical address for following virtual: 0x24a098000>
344:              9           0:       25181 <marker: virtual address for prior physical: 0x401000>
344:             10           0:       25181 <marker: physical address for following virtual: 0x272d1e00e>
344:             11           0:       25181 <marker: virtual address for prior physical: 0x40200e>
344:             12           0:       25181 <marker: physical address for following virtual: 0x27767c000>
344:             13           0:       25181 <marker: virtual address for prior physical: 0x403000>
344: 
344:   Trace invariant failure in T25181 at ref # 14: Timestamp does not increase

Unfortunately it's not printing ref #14 for us there (not sure who does that printing) -- but looking at process_entry_for_physaddr() it seems to deliberately clone a timestamp, so we have 2 in a row with the same value. Looks like disallowing identical timestamps was a mistake :(

@derekbruening
Copy link
Contributor

Also seeing Windows test failures: Trace invariant failure in T6600 at ref # 63: Timestamp does not increase -- could it really be from consecutive live timestamps? Maybe in these VM's used for testing it's possible to get the same microsecond value.

@ivankyluk
Copy link
Contributor Author

Let me update the code to allow timestamps to stay the same.

@derekbruening
Copy link
Contributor

My original request was misguided it seems: sorry about that. Maybe can just revert the 2nd commit.

@derekbruening
Copy link
Contributor

Wow the 32-bit Windows filter-simple test failed with:

 Trace invariant failure in T1040 at ref # 58: Timestamp is less than the previous one

So did tool.drcachesim.invariants:

Trace invariant failure in T2556 at ref # 299: Timestamp is less than the previous one

So it's not sthg weird with filtering: the invariants test runs the client.winxfer app under regular online tracing. Hmm. We get the timestamp from the OS. Not sure how it can decrease...is it some artifact of -align_endpoints?

@derekbruening
Copy link
Contributor

Wow the 32-bit Windows filter-simple test failed with:

@abhinav92003 reminded me that 32-bit truncates the timestamp counters (!!): which is #5634. So for the short term until that is fixed (32-bit is not a high priority though...) I think we have to restrict this check to 64-bit.

@ivankyluk
Copy link
Contributor Author

I would like to clarify if "drmemtrace 32-bit truncates timestamps" means it's taking the least significant 32 bits?
Or the most significant 32 bits.

If it's taking the least significant 32 bits, the timestamp check can be modified to add a rollover check. For example,
the previous timestamp is 2^31 (or another number) more than the current one, assuming there should be no such gaps in between in non rollover case.

@derekbruening
Copy link
Contributor

Least significant:

$ git grep -A 1 i#5634
clients/drcachesim/tracer/raw2trace.cpp:                                // XXX i#5634: Truncated for 32-bit, as documented.
clients/drcachesim/tracer/raw2trace.cpp-                                (uintptr_t)timestamp);

@ivankyluk
Copy link
Contributor Author

I filed #6187 to track it using a different pull request.

@ivankyluk ivankyluk closed this Jul 14, 2023
@ivankyluk ivankyluk deleted the iX-add-timestamps-monotonically-increase-invariant-check branch July 14, 2023 22:00
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

2 participants