Replace func trie with hashmap #179

ChAoSUnItY · 2025-01-18T18:01:41Z

Previously, trie implementation is not consistent, mainly because of using index to point the referencing func_t to FUNCS, additionally, it lacks of dynamic allocation which might cause segmentation fault and results more technical debt to debug on either FUNCS or FUNCS_TRIE. Thus, in this PR, we can resolve this issue by introducing a dynamic hashmap.

Current implementation is using FNV-1a hashing algorithm (32-bit edition to be precise), and due to lack of unsigned integer implementation, hashing result ranges from 0 to 2,147,483,647.

Notice that current implementation may suffer from lookup issue when the function amount keeps increasing since current hashmap implementation doesn't offer rehashing based on load factor (which ideally, 0.75 would be best and currently shecc does not support floating number).

This also enables us to refactor more structures later with hashmap implementation in shecc.

Benchmark for ./tests/hello.c compilation

Before

Command being timed: "./out/shecc tests/hello.c"
        User time (seconds): 0.00
        System time (seconds): 0.02
        Percent of CPU this job got: 76%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.03
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 52112
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 12220
        Voluntary context switches: 0
        Involuntary context switches: 0
        Swaps: 0
        File system inputs: 0
        File system outputs: 32
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

After

Command being timed: "./out/shecc tests/hello.c"
        User time (seconds): 0.00
        System time (seconds): 0.02
        Percent of CPU this job got: 71%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.03
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 49916
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 12224
        Voluntary context switches: 1
        Involuntary context switches: 0
        Swaps: 0
        File system inputs: 8
        File system outputs: 32
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

Summary by Bito

This PR implements a new hashmap-based function lookup system replacing the existing trie implementation. The changes introduce a FNV-1a hashing algorithm for efficient key distribution and include core hashmap operations (create, put, get, free). The implementation improves code maintainability and addresses previous segmentation fault issues, though lacks rehashing functionality.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

jserv · 2025-01-19T07:20:56Z

@visitorckw, can you comment this?

src/globals.c

visitorckw · 2025-01-19T07:51:35Z

Looks good as is.

However, as mentioned, a large number of functions may cause excessive collisions and slow down performance. For smaller function counts, the default 512 buckets might be overkill. Therefore, a radix tree with dynamic memory allocation could still be a method worth exploring in the future.

ChAoSUnItY · 2025-01-19T08:05:21Z

I'm concerning that dynamic memory allocation at this moment is not reliable and potentially flawed, I've attempted to implement rehashing algorithm before, but on stage 2 the compilation will fail, while the GCC and stage 1 are fine.

src/globals.c

visitorckw · 2025-01-19T14:48:13Z

src/globals.c

+
+    for (; *key; key++) {
+        hash ^= *key;
+        hash *= 0x01000193;


The multiplication here may cause overflow, leading to undefined behavior. Signed integer overflow is undefined, while unsigned integer overflow is not. Since shecc currently lacks support for unsigned integers, we might consider adding it to address this issue.

I think we can just simply add unsigned type at this moment to simplify the effort of new type? This way, unsigned can still uses current signed's arithmetic algorithm (due to the fact that they both based on 2's complement), since in ARMv7 and RISC-V 32bit assembly, signed overflow is well-defined, the only difference would be intrepretation of most-significant bit.

Edit: I just realized we still need to handle comparison, but I prefer to defer unsigned integer feature since we have an ongoing project which requires full resolution of shecc's specification, and which doesn't include unsigned types at this moment. I think this addition would alters the simplicity of project. @jserv should we postpone this hashmap implementation?

should we postpone this hashmap implementation?

You can simply convert this pull request to draft state.

I think this pull request should be implemented as soon as possible, the reason is that I'm currently working on type_t refactor, but I have encountered the weird free(): invalid pointer issue when adding functions in globals.c, which I assume the reason is that the function trie cannot hold more than certain numbers of functions and probably function name's length could also contribute to this issue, these 2 factors and the flaw is already described here:

shecc/src/globals.c

Lines 103 to 109 in 09bb918

if (!trie->next[fc]) {

/* FIXME: The func_tries_idx variable may exceed the maximum number,

* which can lead to a segmentation fault. This issue is affected by

* the number of functions and the length of their names. The proper

* way to handle this is to dynamically allocate a new element.

*/

trie->next[fc] = func_tries_idx++;

But after cherry-picked this branch without any changes to function structures, the issue immediately gone.

One possible solution towards this is to add -fwrapv compilation flag to gcc to instruct compiler to wrap signed integer overflow result according to the 2's compliment representation, this ensures defined behavior at least when compiling with gcc. Meanwhile in shecc, it's fine at this moment since both ARM 32bit and RISC-V 32bit assembly wraps the overflow value according to the 2's compliment representation as well.

Could enlarging limitation in refs.h temporarily fix this problem? I remember the trie count is almost reaching the limitation in the last change.

This workaround could be remove after applying this patch.

Changing macro MAX_FUNC_TRIES in defs.h to 3000 does the trick.

I think this pull request should be implemented as soon as possible, the reason is that I'm currently working on type_t refactor, but I have encountered the weird free(): invalid pointer issue when adding functions in globals.c, which I assume the reason is that the function trie cannot hold more than certain numbers of functions and probably function name's length could also contribute to this issue, these 2 factors and the flaw is already described here:

shecc/src/globals.c

Lines 103 to 109 in 09bb918

if (!trie->next[fc]) {

/* FIXME: The func_tries_idx variable may exceed the maximum number,

* which can lead to a segmentation fault. This issue is affected by

* the number of functions and the length of their names. The proper

* way to handle this is to dynamically allocate a new element.

*/

trie->next[fc] = func_tries_idx++;

I don't necessarily oppose this PR. However, if the issue is that MAX_FUNC_TRIES is too small, causing an out-of-bounds array access, it seems unrelated to switching to a hash table instead of a trie. A hash table can also use an array, and a trie can use dynamic memory allocation. This feels more like adding a new feature unrelated to fixing the bug itself. But I'm fine if we decide to switch to a hash table as a workaround.

The major reason that I would like to replace trie is that some errors are not straightforward to be realized, in this case, it generates free(): invalid pointer, which isn't that obvious to do with insufficient trie size and is not friendly to new-coming contributors in my opinion.

src/globals.c

Previously, trie implementation is not consistent, mainly because of using index to point the referencing func_t to FUNCS, additionally, trie's advantage is that enables prefix lookup, but in shecc, it hasn't been used in this way, furthur more, it takes 512 bytes per trie node, while in this implementation, it 24 + W (W stands for key length including NULL character) bytes per hashmap bucket node, which significantly reduces memory usage. This also allows for future refactoring of additional structures using a hashmap implementation. Notice that currently FNV-1a hashing function uses signed integer to hash keys, which would lead to undefined behavior, instead of adding unsigned integer to resolve this, we add "-fwrapv" compiler flag to instruct gcc to wrap overflow result according to 2's complement representation. Meanwhile in shecc, it's guaranteed to be always wrap around according to 2's complement representation.

bito-code-review · 2025-02-07T17:26:27Z

Code Review Agent Run #86353d

Actionable Suggestions - 9

src/globals.c - 9
- Missing malloc NULL checks · Line 117-119
- Possible buffer overflow in hashmap creation · Line 119-119
- Consider adding malloc null check · Line 407-408
- Consider initializing allocated func_t structure · Line 407-407
- Consider adding key parameter validation · Line 135-136
- Consider adding memory allocation checks · Line 137-138
- Consider explicit NULL check in hashmap_contains · Line 200-200
- Consider validating hashmap_create return value · Line 682-682
- Consider NULL check before hashmap_free · Line 712-712

Additional Suggestions - 2

src/globals.c - 1
- Consider adding null check for func_name · Line 446-446
src/defs.h - 1
- Consider adding capacity field to hashmap · Line 316-318

Review Details

Files reviewed - 3 · Commit Range: b14a10d..b14a10d
- Makefile
- src/defs.h
- src/globals.c
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

bito-code-review · 2025-02-07T17:31:59Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Replace Trie with Hashmap for Function Lookup	- `Makefile` - Added -fwrapv compiler flag for wrap-around behavior - `defs.h` - Replaced trie structure with hashmap definitions and data structures - `globals.c` - Implemented hashmap functions and replaced trie-based function lookup with hashmap operations

bito-code-review · 2025-02-07T17:32:00Z

src/globals.c

+    hashmap_t *map = malloc(sizeof(hashmap_t));
+    map->size = round_up_pow2(size);
+    map->buckets = malloc(size * sizeof(hashmap_node_t *));


Missing malloc NULL checks

Consider adding NULL checks after malloc() calls to handle out of memory conditions gracefully.

Code suggestion

Check the AI-generated fix before applying

Suggested change

hashmap_t *map = malloc(sizeof(hashmap_t));

map->size = round_up_pow2(size);

map->buckets = malloc(size * sizeof(hashmap_node_t *));

hashmap_t *map = malloc(sizeof(hashmap_t));

if (!map)

return NULL;

map->size = round_up_pow2(size);

map->buckets = malloc(size * sizeof(hashmap_node_t *));

if (!map->buckets) {

free(map);

return NULL;

}

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:02Z

src/globals.c

+{
+    hashmap_t *map = malloc(sizeof(hashmap_t));
+    map->size = round_up_pow2(size);
+    map->buckets = malloc(size * sizeof(hashmap_node_t *));


Possible buffer overflow in hashmap creation

Consider using map->size instead of size when allocating memory for buckets to ensure consistent sizing. The current code allocates memory based on the original size parameter rather than the rounded up power of 2 value stored in map->size.

Code suggestion

Check the AI-generated fix before applying

Suggested change

map->buckets = malloc(size * sizeof(hashmap_node_t *));

map->buckets = malloc(map->size * sizeof(hashmap_node_t *));

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:03Z

src/globals.c

+        fn = malloc(sizeof(func_t));
+        hashmap_put(FUNCS_MAP, name, fn);


Consider adding malloc null check

Consider adding memory allocation failure check after malloc(). The malloc() call could return NULL if memory allocation fails, which should be handled gracefully.

Code suggestion

Check the AI-generated fix before applying

Suggested change

fn = malloc(sizeof(func_t));

hashmap_put(FUNCS_MAP, name, fn);

fn = malloc(sizeof(func_t));

if (!fn) {

return NULL;

}

hashmap_put(FUNCS_MAP, name, fn);

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:04Z

src/globals.c

+    if (hashmap_contains(FUNCS_MAP, name)) {
+        fn = hashmap_get(FUNCS_MAP, name);
+    } else {
+        fn = malloc(sizeof(func_t));


Consider initializing allocated func_t structure

Consider initializing the newly allocated func_t structure using memset() or explicit initialization. Currently, the memory allocated for fn is not initialized which could lead to undefined behavior when accessing uninitialized fields.

Code suggestion

Check the AI-generated fix before applying

Suggested change

fn = malloc(sizeof(func_t));

fn = malloc(sizeof(func_t));

memset(fn, 0, sizeof(func_t));

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:05Z

src/globals.c

+{
+    int len = strlen(key);


Consider adding key parameter validation

Consider adding a NULL check for the key parameter to prevent undefined behavior if a NULL key is passed.

Code suggestion

Check the AI-generated fix before applying

Suggested change

{

int len = strlen(key);

{

if (!key)

return NULL;

int len = strlen(key);

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:06Z

src/globals.c

+    hashmap_node_t *node = malloc(sizeof(hashmap_node_t));
+    node->key = calloc(len + 1, sizeof(char));


Consider adding memory allocation checks

Consider adding NULL checks after memory allocation calls. If either malloc() or calloc() fails, dereferencing node->key could lead to a segmentation fault.

Code suggestion

Check the AI-generated fix before applying

Suggested change

hashmap_node_t *node = malloc(sizeof(hashmap_node_t));

node->key = calloc(len + 1, sizeof(char));

hashmap_node_t *node = malloc(sizeof(hashmap_node_t));

if (!node)

return NULL;

node->key = calloc(len + 1, sizeof(char));

if (!node->key) {

free(node);

return NULL;

}

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:06Z

src/globals.c

+ */
+bool hashmap_contains(hashmap_t *map, char *key)
+{
+    return hashmap_get(map, key);


Consider explicit NULL check in hashmap_contains

The hashmap_contains() function directly returns the result of hashmap_get() which returns a void pointer. This could lead to incorrect boolean evaluation since a non-NULL pointer doesn't necessarily mean true. Consider explicitly comparing the return value with NULL.

Code suggestion

Check the AI-generated fix before applying

Suggested change

return hashmap_get(map, key);

return hashmap_get(map, key) != NULL;

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:07Z

src/globals.c

@@ -600,8 +679,7 @@ void global_init()
    BLOCKS.head = NULL;
    BLOCKS.tail = NULL;
    MACROS = malloc(MAX_ALIASES * sizeof(macro_t));
-    FUNCS = malloc(MAX_FUNCS * sizeof(func_t));
-    FUNC_TRIES = malloc(MAX_FUNC_TRIES * sizeof(trie_t));
+    FUNCS_MAP = hashmap_create(MAX_FUNCS);


Consider validating hashmap_create return value

Consider validating the return value of hashmap_create() to handle potential memory allocation failures. The function could return NULL if malloc() fails inside it.

Code suggestion

Check the AI-generated fix before applying

Suggested change

FUNCS_MAP = hashmap_create(MAX_FUNCS);

FUNCS_MAP = hashmap_create(MAX_FUNCS);

if (!FUNCS_MAP) {

fprintf(stderr, "Failed to create functions hashmap\n");

exit(1);

}

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-07T17:32:08Z

src/globals.c

@@ -630,8 +709,7 @@ void global_release()
        BLOCKS.head = next;
    }
    free(MACROS);
-    free(FUNCS);
-    free(FUNC_TRIES);
+    hashmap_free(FUNCS_MAP);


Consider NULL check before hashmap_free

Consider checking if FUNCS_MAP is not NULL before calling hashmap_free(). The current implementation might lead to undefined behavior if FUNCS_MAP is NULL.

Code suggestion

Check the AI-generated fix before applying

Suggested change

hashmap_free(FUNCS_MAP);

if (FUNCS_MAP) {

hashmap_free(FUNCS_MAP);

}

Code Review Run #86353d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

ChAoSUnItY force-pushed the refactor/hashmap branch from 1f5ea61 to 0c48824 Compare January 19, 2025 08:25

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

ChAoSUnItY force-pushed the refactor/hashmap branch from 0c48824 to 400e3ae Compare January 19, 2025 10:04

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

ChAoSUnItY force-pushed the refactor/hashmap branch 4 times, most recently from 1bacbd7 to cb82f7a Compare January 19, 2025 10:27

jserv reviewed Jan 19, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

ChAoSUnItY force-pushed the refactor/hashmap branch from cb82f7a to 4a5192e Compare January 19, 2025 11:12

visitorckw reviewed Jan 19, 2025

View reviewed changes

ChAoSUnItY marked this pull request as draft January 19, 2025 19:55

ChAoSUnItY force-pushed the refactor/hashmap branch from 4a5192e to 08380c3 Compare January 24, 2025 06:19

ChAoSUnItY marked this pull request as ready for review January 24, 2025 06:22

jserv requested a review from visitorckw January 24, 2025 06:42

jserv reviewed Feb 3, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

jserv reviewed Feb 3, 2025

View reviewed changes

src/globals.c Outdated Show resolved Hide resolved

ChAoSUnItY force-pushed the refactor/hashmap branch from 08380c3 to b14a10d Compare February 7, 2025 17:24

bito-code-review bot reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace func trie with hashmap #179

Replace func trie with hashmap #179

ChAoSUnItY commented Jan 18, 2025 •

edited by bito-code-review bot

Loading

jserv commented Jan 19, 2025

visitorckw commented Jan 19, 2025

ChAoSUnItY commented Jan 19, 2025

This comment was marked as resolved.

visitorckw Jan 19, 2025

ChAoSUnItY Jan 19, 2025 •

edited

Loading

jserv Jan 19, 2025

ChAoSUnItY Jan 23, 2025 •

edited

Loading

vacantron Jan 24, 2025 •

edited

Loading

ChAoSUnItY Jan 24, 2025

visitorckw Jan 27, 2025

ChAoSUnItY Jan 31, 2025

bito-code-review bot commented Feb 7, 2025 •

edited

Loading

Code Review Agent Run #86353d

bito-code-review bot commented Feb 7, 2025

Changelist by Bito

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

bito-code-review bot Feb 7, 2025

	if (!trie->next[fc]) {
	/* FIXME: The func_tries_idx variable may exceed the maximum number,
	* which can lead to a segmentation fault. This issue is affected by
	* the number of functions and the length of their names. The proper
	* way to handle this is to dynamically allocate a new element.
	*/
	trie->next[fc] = func_tries_idx++;

	map->buckets = malloc(size * sizeof(hashmap_node_t *));
	map->buckets = malloc(map->size * sizeof(hashmap_node_t *));

		fn = malloc(sizeof(func_t));
		hashmap_put(FUNCS_MAP, name, fn);

	fn = malloc(sizeof(func_t));
	fn = malloc(sizeof(func_t));
	memset(fn, 0, sizeof(func_t));

		hashmap_node_t *node = malloc(sizeof(hashmap_node_t));
		node->key = calloc(len + 1, sizeof(char));

-    hashmap_node_t *node = malloc(sizeof(hashmap_node_t));
-    node->key = calloc(len + 1, sizeof(char));
+    hashmap_node_t *node = malloc(sizeof(hashmap_node_t));
+    if (!node)
+        return NULL;
+    node->key = calloc(len + 1, sizeof(char));
+    if (!node->key) {
+        free(node);
+        return NULL;
+    }

	return hashmap_get(map, key);
	return hashmap_get(map, key) != NULL;

-    FUNCS_MAP = hashmap_create(MAX_FUNCS);
+    FUNCS_MAP = hashmap_create(MAX_FUNCS);
+    if (!FUNCS_MAP) {
+        fprintf(stderr, "Failed to create functions hashmap\n");
+        exit(1);
+    }

Replace func trie with hashmap #179

Are you sure you want to change the base?

Replace func trie with hashmap #179

Conversation

ChAoSUnItY commented Jan 18, 2025 • edited by bito-code-review bot Loading

Before

After

Summary by Bito

jserv commented Jan 19, 2025

visitorckw commented Jan 19, 2025

ChAoSUnItY commented Jan 19, 2025

This comment was marked as resolved.

visitorckw Jan 19, 2025

Choose a reason for hiding this comment

ChAoSUnItY Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

jserv Jan 19, 2025

Choose a reason for hiding this comment

ChAoSUnItY Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

vacantron Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

ChAoSUnItY Jan 24, 2025

Choose a reason for hiding this comment

visitorckw Jan 27, 2025

Choose a reason for hiding this comment

ChAoSUnItY Jan 31, 2025

Choose a reason for hiding this comment

bito-code-review bot commented Feb 7, 2025 • edited Loading

Code Review Agent Run #86353d

bito-code-review bot commented Feb 7, 2025

Changelist by Bito

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

bito-code-review bot Feb 7, 2025

Choose a reason for hiding this comment

ChAoSUnItY commented Jan 18, 2025 •

edited by bito-code-review bot

Loading

ChAoSUnItY Jan 19, 2025 •

edited

Loading

ChAoSUnItY Jan 23, 2025 •

edited

Loading

vacantron Jan 24, 2025 •

edited

Loading

bito-code-review bot commented Feb 7, 2025 •

edited

Loading