Skip to content

pin_exec_driven: fix Inst_Info lookup aliasing across cores #20

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

Open
iconstan01 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

pin_exec_driven: fix Inst_Info lookup aliasing across cores #20

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c15d84
pin_exec_driven: fix Inst_Info lookup aliasing across cores
Mar 11, 2026
1f96562
pin_exec_driven: clarify canonical lookup comment
Mar 12, 2026
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
15 changes: 11 additions & 4 deletions src/pin/pin_lib/uop_generator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,9 @@ static uns generate_uops(uns8 proc_id, ctype_pin_inst* pi, Trace_Uop** trace_uop
void convert_pinuop_to_t_uop(uns8 proc_id, ctype_pin_inst* pi, Trace_Uop** trace_uop) {
Flag new_entry = FALSE;
Inst_Info* info;
// Use canonical address (proc_id=0) for consistent hash key within per-core table.
// The uop cache key should use the original program address bits only.
Comment on lines +786 to +787
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The comment says the key should use the original program address bits only, but the code explicitly stores the per-core tagged address into info->addr. If info->addr is intended to remain tagged (as the previous assert implied), please adjust the comment to avoid implying that Inst_Info::addr is untagged/canonical. If Inst_Info::addr is intended to be canonical, then rebinding it to a tagged address contradicts that and should be reworked (e.g., store canonical and tagged addresses in distinct fields).

Suggested change
// Use canonical address (proc_id=0) for consistent hash key within per-core table.
// The uop cache key should use the original program address bits only.
// Use canonical address (proc_id=0) for the uop cache key so it depends only on
// the original program address bits, independent of any per-core address tagging.

Copilot uses AI. Check for mistakes.
Addr lookup_addr = convert_to_cmp_addr(0, pi->instruction_addr);
Comment on lines +786 to +788
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

convert_to_cmp_addr(0, ...) reads like it adds CMP tagging rather than stripping/normalizing it, which makes the canonicalization intent ambiguous and easy to misuse later. Consider switching to (or introducing) an explicitly-named helper that extracts the untagged program address bits (e.g., a strip_cmp_tag()/canonicalize_inst_addr()), then (if needed) re-tagging in a clearly separate step.

Copilot uses AI. Check for mistakes.
// Due to JIT compilation, each branch must be decoded to verify which instruction the PC maps to.
// To decrease unnecessary malloc/free, fetch inst_info from hashmap
// instead of allocating. However first instruction must be decoded.
Expand All @@ -797,7 +800,7 @@ void convert_pinuop_to_t_uop(uns8 proc_id, ctype_pin_inst* pi, Trace_Uop** trace
info->fake_inst = TRUE;
info->fake_inst_reason = pi->fake_inst_reason;
} else {
info = cpp_hash_table_access_create(proc_id, pi->instruction_addr, pi->inst_binary_lsb, pi->inst_binary_msb, 0,
info = cpp_hash_table_access_create(proc_id, lookup_addr, pi->inst_binary_lsb, pi->inst_binary_msb, 0,
&new_entry);
info->fake_inst = FALSE;
info->fake_inst_reason = WPNM_NOT_IN_WPNM;
Expand Down Expand Up @@ -838,7 +841,7 @@ void convert_pinuop_to_t_uop(uns8 proc_id, ctype_pin_inst* pi, Trace_Uop** trace
info->fake_inst = TRUE;
info->fake_inst_reason = pi->fake_inst_reason;
} else {
info = cpp_hash_table_access_create(proc_id, pi->instruction_addr, pi->inst_binary_lsb, pi->inst_binary_msb,
info = cpp_hash_table_access_create(proc_id, lookup_addr, pi->inst_binary_lsb, pi->inst_binary_msb,
ii, &new_entry);

info->fake_inst = FALSE;
Expand Down Expand Up @@ -889,14 +892,18 @@ void convert_pinuop_to_t_uop(uns8 proc_id, ctype_pin_inst* pi, Trace_Uop** trace

for (ii = 0; ii < num_uop; ii++) {
if (ii > 0) {
info = cpp_hash_table_access_create(proc_id, pi->instruction_addr, pi->inst_binary_lsb, pi->inst_binary_msb, ii,
info = cpp_hash_table_access_create(proc_id, lookup_addr, pi->inst_binary_lsb, pi->inst_binary_msb, ii,
&new_entry);
}
ASSERT(proc_id, !new_entry);

trace_uop[ii]->info = info;
trace_uop[ii]->eom = FALSE;
ASSERT(proc_id, info->addr == pi->instruction_addr);
if (info->addr != pi->instruction_addr) {
// A stale entry can differ only by CMP core bits; rebind it to this core's tagged address.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The comment says the key should use the original program address bits only, but the code explicitly stores the per-core tagged address into info->addr. If info->addr is intended to remain tagged (as the previous assert implied), please adjust the comment to avoid implying that Inst_Info::addr is untagged/canonical. If Inst_Info::addr is intended to be canonical, then rebinding it to a tagged address contradicts that and should be reworked (e.g., store canonical and tagged addresses in distinct fields).

Suggested change
// A stale entry can differ only by CMP core bits; rebind it to this core's tagged address.
// A stale Inst_Info entry may differ from the current instance only in CMP core-tag bits.
// In that case, we intentionally rebind Inst_Info::addr to this core's tagged address so
// that Inst_Info::addr may hold a per-core tagged address rather than a strictly canonical one.

Copilot uses AI. Check for mistakes.
ASSERT(proc_id, convert_to_cmp_addr(0, info->addr) == convert_to_cmp_addr(0, pi->instruction_addr));
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

This computes convert_to_cmp_addr(0, ...) twice in a per-uop loop. Since pi->instruction_addr is constant for the instruction and info->addr is also available, consider precomputing the canonicalized values once (e.g., canonical pi address before the loop, and/or canonical info address just before the assert) to reduce repeated work in hot code.

Copilot uses AI. Check for mistakes.
info->addr = pi->instruction_addr;
}
ASSERT(proc_id, info->trace_info.inst_size == pi->size);

Flag is_last_uop = (ii == (num_uop - 1));
Expand Down