-
Notifications
You must be signed in to change notification settings - Fork 1
rewriter: add IA2_{CON,DE}STRUCTOR
attribute macros, emit ia2_type_registry_*
calls, and enable type_confusion
tests
#524
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
Conversation
0e6f7bc
to
7369994
Compare
ee114a4
to
d2fbef6
Compare
7369994
to
8791b04
Compare
9009d7d
to
c691d75
Compare
8791b04
to
37710ee
Compare
c691d75
to
1d9bbbb
Compare
37710ee
to
69f8e0a
Compare
1d9bbbb
to
7a56926
Compare
69f8e0a
to
afac829
Compare
7a56926
to
589c938
Compare
afac829
to
57b933d
Compare
9f4155a
to
ba28321
Compare
ba28321
to
a0da8ae
Compare
d207a80
to
4bf1d75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks OK (one comment inline), but tests seem to be failing; fix them before merging.
4bf1d75
to
ac42bf8
Compare
b6f1f13
to
aa6cec2
Compare
I diagnosed and (as far as I can tell locally) fixed the compartment violation segfault here. It stemmed from the default panic handler using TLS to access the panic counter. While I was here, I slimmed down the Rust so it has a minimal impact on the call-gate shims; we don't want to bring in libstd if we can avoid it. |
CI is failing with the tracer because donna has |
d4f7180
to
2067064
Compare
use std::sync::LazyLock; | ||
use std::sync::RwLock; | ||
#![no_std] | ||
/* use the C allocator; don't use libstd */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to avoid std
? It doesn't seem smart to instead write our own Mutex
; there's a lot we can do incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to write our own Mutex
, we should use no_std
lock_api
to write it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to be careful about what we're including in the TCB and libstd does a lot on its own, for example randomization of hash maps and panic infrastructure that allocates and accesses thead locals. We want to start from zero and control everything, because we can't assume to be operating in a normal process context.
runtime/type-registry/src/lib.rs
Outdated
@@ -44,7 +162,9 @@ impl Debug for TypeId { | |||
} | |||
} | |||
|
|||
static GLOBAL_TYPE_REGISTRY: LazyLock<TypeRegistry> = LazyLock::new(TypeRegistry::default); | |||
static GLOBAL_TYPE_REGISTRY: TypeRegistry = TypeRegistry { | |||
map: Mutex::new(BTreeMap::new()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can build a const
-compatible HashMap
now (HashMap::with_hasher(BuildHasherDefault::new())
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The de-LazyLock
PR is merged; the remaining reason to use BTreeMap
instead is that HashMap
requires std
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize that (rust-lang/rust#27242). That's annoying. We could just use hashbrown
, though, which is what std
uses, and it can be used with no_std
+ alloc
.
@@ -15,6 +15,8 @@ crate-type = ["staticlib"] | |||
|
|||
[profile.dev] | |||
panic = "abort" | |||
lto = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need LTO for debug builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't compared sizes in debug builds. It turns out it doesn't help us much there (in fact it somehow increases .so size by ~0.1M?), and I'm not sure why:
debug+lto:
$ du -sh runtime/type-registry/debug/libtype_registry.a
6,2M runtime/type-registry/debug/libtype_registry.a
$ du -sh tests/type_confusion/libtype_confusion_call_gates.so
1,8M tests/type_confusion/libtype_confusion_call_gates.so
debug sans lto:
$ du -sh runtime/type-registry/debug/libtype_registry.a
7,8M runtime/type-registry/debug/libtype_registry.a
$ du -sh tests/type_confusion/libtype_confusion_call_gates.so
1,7M tests/type_confusion/libtype_confusion_call_gates.so
release+lto:
$ du -sh runtime/type-registry/release/libtype_registry.a
3,1M runtime/type-registry/release/libtype_registry.a
$ du -sh tests/type_confusion/libtype_confusion_call_gates.so
68K tests/type_confusion/libtype_confusion_call_gates.so
release sans lto:
$ du -sh runtime/type-registry/release/libtype_registry.a
6,4M runtime/type-registry/release/libtype_registry.a
$ du -sh tests/type_confusion/libtype_confusion_call_gates.so
1,2M tests/type_confusion/libtype_confusion_call_gates.so
The critical number here is that we only take 68kb with release+LTO, which is reasonable to be including in our call gates. If there's a major drawback to using LTO in debug we can disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTO is just not normally used for debug builds at all. It's link time optimization when there's no optimization happening in the first place. I'm fine with enabling it for release builds, though; that makes a lot of sense.
2067064
to
994870c
Compare
… APIs to match the format
…y and add clearer docs
the latter is a no-op in some build configurations
this takes _call_gates.so from MB to double-digit kB
994870c
to
55e0440
Compare
HashMap and RwLock require libstd, but with BTreeMap and a simple Mutex of our own, we can avoid it finally, we define our own panic handler because even in panic=abort mode, the libstd one writes to a thread-local for the panic count and does more work than we want
55e0440
to
7e7aaac
Compare
Note that this PR includes #521 (
git cherry-pick
ed) since the branches are orthogonal (#521 points directly tomain
), so once #521 is merged, those commits can be rebased out of this PR.