Skip to content

Drop flag statement creation depends on FxHashMap interation order #91943

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Aaron1011 opened this issue Dec 14, 2021 · 2 comments · Fixed by #110962
Closed

Drop flag statement creation depends on FxHashMap interation order #91943

Aaron1011 opened this issue Dec 14, 2021 · 2 comments · Fixed by #110962
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

This code creates MIR statements based on an FxHashMap iteration order:

for flag in self.drop_flags.values() {
self.patch.add_assign(loc, Place::from(*flag), false_.clone());
}

drop_flags is defined as:

drop_flags: FxHashMap<MovePathIndex, Local>,

Since FxHashMap's fixed seed is different between 32 and 64 bit platforms, we may end up creating statements in a different order on 32-bit vs 64-bit hosts (the target platform passed to the compiler has no effect).

We should probably sort by MovePathIndex before creating the statements.

@Aaron1011 Aaron1011 added the C-bug Category: This is a bug. label Dec 14, 2021
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 15, 2021
@camelid
Copy link
Member

camelid commented Dec 15, 2021

Perhaps it should just use BTreeMap instead?

@pnkfelix pnkfelix added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 1, 2022
@apiraino
Copy link
Contributor

Revisiting this issue to evaluate the priority.

@pnkfelix unsure about the status and global direction of this issue. Do we have some work in progress somewhere? I've tried following the breadcrumbs and landed on #102698. Unsure if it helps and - if yes - how much does that pull request covers to fix this issue.

opinions? thanks :)

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 27, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 29, 2023
@bors bors closed this as completed in 7721c73 Apr 29, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants