Follow the pointer for member access on pointer-to-struct/union vars - #199
Follow the pointer for member access on pointer-to-struct/union vars#199ChrisJr404 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Issues outside the diff
These findings reference lines that are not present in the diff and could not be posted as inline comments:
-
[suggestion] ast.c:4607 (
enter_function_scope) -- Function-parametergang Point *ppstill does not work. Semantic analysis accepts it (add_symbolgetsparam->struct_name), but thepointer_level > 0bind path never copiesstruct_nameonto the boundVariable.resolve_struct_access()then fails withUnknown struct or union type. Probe:skibidi bump(gang Point *pp) { yapping("param %d %d", pp.x, pp.y); pp.x = 99; }
Actual: errors, prints
param 0 0, original unchanged. Pre-existing (that path never allocated a blob either), so not a regression — but it is the same user-facing feature, and the fix is a few lines next to the existing by-valuebound->struct_name = safe_strdup(...). Worth doing here or as a tight follow-up, with a fixture. -
[suggestion] docs/the-brainrot-programming-language.md -- Optional one-liner that
pp.xon a pointer variable now follows the pointer, while chained access through a pointer-typed field (a.ptr.b) is still unsupported. Not required for a bugfix.
| pointer was invalidated by hashmap resize during semantic | ||
| analysis. */ | ||
| if (!var->value.array_data) | ||
| if (var->pointer_level > 0) |
There was a problem hiding this comment.
pointer_level > 0treatspvalueas the struct blob. That is correct forT*. For T** (gang Point **ppp = &pp; ppp.x) this reads a slice of the inner pointer and prints garbage (I saw 112/79191968` depending on ASLR) — same "wrong answer, no error" class as #196.
Suggestion: require pointer_level == 1 here (and reject other levels at semantic time) instead of assuming every pointer-typed struct/union var is a T*.
| if (node->struct_init_expr) | ||
| sv->value.pvalue = | ||
| evaluate_expression_pointer(node->struct_init_expr); | ||
| return; |
There was a problem hiding this comment.
This early return skips pending_initializer. gang Point *pp = {5, 9} is accepted by the brace-init production in lang.y, then this path leaves pp null and pp.x becomes Member access through a null struct/union pointer.
Suggestion: reject brace-init of a pointer-typed struct/union at semantic/parse time rather than silently dropping the initializer.
| p.y = 9; | ||
|
|
||
| 🚽 read through the pointer | ||
| gang Point *pp = &p; |
There was a problem hiding this comment.
Happy-path local init + write-through is covered (struct and union). The PR also claims a null-pointer error — that path works, but is untested. Also untested: assignment after declaration (gang Point *pp; pp = &p;), reassignment, nested members through a pointer (lp.start.x), and function-parameter pointers.
Suggestion: a fixture that only assigns through a null pointer (no yapping) would match the existing error-test style; yapping after the error still prints 0 and exits 0, which fights the harness.
|
Good catch — you're right, the Fixed it by carrying the tag across with |
0c529b8 to
83925ef
Compare
|
Pushed the pointer-param fix (the bind path now copies struct_name via safe_strdup so resolve_struct_access finds the type) and rebased onto main to clear the conflict, so it's mergeable again. Full suite is green at 137 passing under the ASan/UBSan build, and the new struct_pointer_param fixture covers read-through and write-through for both a struct and a union pointer param. |
|
Please rebase. I pushed some fixes to our CI. |
A `gang Point *pp = &p;` declaration went through the same path as a by-value `gang Point p;`, callocing a fresh zeroed blob and ignoring pointer_level, so `pp.x` read/wrote that blob instead of dereferencing the stored address. Now a pointer-typed struct/union var skips the blob and keeps the address in value.pvalue, resolve_struct_access dereferences it for the base, `&s` on a struct yields its blob address, and hm_free only frees the blob for by-value structs so the borrowed pointer isn't invalid-freed. Fixes Brainrotlang#196
The pointer_level > 0 bind path in enter_function_scope copied the pointer value but never the struct_name, so a `gang Point *pp` parameter had no tag for resolve_struct_access to look up, failing with "Unknown struct or union type" and reading a zeroed blob (param 0 0). Carry the tag across with safe_strdup when the parameter is a struct/union, mirroring the existing by-value path a few lines below. Adds a struct_pointer_param fixture covering read-through and write-through for both a struct and a union pointer parameter.
83925ef to
151db44
Compare
|
Rebased onto main to pick up your CI fixes, thanks. |
Description
gang Point *pp = &p;parses fine, butpp.xwas reading a separate always-zeroed blob instead of following the pointer. The struct-declaration path callocs atotal_sizeblob unconditionally, ignoringpointer_level, so a pointer-typed struct var got its own storage just like a by-value one andresolve_struct_access()treated that storage as the struct — thepp = &paddress was never dereferenced.The fix, matching what leo-aa88 laid out in the issue:
pointer_level > 0) struct/union declarations skip the blob and store the initializer's address invalue.pvalue(same union slot asarray_data), like any other pointer variable.resolve_struct_access()'s identifier branch dereferencesvalue.pvaluefor the base address whenpointer_level > 0instead of usingvalue.array_data, with a null-pointer guard. By-value structs are unchanged.evaluate_lvalue_address()gained aVAR_STRUCTcase returning the blob base, so&sactually yields the struct's address to feed the pointer (it errored before).hm_free()only frees the blob for by-value structs — a pointer-typed struct var holds a borrowed address, so freeing it would be an invalid free of storage the pointed-at variable owns.Reads and writes both go through the pointer now, and writes are visible on the original. Verified the same for
chungusunions. A null pointer access reports an error rather than crashing.Related Issue
Fixes #196
Type of Change
Checklist
make format-checklocally (ormake formatto fix)New fixture
test_cases/struct_pointer_member_access.brainrotreads + writes members through both a struct and a union pointer and asserts the writes land on the originals.make testis 111/111 green under the-fsanitize=address,undefinedbuild (no ASan/UBSan/LeakSanitizer complaints). valgrind isn't installed on my box so that box is unchecked, but the ASan build (LeakSanitizer included) is clean on the new case.