Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Finish instruction renaming #1792

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Finish instruction renaming #1792

merged 2 commits into from
Dec 21, 2021

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Dec 20, 2021

This finishes #985. This

  • replaces the old names in the tests with the new names
  • drops support for the deprecated names
  • renames test files to match new instruction names

I don't think dropping support for the old names will be a problem at
this point. #985 says the old names are supported for convenience but we
should remove those too at some point; that "some point" may have well
arrived given that three years have passed.

The lists of names updated are in #933, #1564, WebAssembly/spec#720.

This finishes WebAssembly#985. This
- replaces the old names in the tests with the new names
- drops support for the deprecated names
- renames test files to match new instruction names

I don't think dropping support for the old names will be a problem at
this point. WebAssembly#985 says the old names are supported for convenience but we
should remove those too at some point; that "some point" may have well
arrived given that three years have passed.

The lists of names updated are in WebAssembly#933, WebAssembly#1564, WebAssembly/spec#720.
@aheejin aheejin requested review from kripken, sbc100 and binji December 20, 2021 18:55
@aheejin
Copy link
Member Author

aheejin commented Dec 20, 2021

@kripken @sbc100 This contains a renaming of wasm_rt_anyfunc_t -> wasm_rt_funcref_t in wasm2c. Would this make any problem for Emscripten? (I'm not sure if there's any syncing going on between Emscripten and Wabt, so I'm asking)

@@ -1122,7 +1122,7 @@ void CWriter::WriteElemInitializers() {

Write(ExternalRef(table->name), ".data[offset + ", i,
"] = (wasm_rt_elem_t){func_types[", func_type_index,
"], (wasm_rt_anyfunc_t)", ExternalPtr(func->name), "};", Newline());
Copy link
Member

Choose a reason for hiding this comment

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

Renaming this without renaming all the other wasm_rt_anyfunc_t in wasm2c seems odd. wasm_rt_funcref_t doesn't seem to appear anywhere in wasm2c but I see several references to wasm_rt_anyfunc_t and a typedef for it.

Is this change needed for this PR? If not perhaps revert this file and create a seperate PR to change all the wasm_rt_anyfunc_t in wasm2c?

Copy link
Member Author

@aheejin aheejin Dec 20, 2021

Choose a reason for hiding this comment

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

I also renamed it in wasm2c/wasm-rt.h. These two files are the only occurrence of wasm_rt_anyfunc_t in code in the current codebase. (Other than README.md) I also renamed that typedef:

--- a/wasm2c/wasm-rt.h
+++ b/wasm2c/wasm-rt.h
@@ -88,10 +88,10 @@ typedef enum {
   WASM_RT_F64,
 } wasm_rt_type_t;

-/** A function type for all `anyfunc` functions in a Table. All functions are
+/** A function type for all `funcref` functions in a Table. All functions are
  * stored in this canonical form, but must be cast to their proper signature to
  * call. */
-typedef void (*wasm_rt_anyfunc_t)(void);
+typedef void (*wasm_rt_funcref_t)(void);

 /** A single element of a Table. */
 typedef struct {
@@ -99,7 +99,7 @@ typedef struct {
   uint32_t func_type;
   /** The function. The embedder must know the actual C signature of the
    * function and cast to it before calling. */
-  wasm_rt_anyfunc_t func;
+  wasm_rt_funcref_t func;
 } wasm_rt_elem_t;

But if this can be problematic I can split it to another PR. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

After running all tests, out/test/wasm2c directory seems to correctly contains files with wasm_rt_funcref_t:

aheejin@aheejin:~/wabt/out$ grep wasm_rt_funcref_t * -R
test/wasm2c/spec/func_ptrs/func_ptrs.9.c:  w2c_T0.data[offset + 0] = (wasm_rt_elem_t){func_types[0], (wasm_rt_funcref_t)(&w2c_f0)};
test/wasm2c/spec/func_ptrs/func_ptrs.9.c:  w2c_T0.data[offset + 1] = (wasm_rt_elem_t){func_types[0], (wasm_rt_funcref_t)(&w2c_f1)};
test/wasm2c/spec/func_ptrs/func_ptrs.8.c:  w2c_T0.data[offset + 0] = (wasm_rt_elem_t){func_types[0], (wasm_rt_funcref_t)(&w2c_f0)};
test/wasm2c/spec/func_ptrs/func_ptrs.8.c:  w2c_T0.data[offset + 1] = (wasm_rt_elem_t){func_types[0], (wasm_rt_funcref_t)(&w2c_f1)};
...

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % the wasm2c change

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants