HashMap.get() causing seqfault in std.mem.eql. Help pls?

Help
1

Hey! I’ve been learning Zig for a few months, and I’ve run into kind of a weird bug. I’m currently working on a dialogue system, but when I use the get() function (or similar functions like getKey()) for the HashMap, it causes a segfault in the std.mem.eqlBytes function. I’ve verified that it’s getting the right key, and that the key is the right type (I’m using the StringHashMap in case that’s relevant). If anyone can tell me why this is happening, and how I might go about fixing it, I would greatly appreciate it!

Constructive criticism is also appreciated (;

Screenshot 2024-07-19 at 12.35.28 AM
Screenshot 2024-07-19 at 12.35.28 AM2090×1246 554 KB

[EDIT]
I think it has something to do with the string comparison that happens while it’s comparing the given key to the keys in the entries. Take that with an entire shaker of salt though. If I knew what the problem was, I could probably have fixed it on my own XD.

[EDIT2]
. . . If I knew more about the language that is lol

2

Hi

i only took a quick look at the code and you should probably post a minimal reproducible example to get a better answer but is it at all possible that you are freeing the hash map keys?

From StringHashMap docs:

Key memory is managed by the caller. Keys and values will not automatically be freed.

Pedantic note: it is usually prefered to post the code as text in a code block instead of a screenshot. Welcome to ziggit btw :smiley:

4 Likes
3

Thanks for responding so quickly! Yes I am freeing the keys, but it’s in a separate function that frees all the memory used by the HashMap and the actual text itself (I’m using Zig bindings for raylib, so even though this is written to be engine agnostic, to display text the string needs to be a [:0] sentinel terminated slice. So I’m using fmt.allocPrintZ to convert it to a sentinel terminated slice, and since I need to store the text until I need it, I can’t free the memory at the end of the scope). I’ll try a few things to test if the keys are getting freed before too early, Thank you!

[EDIT]
I’ll also try to reproduce it in the most minimal way I can find, and post the results in this topic

[EDIT2]
If your solution doesn’t solve the problem, I’ll try to do some deeper diving into the standard library / docs and get back to you

4

Ok thank you so much! Because I never would have figured that out XD! Basically, my get function uses slices of a larger string as keys and values it basically parses them from a regular old text file full of custom formatted text. While it’s reading through the file, it sets the keys and values to be substrings(slices) of the current line. Only problem is. . . The original string is freed at the end of every loop. . . So yeah, %100 on me lol!

[EDIT]
I thought that the actual contents of the slice would’ve been stored in the HashMap, which is why I didn’t even think about consequences of freeing the main string :sweat_smile:

5

Question. Would the mem.sliceTo() function help me out here?

6

No, it would help you with finding the end of a zero terminated string and giving you a slice that marks the start + length of that corresponding memory, however it doesn’t manage the underlying memory of the actual string for you.

If the file contains only strings and you need all of them, you could read in the entire file and then have the slices refer to that memory block.

If the file contains a lot of other stuff you don’t want to keep loaded, you could append the string from the temporary current line, to some data structure with pointer stability (so not std.ArrayList, because that regularly reallocates the backing array, changing pointer addresses).

While SegmentedList would give you pointer stability, it doesn’t help you with adding a slice that remains contigous across segments, so personally I would either write a custom string memory pool, or use an arena for allocating the backing array for the strings.

2 Likes
7

It looks like you’re using a StringHashMap([]const u8), so if you want you could use std.BufMap which is a wrapper around StringHashMap([]const u8) that will copy the keys/values before inserting them and will also handle freeing the copies for you.

7 Likes
8

That’s all really awesome stuff to know actually, thank you! I got it working, I just wrote a small function that creates a new substring. You are in charge of freeing the memory for it though. The file is going to contain all the dialogue for the game, which is why I have it search for a specific keyword before it starts processing the block of text. This is the test file to give as an example of the formatting

Formatting Guide:
[Tag]Name(Portrait Number):text = dialogue

[Tag]@:text = unnamed or descriptive dialogue

{Tag}Label:Dialogue Tag = Dialogue choice

[Tag]~Begin~ marks the beginning of the dialogue

[Tag]~End~ marks the end of the current dialogue

File:
[Will Never Be Read]~Begin~
[Will Never Be Read0]@:Will never be read.
[Will Never Be Read1]@:How sad.
[Will Never Be Read2]@:I put so much effort into this bit.
{Will Never Be Read2}Indeed:Agree0
{Will Never Be Read2}Say Nothing:Silence0
{Will Never Be Read2}Start crying:Sadness0
{Will Never Be Read2}Get angry:Anger0
[Agree0]@:Yes. . .
[Agree1]@:. . .
[Agree3]@:. . .
[Silence0]@:. . .
[Silence1]@:. . .
[Silence2]@:. . .
[Sadness0]@:Hey come on now, it’s not that sad.
[Anger0]@:Hey! Who exactly do you think you’re yelling at huh? No one! I’m an unnamed line of text put here by someone else!
[Anger1]@:Just get out of here, you’re clearly way too fired up about this.
[Will Never Be Read]~End~

[YourMom]~Begin~
[YourMom0]Billy(2):Lol! Yo mama!!!
{YourMom0}Nuh uhh! Your mom!:Denial0
{YourMom0}Start crying:Laughing at you0
[Denial0]Billy(5):No way man!
[Denial1]@:*Billy would never recover.
[Laughing at you0]Billy(3):Hahhahahhahahha!
[Laughing at you1]@:*You would never recover.
[YourMom]~End~

It just takes everything in that block, and parses it into keys and values. There are actually two HashMaps(I could probably get away with using one TBH) one of them is for dialogue, and the other is for making choices, the output of the choice_map changes the current context, and then you just call the process function to get the dialogue the portrait and the characters name! Then you can just display it however you see fit! I’m still writing it so there are going to be functions like next, that increase the context_count, then checks the Map for an entry with a key matching the current context and count. If none exist, it checks the choice map, if it finds any keys containing the current context and count, it calls the choice function, which takes in the value of a choice_map key, and moves you onto the next branch of the dialogue based on your decision! If there aren’t any matches left in either map, the dialogue ends! Sorry it’s kind of a lengthy response, but I thought you might be interested!

1 Like
9

That’s also good to know! Thanks!

10

One pattern I have found usefule with strings in maps (keys or values) is to use Allocator.dupe to copy the bytes and then before map deinit, I iteratre over the entries and free.

// Set up map cleanup
defer {
    var iter = map.iterator();
    while (iter.next()) |entry| {
        allocator.free(entry.key_ptr.*);
    }
    map.deinit();
}

// When inserting
const key_copy = try allocator.dupe(u8, original_str);
try map.put(key_copy, value);
2 Likes
11

Nice! I’m doing kind of the same thing, just a little bit differently. I have a separate clear function that clears all the memory required by dialogue at the end of a dialogue!

    pub fn clear(self: *dialogue) void {
        if (self.text.len > 0) allocator.free(self.text);
        self.text = undefined;
        self.name = undefined;
        self.context = undefined;
        self.context_count = 0;

        var iter1 = dialogue_map.keyIterator();
        var iter2 = choice_map.keyIterator();

        while (iter1.next()) |key| _ = dialogue_map.remove(key.*);
        while (iter2.next()) |key| _ = choice_map.remove(key.*);

        dialogue_map.clearAndFree();
        choice_map.clearAndFree();
    }

I’m considering initializing the maps as needed and deinitializing them when the dialogue is finished instead of just keeping them initialized through all of runtime. I might setup ways to choose whether you want them to stay initialized or not.

[EDIT]
For my specific needs, I needed to create substrings as keys, and I also felt like creating substrings was something I would like to be able to do easily on the regular, so I just wrote a small function for creating substrings:

pub fn substring(string: []const u8, start: usize, end: usize) ![]const u8 {
    const subtring = try allocator.alloc(u8, end - start);
    mem.copyForwards(u8, subtring, string[start..end]);
    return subtring;
}

I’m pretty sure dupe would do literally the same thing if I passed in a slice though, wouldn’t it? :sweat_smile:

[Edit2]
Yep! Can confirm, literally the exact same thing, thank you for suggesting that to me! That’s so much easier!

1 Like
12

Hey, you’re thinking in sync with the std lib authors. That’s a good thing! :^)

2 Likes