fix(ci): separate non-sanitized binary for valgrind and fix parse-error leaks by timsurrealedu · Pull Request #202 · Brainrotlang/brainrot

timsurrealedu · 2026-08-18T08:46:48Z

Fixes #186.

Summary of Changes

Separate Valgrind Target (brainrot-valgrind):
- Added VALGRIND_TARGET := brainrot-valgrind and VALGRIND_CFLAGS (without -fsanitize=address,undefined) to Makefile.
- Updated make valgrind to build and run against brainrot-valgrind, eliminating the ASan + Valgrind startup crash.
- Added brainrot-valgrind to make clean.
Parameterize run_valgrind_tests.sh:
- Updated run_valgrind_tests.sh to accept a target binary argument (TARGET="${1:-./brainrot}").
Fix Parse-Error Memory Leaks (lang.y):
- Added Bison %destructor rules for <strval> and <declarator> to free allocated strings when Bison discards symbols during parse error handling.
- Resolves memory leaks triggered during syntax errors (such as in cooked_syntax_error.brainrot and cooked_bad_include.brainrot).

leo-aa88 · 2026-08-18T18:48:26Z

Thanks for your contribution. I'll provide a review.

leo-aa88 · 2026-08-18T18:56:06Z

@@ -1,5 +1,7 @@
 #!/bin/bash

+TARGET="${1:-./brainrot}"


must-fix: Default is still the ASan ./brainrot. CI (.github/workflows/ci.yml test job) and CONTRIBUTING.md invoke this with no args, so #186 stays broken there — ./run_valgrind_tests.sh still SIGSEGVs on startup and exits 0.

Default to ./brainrot-valgrind, and have CI build/upload that artifact (or run make valgrind).

leo-aa88 · 2026-08-18T18:56:08Z


 # Output files
 TARGET := brainrot
+VALGRIND_TARGET := brainrot-valgrind


should-fix: .gitignore only lists brainrot, so this binary is untracked. Add brainrot-valgrind next to it.

timsurrealedu · 2026-08-19T14:10:59Z

Thanks for the review @leo-aa88!

I have addressed both comments:

Default Valgrind target & CI update: Updated run_valgrind_tests.sh to default to ./brainrot-valgrind. Updated .github/workflows/ci.yml so that build compiles and uploads brainrot-valgrind as an artifact, and test grants executable permissions to it so the default ./run_valgrind_tests.sh run executes against the non-sanitized binary in CI. Also updated CONTRIBUTING.md to reference make valgrind.
.gitignore: Added brainrot-valgrind to .gitignore.

leo-aa88 · 2026-08-19T19:41:59Z

There are some linter errors in the CI. Can you please address then?

Run make format-check
clang-format --dry-run -Werror ./semantic_analyzer.c ./ast.h ./stdrot.c ./interpreter.h ./interpreter.c ./stdrot.h ./visitor.c ./ast.c ./semantic_analyzer.h ./stdrot/bet.c ./stdrot/stdrot_api.h ./stdrot/baka.c ./stdrot/ragequit.c ./stdrot/slorp.c ./stdrot/yapping.c ./stdrot/registry.c ./visitor.h ./lib/arena.h ./lib/input.c ./lib/arena.c ./lib/hm.c ./lib/mem.c ./lib/string_value.h ./lib/input.h ./lib/hm.h ./lib/mem.h
./stdrot.c:636:16: error: code should be clang-formatted [-Wclang-format-violations]
        (String (*)(String, size_t))stdrot_lookup_symbol(s);
               ^
./lib/mem.h:15:33: error: code should be clang-formatted [-Wclang-format-violations]
#define MAX_ALLOC_SIZE ((size_t)-1 >> 1)
                                ^
./lib/mem.h:15:34: error: code should be clang-formatted [-Wclang-format-violations]
#define MAX_ALLOC_SIZE ((size_t)-1 >> 1)
                                 ^
make: *** [Makefile:193: format-check] Error 1
Error: Process completed with exit code 2.

leo-aa88 · 2026-08-19T22:24:33Z

Please rebase. I pushed some fixes to our CI.

…structor Fix ASan + Valgrind incompatibility by introducing a non-sanitized build target 'brainrot-valgrind' for 'make valgrind'. Update 'run_valgrind_tests.sh' to accept the binary target path as an argument. Also add Bison %destructor directives in lang.y for <strval> and <declarator> to free allocated strings when Bison discards symbols during parse error recovery (e.g. syntax errors in included files).

…s.sh

timsurrealedu · 2026-08-20T05:47:57Z

Done

Linter Violations: Fixed clang-format issues in lib/mem.h and stdrot.c (make format-check is clean).
Valgrind Test Suite: Added stdin inputs for the newly added native_call_* test fixtures in run_valgrind_tests.sh so make valgrind runs smoothly across all 135+ fixtures without blocking on stdin.

Thanks @leo-aa88!

leo-aa88

The ASan/Valgrind split is the right fix for #186, and it actually runs. CI invokes ./brainrot-valgrind, cooked_syntax_error / cooked_bad_include now free every heap block, make valgrind is green, pytest is 135/135. The %destructor for <strval> / <declarator> is the correct tool for lexer safe_malloc strings that never get reduced.

That is not the same as finishing the contract this PR claims.

make valgrind used to report success while doing nothing. After this patch, ./run_valgrind_tests.sh — the path CONTRIBUTING now documents as equivalent — can still report success while doing nothing. And in the same lang.y this PR edits, a comment still asserts that the grammar has no destructors.

See inline comments. Fix the script to fail closed. Rewrite the struct_field comment. Drop graphify-out/. Then this is mergeable.

leo-aa88 · 2026-08-20T06:16:24Z

@@ -1,23 +1,29 @@
 #!/bin/bash

+TARGET="${1:-./brainrot-valgrind}"


MAJOR. Default is a binary make does not build. Only make valgrind / make brainrot-valgrind produces it.

CONTRIBUTING now documents ./run_valgrind_tests.sh with no args as equivalent to make valgrind. After a normal make, that path hits a missing file. Combined with the -eq 100 check below, the script still exits 0.

make valgrind passes the path explicitly, so that target is fine. CI is green only because the build job happens to upload the artifact first. That is job ordering, not a script invariant.

Fail closed here: [[ -x "$TARGET" ]] and command -v valgrind, or this default is a silent-success footgun.

leo-aa88 · 2026-08-20T06:16:24Z

+        valgrind --leak-check=full --error-exitcode=100 "$TARGET" "$f"
    fi

    valgrind_exit_code=$?  # Capture only valgrind’s exit code


MAJOR. This is the same success policy that made #186 silent.

The next lines only fail on valgrind exit code 100. Missing binary is 127. Valgrind not installed is 127. The ASan SIGSEGV that #186 caught was also not 100. That check is unchanged.

Verified: missing binary → valgrind exit 127 → loop continues → script exit 0.

--error-exitcode=100 only fires when valgrind itself classifies a client error. "Could not run the client" is not that. After 135 files of No such file or directory, this still reports a clean leak run.

Treat 127, and any failure to start the target, as a hard error. Do not make "not 100" mean "no leaks."

#186 was not "wrong binary." It was "the job can succeed without testing leaks." The sanitizer split removed one trigger. This check is the mechanism.

leo-aa88 · 2026-08-20T06:16:24Z

 ```

-Run:
+Alternatively, make `run_valgrind_tests.sh` executable and run it directly (defaults to `./brainrot-valgrind`):


This documents the broken path as a supported workflow. After make, ./brainrot-valgrind does not exist, and run_valgrind_tests.sh will still "pass" because it only treats exit code 100 as failure.

Either delete this alternative or make the script refuse to run without an executable target. Do not tell people to invoke a checker that fail-opens.

timsurrealedu · 2026-08-20T07:23:42Z

Thanks for the detailed review, @leo-aa88! I’ve addressed the remaining comments:

run_valgrind_tests.sh now checks that Valgrind is installed and the target is executable before running.
The script now fails on Valgrind errors and abnormal target exits instead of treating every non-100 exit code as success.
VALGRIND_CFLAGS is derived from CFLAGS, keeping flags like -fno-omit-frame-pointer while removing the sanitizers.
Updated the stale struct_field comment in lang.y to reflect the current %destructor behavior.
Removed the unrelated graphify-out/ entry from .gitignore.

I also ran the full test suite and Valgrind checks locally. All 135 tests passed, and make valgrind completed successfully.

The fixes are in commit a1a313f. Thanks again for taking the time to review this!

timsurrealedu requested review from Mazinger677 and leo-aa88 as code owners August 18, 2026 08:46

leo-aa88 reviewed Aug 18, 2026

View reviewed changes

leo-aa88 mentioned this pull request Aug 18, 2026

run_valgrind_tests.sh treats only exit 100 as failure, so crashes look green #203

Open

leo-aa88 requested changes Aug 18, 2026

View reviewed changes

timsurrealedu added 3 commits August 20, 2026 12:30

fix(ci): default valgrind target, upload artifact, and update gitignore

b98e632

fix(ci): add stdin for native_call test fixtures in run_valgrind_test…

a5cddc9

…s.sh

timsurrealedu force-pushed the fix-valgrind-asan-incompatibility branch from fe7013d to a5cddc9 Compare August 20, 2026 05:46

fix(ci): restore upstream formatting in stdrot.c and lib/mem.h

3f22561

leo-aa88 requested changes Aug 20, 2026

View reviewed changes

fix(ci): make valgrind checks fail closed

a1a313f

		@@ -1,23 +1,29 @@
		#!/bin/bash

		TARGET="${1:-./brainrot-valgrind}"

Uh oh!

Conversation

timsurrealedu commented Aug 18, 2026

Summary of Changes

Uh oh!

leo-aa88 commented Aug 18, 2026

Uh oh!

leo-aa88 Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

leo-aa88 Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

timsurrealedu commented Aug 19, 2026

Uh oh!

leo-aa88 commented Aug 19, 2026

Uh oh!

leo-aa88 commented Aug 19, 2026

Uh oh!

timsurrealedu commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leo-aa88 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leo-aa88 Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

leo-aa88 Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

leo-aa88 Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timsurrealedu commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

timsurrealedu commented Aug 20, 2026 •

edited

Loading

leo-aa88 left a comment •

edited

Loading