-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
std: introduce pointer stability locks to hash maps #17719
Conversation
f935457
to
965e59f
Compare
The main problem being fixed here is there was a getOrPut() that held on to a reference to the value pointer too long, and meanwhile the call to `lowerConst` ended up being recursive and mutating the hash map, invoking undefined behavior. caught via #17719
pub const SafetyLock = struct { | ||
state: State = .unlocked, | ||
|
||
pub const State = if (runtime_safety) enum { unlocked, locked } else enum { unlocked }; |
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 not else void
to not rely on the optimizer? Would this break @setRuntimeSafety?
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.
an enum with only 1 tag is a 0-bit type just like void
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.
Today I learned. 😍
965e59f
to
4a45a83
Compare
I think it would be good to add a pub const SafetyLock = struct {
state: State = .unlocked,
trace: std.debug.ConfigurableTrace(2, 1, runtime_safety) = .{},
pub const State = if (runtime_safety) enum { unlocked, locked } else enum { unlocked };
pub fn lock(l: *SafetyLock) void {
if (!runtime_safety) return;
l.trace.addAddr(@returnAddress(), "locked");
if (l.state == .locked) l.panic("locked while already locked");
l.state = .locked;
}
pub fn unlock(l: *SafetyLock) void {
if (!runtime_safety) return;
l.trace.addAddr(@returnAddress(), "unlock");
if (l.state == .unlocked) l.panic("unlocked while already unlocked");
l.trace = .{};
l.state = .unlocked;
}
pub fn assertUnlocked(l: *SafetyLock, comptime msg: []const u8, ret_addr: usize) void {
if (!runtime_safety) return;
l.trace.addAddr(ret_addr, msg);
if (l.state != .unlocked) l.panic(msg);
}
fn panic(l: SafetyLock, comptime msg: []const u8) noreturn {
var insts = [2]usize{ l.trace.addrs[0][0], l.trace.addrs[1][0] };
const stack_trace = std.builtin.StackTrace{
.index = l.trace.index,
.instruction_addresses = &insts,
};
std.debug.panicImpl(&stack_trace, null, msg);
}
}; The trace can then look like this:
|
4a45a83
to
4ca7257
Compare
This adds std.debug.SafetyLock and uses it in std.HashMapUnmanaged by adding lockMutation() and unlockMutation(). This provides a way to detect when an illegal mutation has happened and panic rather than invoke undefined behavior.
@GethDW that would be handy. I think I could see that being a global std lib option perhaps, to enable sometimes. I would imagine the stack trace collection might be a bit punishing in terms of performance. That said, we could try it and find out for sure. Let's explore that in a follow-up proposal. |
4ca7257
to
3773f64
Compare
lib/std/array_hash_map.zig
Outdated
/// assertion. | ||
/// | ||
/// `unlockMutation` returns the hash map to the previous state. | ||
pub fn lockMutation(self: *Self) void { |
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.
Would lockRealloc()
be a better name for this? Some mutation is allowed, for example the fooAssumeCapacity()
function variants as you mentioned.
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.
That's a nice name. I'll make that change.
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.
On second thought, "realloc" is not the right concept either since it also gets triggered for clearing and shrinking (with retained capacity).
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.
Indeed! I think what you've landed on with lockPointers()
is great :)
FYI @mlugg I imagine this tool will be quite handy when working on incremental compilation stuff. |
This adds std.debug.SafetyLock and uses it in std.HashMapUnmanaged by adding lockPointers() and unlockPointers(). This provides a way to detect when an illegal modification has happened and panic rather than invoke undefined behavior.
This adds
std.debug.SafetyLock
and uses it in standard library hash maps by addinglockPointers()
andunlockPointers()
.This provides a way to detect when an illegal modification has happened and panic rather than invoke undefined behavior.
Example
Something nice about this is that if you use the "assume capacity" variants then such mutations are actually well-defined, and don't trigger the problem:
Merge Checklist
Follow-Up Issues