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

Upgrade to hashbrown 0.15 #30

Merged
merged 6 commits into from
Dec 6, 2024
Merged

Upgrade to hashbrown 0.15 #30

merged 6 commits into from
Dec 6, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Dec 4, 2024

RUSTSEC-2024-0402 briefly stated that it affected hashbrown 0.14, but that was later corrected. Probably still good to upgrade!

tests/linked_hash_set.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@djc
Copy link
Contributor Author

djc commented Dec 4, 2024

Current CI failure:

     Running tests/linked_hash_set.rs (target/x86_64-unknown-linux-gnu/debug/deps/linked_hash_set-4316cf24f2740083)
MemorySanitizer: CHECK failed: msan_linux.cpp:193 "((personality(old_personality | ADDR_NO_RANDOMIZE))) != ((-1))" (0xffffffffffffffff, 0xffffffffffffffff) (tid=4338)
    <empty stack>

error: test failed, to rerun pass `--test linked_hash_set`

Caused by:
  process didn't exit successfully: `/home/circleci/project/target/x86_64-unknown-linux-gnu/debug/deps/linked_hash_set-4316cf24f2740083` (exit status: 1)

Not sure if that's caused by my clippy fixes here...

@kyren
Copy link
Owner

kyren commented Dec 6, 2024

Current CI failure:

     Running tests/linked_hash_set.rs (target/x86_64-unknown-linux-gnu/debug/deps/linked_hash_set-4316cf24f2740083)
MemorySanitizer: CHECK failed: msan_linux.cpp:193 "((personality(old_personality | ADDR_NO_RANDOMIZE))) != ((-1))" (0xffffffffffffffff, 0xffffffffffffffff) (tid=4338)
    <empty stack>

error: test failed, to rerun pass `--test linked_hash_set`

Caused by:
  process didn't exit successfully: `/home/circleci/project/target/x86_64-unknown-linux-gnu/debug/deps/linked_hash_set-4316cf24f2740083` (exit status: 1)

Not sure if that's caused by my clippy fixes here...

I don't think it's you, I just tried the build of the master branch and it has started failing in exactly the same way.

Interestingly, gc-arena has also started failing with msan in the same way recently (also some weird linking issue that I never fully tracked down). The only thing in common with the two projects I can think of would be hashbrown itself? I'm not going to pretend to be knowledgeable enough to track that down right now, but I am a little concerned (but also it wouldn't surprise me if it was a false positive either, I just can't be sure).

Do you happen to know what kind of things (if there even are any) that msan would catch that wouldn't be caught by miri?

@djc
Copy link
Contributor Author

djc commented Dec 6, 2024

Nope. @Amanieu would you be able to take a look?

@kyren
Copy link
Owner

kyren commented Dec 6, 2024

The only thing in common with the two projects I can think of would be hashbrown itself?

Also this is meaningless because it's not even used in tests right now, it's just an optional dependency to implement a trait on it. gc-arena does use it via std in tests right now ofc but... so does basically everything haha. Anyway I highly doubt it's anything to do with hashbrown, I only mentioned that to say if it's not a false positive it's two entirely separate projects that potentially made the same error uncovered at the exact same time?

@djc
Copy link
Contributor Author

djc commented Dec 6, 2024

I don't think it's you, I just tried the build of the master branch and it has started failing in exactly the same way.

Interestingly, gc-arena has also started failing with msan in the same way recently (also some weird linking issue that I never fully tracked down). The only thing in common with the two projects I can think of would be hashbrown itself? I'm not going to pretend to be knowledgeable enough to track that down right now, but I am a little concerned (but also it wouldn't surprise me if it was a false positive either, I just can't be sure).

Might be interesting to look at what else changed. Maybe Rust 1.82 vs 1.83?

@djc djc force-pushed the hashbrown-0.15 branch 3 times, most recently from 617212f to 05b4c3a Compare December 6, 2024 13:12
@kyren
Copy link
Owner

kyren commented Dec 6, 2024

Thank you for all of the cleanup, everything looks great to me. I'll merge this now, but if I can I want to figure out the CI failure before doing another release? I guess... that's not actually a good reason to wait since whatever the cause is is also in the previous release (if it is even real), I just want to see if anyone has any insight really. If I have time I'll try and look into it but I'm out of my normal depth so it would probably take me a long time.

@kyren kyren merged commit fa27344 into kyren:master Dec 6, 2024
1 check failed
@djc
Copy link
Contributor Author

djc commented Dec 6, 2024

Thank you for all of the cleanup, everything looks great to me. I'll merge this now, but if I can I want to figure out the CI failure before doing another release? I guess... that's not actually a good reason to wait since whatever the cause is is also in the previous release (if it is even real), I just want to see if anyone has any insight really. If I have time I'll try and look into it but I'm out of my normal depth so it would probably take me a long time.

Yeah, I think it would still make sense to release since it's not a regression from the previous release. But, ultimately up to you.

@kyren
Copy link
Owner

kyren commented Dec 6, 2024

Yeah, I think it would still make sense to release since it's not a regression from the previous release. But, ultimately up to you.

I won't let it wait very long, it's not a regression so if nobody has time to look into it within the next few days I'll just do a new release.

@kyren
Copy link
Owner

kyren commented Dec 9, 2024

Yeah, I think it would still make sense to release since it's not a regression from the previous release. But, ultimately up to you.

I won't let it wait very long, it's not a regression so if nobody has time to look into it within the next few days I'll just do a new release.

released!

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.

2 participants