Skip to content

feat: add BinaryenExpressionAllocateAndWriteText - #8717

Merged
kripken merged 10 commits into
WebAssembly:mainfrom
chharvey:feat/8716-stringify-expression
Jun 5, 2026
Merged

kripken merged 10 commits into
WebAssembly:mainfrom
chharvey:feat/8716-stringify-expression

Conversation

@chharvey

Copy link
Copy Markdown
Contributor

adds a function for returning a string representation of an expression in s-expression format.

closes #8716

@chharvey
chharvey requested a review from a team as a code owner May 16, 2026 23:39
@chharvey
chharvey requested review from stevenfontanella and removed request for a team May 16, 2026 23:39
Comment thread src/binaryen-c.cpp
auto str = os.str();
const size_t len = str.length() + 1;
char* output = (char*)malloc(len);
std::copy_n(str.c_str(), len, output);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy_n is a little strange for copying bytes, can we use strncpy or memcpy instead to convey the intent better?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly not sure, i just copied the exact code from BinaryenModuleAllocateAndWriteText.

I'll defer to the C developers for what's best. I'm happy as long as it's consistent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @stevenfontanella , but yeah, let's keep it consistent here. We can simplify all these copy_n uses separately.

Comment thread src/binaryen-c.h Outdated
// Serialize an expression in s-expression form. Implicitly allocates the returned
// char* with malloc(), and expects the user to free() them manually
// once not needed anymore.
BINARYEN_API char* BinaryenExpressionAllocateAndWriteText(BinaryenExpressionRef expr);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a CHANGELOG.md entry for this and unit tests in test/example/c-api-kitchen-sink.c / test/example/c-api-kitchen-sink.txt.

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with @stevenfontanella 's comment about the changelog and testing

Comment thread src/binaryen-c.cpp
auto str = os.str();
const size_t len = str.length() + 1;
char* output = (char*)malloc(len);
std::copy_n(str.c_str(), len, output);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @stevenfontanella , but yeah, let's keep it consistent here. We can simplify all these copy_n uses separately.

Comment thread src/js/binaryen.js-post.js Outdated
Comment on lines +3311 to +3313
const textPtr = BinaryenObj["_BinaryenExpressionAllocateAndWriteText"](expr);
const text = UTF8ToString(textPtr);
if (textPtr) _free(textPtr);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use try ... finally here to ensure that the pointer is freed even if UTF8ToString throws an exception? I can't find the definition of UTF8ToString so I'm not sure if that's possible, but it conveys the intent better especially in case this code changes.

Also, we typically use braces around if bodies, let's do that here.

@chharvey chharvey May 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, i again copied from the emitText for Modules but i can make the same updates to both.

@chharvey chharvey May 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also… digging into this project helped me learn a lot about Emscripten and where all this stuff like UTF8ToString comes from. see my .d.ts file here for a quasi-explanation. Looks like Emscripten declares it here

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but please add a test for the new API.

@chharvey

Copy link
Copy Markdown
Contributor Author

@kripken Can you point me to directions on how to do that? I ran cmake . && make and then ./check.py but got the error:

AssertionError:
File "./check.py", line 54, in run_version_tests

assert executables
       ^^^^^^^^^^^

Also i wouldn't know how/where to add tests… is there a high-level list of steps? -Thanks

@kripken

kripken commented May 26, 2026

Copy link
Copy Markdown
Member

@chharvey for a JS test, see build-emscripten in .github/workflows/ci.yml. That does some setup and then calls ./scripts/emcc-tests.sh, so you can follow those steps locally.

For where the tests are, see e.g. #8660 which added related tests.

@kripken

kripken commented May 28, 2026

Copy link
Copy Markdown
Member

Looks like there are errors on CI. At least one might be fixed by merging in latest main.

@chharvey

Copy link
Copy Markdown
Contributor Author

@kripken I just pulled in the latest from main (a6f59c6) but still seeing this error when running tests locally:

Traceback (most recent call last):
  File "/bin/binaryen-lit", line 18, in <module>
    from lit.main import main
ModuleNotFoundError: No module named 'lit'
Traceback (most recent call last):
  File "/./check.py", line 492, in <module>
    sys.exit(main())
             ~~~~^^
  File "/./check.py", line 474, in main
    TEST_SUITES[test]()
    ~~~~~~~~~~~~~~~~~^^
  File "/./check.py", line 427, in wrapper
    raise Exception(f'test suite failed: {name}')
Exception: test suite failed: lit

@kripken

kripken commented Jun 2, 2026

Copy link
Copy Markdown
Member

Which specific lit test is failing? You can run ./check.py lit to run just the lit tests, then can run lit -v [TEST] to run a specific test to debug it.

@stevenfontanella

Copy link
Copy Markdown
Member

@chharvey Looks like you need to pip install lit

@chharvey

chharvey commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor Author

I installed lit but seeing these failures:

Failed Tests (15):
  Binaryen lit tests :: fuzz-types.test
  Binaryen lit tests :: help/wasm-as.test
  Binaryen lit tests :: help/wasm-ctor-eval.test
  Binaryen lit tests :: help/wasm-dis.test
  Binaryen lit tests :: help/wasm-emscripten-finalize.test
  Binaryen lit tests :: help/wasm-fuzz-types.test
  Binaryen lit tests :: help/wasm-merge.test
  Binaryen lit tests :: help/wasm-metadce.test
  Binaryen lit tests :: help/wasm-opt.test
  Binaryen lit tests :: help/wasm-reduce.test
  Binaryen lit tests :: help/wasm-shell.test
  Binaryen lit tests :: help/wasm-split.test
  Binaryen lit tests :: help/wasm2js.test
  Binaryen lit tests :: merge/manifest.wat
  Binaryen lit tests :: passes/stack-ir-eh-legacy.wast

I'm running:

  • Python 3.14.5 (/opt/homebrew/bin/python3 -> ../Cellar/python@3.14/3.14.5/bin/python3)
  • pip 26.1.1 from /opt/homebrew/lib/python3.14/site-packages/pip (python 3.14)
  • lit (/opt/homebrew/bin/lit) version 0.11.0.post1
  • filecheck (/opt/homebrew/opt/llvm@18/bin/filecheck) Homebrew LLVM version 18.1.8
  • macOS Tahoe 26.5.1

I even tried running tests from main branch (latest at 5e0ad2c) and still getting the same errors.

Also, not sure why the CI is failing… looks unrelated to this PR.

@chharvey

chharvey commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

update: found the problem. i forgot the trailing newline from the original function. all mjs tests are passing now

@kripken
kripken merged commit 32a8e1b into WebAssembly:main Jun 5, 2026
16 checks passed
@chharvey
chharvey deleted the feat/8716-stringify-expression branch June 6, 2026 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please provide a stringify-expression function in the C libarary

3 participants