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

Default and unspecified values for offline_file_type_t #7228

Open
abhinav92003 opened this issue Jan 28, 2025 · 2 comments
Open

Default and unspecified values for offline_file_type_t #7228

abhinav92003 opened this issue Jan 28, 2025 · 2 comments

Comments

@abhinav92003
Copy link
Contributor

Today, offline_file_type_t has OFFLINE_FILE_TYPE_DEFAULT which perhaps at some point previously was intended for real trace files with no other "special" bit. But as of today, it seems that we would expect atleast one of the arch-related bits (OFFLINE_FILE_TYPE_ARCH_*).

OFFLINE_FILE_TYPE_DEFAULT = 0x00,

Some existing code in opcode_mix does assume that no existing trace may use OFFLINE_FILE_TYPE_DEFAULT-only.

* OFFLINE_FILE_TYPE_DEFAULT (== 0) should never be present alone and can be used as

Also, the scheduler initializes the get_filetype value to 0 (which is essentially OFFLINE_FILE_TYPE_DEFAULT):

uint64_t filetype_ = 0;

But then there are various unit tests that use 0 as a real value for the test trace:

gen_marker(t1, TRACE_MARKER_TYPE_FILETYPE, 0),

and also existing code that uses -1 to mean uninitialized:

if (filetype_ == -1) {

Goal: If it is indeed so that OFFLINE_FILE_TYPE_DEFAULT cannot be present on its own in real traces, perhaps we can use that as the uninitialized sentinel value, which is much cleaner. In that case, we should rename it (keeping the original enum value for backward compatibility) to OFFLINE_FILE_TYPE_UNINITIALIZED.

@abhinav92003
Copy link
Contributor Author

Also some existing if-conditions are set up in a way that shows that we did assume previously that some traces may not have any arch-bit set.

if (TESTANY(OFFLINE_FILE_TYPE_ARCH_ALL & ~OFFLINE_FILE_TYPE_ARCH_REGDEPS,

@derekbruening
Copy link
Contributor

An old-style trace only meant for address analysis such as through drcachesim that has no embedded encodings and so has no need to indicate an architecture would today have zero filetype bits set and still be perfectly valid for analysis tools that don't need to decode instructions. So I'm not sure it's worth completely disallowing such traces just because we want a sentinel value that uses an existing enum value. It seems cleaner to use the -1 or create a new sentinel value than to try to force a value of 0 to be invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants