feat:! Use segment names #6254

ericvergnaud · 2024-01-30T12:57:50Z

Fixes #6247

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

This reverts commit efc7fd4.

kripken

Nice work, lgtm aside from one suggestion.

kripken · 2024-01-30T18:59:56Z

src/binaryen-c.h

                                    const char* exportName,
-                                    const char** segments,
-                                    bool* segmentPassive,
+                                    const char** segmentNames,


Perhaps we could allow segmentNames to be NULL, and then we autogenerate the names? That's much simpler for people that don't need the names, equally as simple as before.

Agreed, done.

Please document this change in the comment before this declaration.

kripken · 2024-01-30T19:04:23Z

(looks like there's a CI error too)

ericvergnaud · 2024-01-30T19:20:53Z

(looks like there's a CI error too)

It comes from code coverage, but I have no clue how this PR's changes can possibly affect the code coverage findings...

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

src/js/binaryen.js-post.js

kripken · 2024-01-30T23:21:29Z

Sorry, I didn't mean code coverage (that can be ignored), but the emscripten (JS) build:

https://github.com/WebAssembly/binaryen/actions/runs/7717838024/job/21037807450?pr=6254

ericvergnaud · 2024-01-31T08:21:21Z

Sorry, I didn't mean code coverage (that can be ignored), but the emscripten (JS) build:

https://github.com/WebAssembly/binaryen/actions/runs/7717838024/job/21037807450?pr=6254

It was CodeCov in my initial submission, digging into the latest...

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

ericvergnaud · 2024-01-31T18:24:35Z

@kripken the remaining failure in emscripten make no sense to me...
It finds 2 diffs between the kitchen-sink.js output and its expected output.

The first one is:

Not sure why it sees a diff between // as being different from // ?! I haven't touched that area at all...

The second one is:

Again it sees a diff between 2 lines that are visually equal, and it's an area that I haven't touched at all.

This smells like an encoding issue unrelated to my changes...
Are this repo and the CI configured to deal with line endings ? (see https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings)

ericvergnaud · 2024-01-31T22:51:08Z

Looks like I am going to need help...
I tried forcing the update by changing non-whitespace, and then revert that change whilst adding the missing whitespace, to no end.
Git filters out the whitespace-only changes...

This reverts commit b6bc3c8.

ericvergnaud · 2024-01-31T23:19:27Z

I've tried running auto_update_tests.py, but it doesn't regenerate the binaryen.js txt files, maybe because of this line:
skip_by_default = ['binaryenjs']

…inaryen into use-segment-names

ericvergnaud · 2024-01-31T23:35:35Z

ok I managed to fix the trailing spaces issue by editing the file using GH web editor...
I've reverted the test harness changes

kripken · 2024-02-01T17:13:48Z

Sorry, I should have said, ./scripts/auto_update_tests.py binaryenjs should autoupdate those. (by default binaryenjs is skipped because it's a separate build, that not all devs constantly keep in sync)

kripken · 2024-02-01T17:16:48Z

test/binaryen.js/kitchen-sink.js

+  try {
+    m();
+  } catch(e) {
+    console.trace();


How helpful is this? The stack trace of the error is on e anyhow, so I'm not sure what this can add here in this catch block, but I'm not familiar with console.trace...

in theory console.trace prints the stack trace of the last raised error, see https://developer.mozilla.org/fr/docs/Web/API/console/trace_static
but it doesn't seem to help much tbh, so I can remove it

Let's remove it then, if there isn't a clear benefit. That stack trace should be printed anyhow by node for an uncaught exception.

kripken

lgtm % test question

ericvergnaud · 2024-02-01T17:46:40Z

Sorry, I should have said, ./scripts/auto_update_tests.py binaryenjs should autoupdate those. (by default binaryenjs is skipped because it's a separate build, that not all devs constantly keep in sync)

That works but raises the question: what exactly are we testing ? If the generated data is checked against stored data that was actually generated by the same code, it only covers NR testing, but not correctness ? Is that the intent ?

(if we move forward with the merge/TypeScript proposal, I could implement (gradually) a series of granular unit tests that would check for correctness if it makes sense)

kripken · 2024-02-01T18:45:30Z

What is "NR" in this context?

Yeah, these particular tests are not "functional" tests, they only compare to "golden" outputs that we trust. That is, during review we carefully verify the updates to the test expectations, and after review and landing, if a later PR would cause a change to the output then a test fails.

We also need functional tests, of course. We get that in other parts of the test suite, by assertions and validation that happen in all tests, and via the fuzzer.

(Are there better terms for these test types?)

ericvergnaud · 2024-02-01T21:25:20Z

"NR" means non-regression.
Yes there are somewhat standard terms, at least in the information system space where I've spent most of my time:

unit testing, that check behavior at a granular level: function, class, component
integration testing, that ensure consistent behavior of connected components
functional testing, that check expected behavior end to end (without caring for state)
fuzz testing, that checks for resilience
performance testing, self-explanatory
acceptance testing, that is normally performed by the business (here it would be binaryen users)
I suspect binaryen does a bit of all this (except acceptance) but maybe not in a structured way ? Also, the bulk testing approach makes it very difficult to identify what's causing a failure... I suspect that approach is largely driven by the fact that C++ doesn't have an equivalent of JUnit. Since that's easy to implement at the JS/TS level, that would in turn make it easier to locate an issue.

kripken · 2024-02-01T21:36:32Z

I see, thanks! I haven't seen enough usage of those terms to internalize them I guess, but most are familiar to me (and the concepts are).

We do all of the above, though performance testing of the compiler itself is not tracked (but the output is), and I guess for acceptance testing it depends how you consider Emscripten (Emscripten is a user of Binaryen and tests it heavily as part of testing itself).

For unit tests we do use gtest in the test/gtest subdir (also the older test/unit/).

Move from segment indexes to names. This is a breaking change to make the API more capable and consistent. An effort has been made to reduce the burden on C API users where possible (specifically, you can avoid providing names and let Binaryen make them for you, which will basically be numbers that match the indexes from before). Fixes WebAssembly#6247

ericvergnaud added 12 commits January 30, 2024 12:16

update c++ api

60040c3

Use segment names instead of indices

5c66694

use segment names instead of segment indices

53cd9a6

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

fix formatting

2599ea8

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

fix failing js tests

2ef578e

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

fix failing js tests

19d841a

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

fix failing tests

3471b35

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

fix failing tests

081f3f0

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

fix failing tests

de0f397

troubleshoot failing tests

efc7fd4

fix failing tests

47ea12b

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

Revert "troubleshoot failing tests"

19dc106

This reverts commit efc7fd4.

kripken reviewed Jan 30, 2024

View reviewed changes

ericvergnaud added 7 commits January 30, 2024 20:26

allow segment names to be null

d42e44c

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

something slipped...

b42756d

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

support anonymous segments in js too

7e03f19

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

long day...

960bea5

make names explicit only when provided

bf35b05

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

make names explicit only when provided

25a67e8

bad day, switching between languages...

c35cdf9

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

kripken reviewed Jan 30, 2024

View reviewed changes

src/js/binaryen.js-post.js Show resolved Hide resolved

ericvergnaud added 2 commits January 31, 2024 09:19

fix indent

c9ab883

print JS stack trace in emcc tests

f9ebb93

ericvergnaud added 2 commits January 31, 2024 09:28

print JS stack trace in emcc tests

c839752

fix failing tests

b6bc3c8

Signed-off-by: Eric Vergnaud <eric.vergnaud@wanadoo.fr>

ericvergnaud added 3 commits January 31, 2024 23:44

reverse forced update

aa4a907

force update

ed97acb

last attempt...

d15d3c8

ericvergnaud added 5 commits January 31, 2024 23:55

Revert "fix failing tests"

e219adf

This reverts commit b6bc3c8.

remove to recreate

a90fb38

recreate

32b1d92

remove and push ?

58fb2e6

restore

24f923f

ericvergnaud added 5 commits February 1, 2024 00:21

add test file

4f420f1

Update kitchen-sink.js.txt

81f94b5

revert changes in test harness

deb4694

Merge branch 'use-segment-names' of https://github.com/ericvergnaud/b…

0ae7ac4

…inaryen into use-segment-names

Update kitchen-sink.js.txt

0904749

restore incorrectly reverted change

416d80c

kripken reviewed Feb 1, 2024

View reviewed changes

remove test wrapper

f7f44b9

kripken approved these changes Feb 1, 2024

View reviewed changes

kripken merged commit 2b3a2e8 into WebAssembly:main Feb 1, 2024

ericvergnaud deleted the use-segment-names branch February 1, 2024 21:39

gkdn mentioned this pull request Aug 31, 2024

stringconsts gkdn/binaryen#1

Closed

feat:! Use segment names #6254

feat:! Use segment names #6254

Uh oh!

Conversation

ericvergnaud commented Jan 30, 2024

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jan 30, 2024

Uh oh!

ericvergnaud commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kripken commented Jan 30, 2024

Uh oh!

ericvergnaud commented Jan 31, 2024

Uh oh!

ericvergnaud commented Jan 31, 2024

Uh oh!

ericvergnaud commented Jan 31, 2024

Uh oh!

ericvergnaud commented Jan 31, 2024

Uh oh!

ericvergnaud commented Jan 31, 2024

Uh oh!

kripken commented Feb 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

ericvergnaud commented Feb 1, 2024

Uh oh!

kripken commented Feb 1, 2024

Uh oh!

ericvergnaud commented Feb 1, 2024

Uh oh!

kripken commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericvergnaud commented Jan 30, 2024 •

edited

Loading