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

Fix pin prob #855

Merged
merged 9 commits into from
Mar 1, 2022
Merged

Fix pin prob #855

merged 9 commits into from
Mar 1, 2022

Conversation

adetaylor
Copy link
Collaborator

Fixes #833 by documenting all unsafe code, and forbidding unsafe code in most places.

This reverts commit ab2a8be.
This does not forbid unsafe _generated_ code of course.
Also, we still have unsafe code in the autocxx crate itself.
@adetaylor adetaylor merged commit 336af00 into main Mar 1, 2022
@bsilver8192
Copy link
Contributor

I think it's still broken the same way it was before @adetaylor . Rust is moving storage without calling a C++ copy or move constructor again.

It's interesting that it passes asan, maybe the compiler optimizes the memory allocation out completely? https://github.com/google/autocxx/runs/5391090192?check_suite_focus=true shows it failing even though I'm pretty sure my changes in #843 don't cause it.

On top of current main, this patch:

diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs
index f9c6b91..0fc0560 100644
--- a/integration-tests/tests/integration_test.rs
+++ b/integration-tests/tests/integration_test.rs
@@ -7739,7 +7739,11 @@ fn test_pass_by_value_moveit() {
     let hdr = indoc! {"
     #include <stdint.h>
     #include <string>
+    #include <stdio.h>
     struct A {
+        A() { fprintf(stderr, \"default %p\\n\", this); }
+        ~A() { fprintf(stderr, \"destructor %p\\n\", this); }
+        A(const A&other) : a(other.a) { fprintf(stderr, \"copy %p from %p\\n\", this, &other); }
         void set(uint32_t val) { a = val; }
         uint32_t a;
         std::string so_we_are_non_trivial;

makes it fail for me locally, and print this when it does fail:

default 0x7ffe4bce6998
copy 0x7ffe4bce68f0 from 0x7ffe4bce6998
copy 0x7ffe4bce6870 from 0x7ffe4bce68f0
destructor 0x7ffe4bce6870
destructor 0x7ffe4bce6838
free(): invalid pointer

@adetaylor
Copy link
Collaborator Author

I don't see the crash, but I do see that one of the destructor addresses doesn't match up, so looking now.

@adetaylor
Copy link
Collaborator Author

Pretty sure this is it - what do you think, @bsilver8192? #872

@bsilver8192
Copy link
Contributor

Yep, I think that fixes it.

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.

Document all unsafe and fix pinning booboos
2 participants