community improvements#11
Open
rofl0r wants to merge 43 commits into
Open
Conversation
boehm GC is a highly problematic package, since what it tries to achieve is not possible in standard C, so it must use all kinds of nasty platform-specific tricks to get start offset of stack and so on. for this reason, some or most distributions apply custom settings or even patches to make it work. these tweaks are not available when embedding the source of boehm into the tree and hoping that it will turn out to work well... windows users are advised to install the version provided by mingw package manager, while mac os users can use homebrew's version. closes #6 closes #9 closes #4
the original code used to have a tiny patch on top of peg 0.1.4 overriding all calls to malloc/realloc via a YYMALLOC/YYREALLOC macro, so that all parts use the same GC (probably boehm). re-applying this change was overlooked when we updated to latest peg. meanwhile peg supplies its own YY_MALLOC which we now use instead. this caused odd issues such as "TypeError" message when running the strings.rb test on linux distributions based on musl, which is generally more sensible to undefined behaviour.
the original commit[0] did not document what was wrong. [0]: sanchapereira/tinyrb-ist@3098dfe
commit 35207051557c79ea25942c021fb18856c72af8e3 https://github.com/orangeduck/tgc (removed a couple big files from examples and .git*)
make GC=BOEHM (default) builds tinyrb with boehm GC support make GC=TGC builds tinyrb with built-in TGC make GC=NONE builds tinyrb against libc's malloc impl (without GC)
the STRING_PUSH macro does not zero-terminate strings, so we must use the pushed strings with TrString_new which takes a length argument and takes care of terminating the produced string, unlike TrString_new2 which uses strlen() on the input string, requiring it to be zero- terminated. this happened to work in the past by accident, because boehm gc hands out only zeroed memory.
some members were not properly initialized and relied on the property of boehm gc that all memory is zeroed by default.
uninitialized memory in the classes[] array caused crashes. almost certainly other members aren't properly initialized either.
the alloca call causes issues with clang's address sanitizer: ==951==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffd7 2535e0b at pc 0x0000004615f1 bp 0x7ffd72535dd0 sp 0x7ffd72535580 besides, it was also inefficient since the string has to be copied into a heap allocated one anyway, and dangerous, since ruby strings can easily be several MBs, especially when one slurps entire files into memory, which would then smash the stack.
TrCallSite obj's are allocated implicitly via kv_pushp which in turn calls realloc. the error slipped through the author's radar because boehm's malloc and realloc both hand out only zero-initialized mem. found by valgrind.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
see list of commits