Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace func trie with hashmap #179
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
Replace func trie with hashmap #179
Changes from all commits
c50f8c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
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 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?
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.
You can simply convert this pull request to draft state.
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 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 weirdfree(): invalid pointer
issue when adding functions inglobals.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
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 togcc
to instruct compiler to wrap signed integer overflow result according to the 2's compliment representation, this ensures defined behavior at least when compiling withgcc
. 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.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.
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.
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.
Changing macro
MAX_FUNC_TRIES
indefs.h
to 3000 does the trick.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 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.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 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.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
printf
format string is missing the format specifier forlen
. Consider updating to include%d
in the format string.Code suggestion
Code Review Run #66bd40
Is this a valid issue, or was it incorrectly flagged by the Agent?