Adds more testing in basic-assembly.s and a new test tables.s.
Adds support to yaml reading and writing of tables as well.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks good to me! I would want to wait and see what @sbc100 thinks, too. It would also be good to add that missing newline in tables.s.
Looks great! Just a couple of nits
llvm/include/llvm/MC/MCSymbolWasm.h | ||
---|---|---|
114 | Should we use ValType for table type? | |
llvm/lib/MC/WasmObjectWriter.cpp | ||
1529 | Presumably the limits should be part of the type eventually? | |
llvm/lib/ObjectYAML/WasmEmitter.cpp | ||
365 | Oops.. I guess we don't have enough testing here. | |
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp | ||
84 | Would save this static cast too if that was ValType maybe? | |
llvm/test/MC/WebAssembly/tables.s | ||
43 | Add newline |
llvm/lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
1529 | I agree but I can work on moving the limits in a future patch, if that's fine with you. |
There were some tests failing due to mismatched expected output.
In all of them I missed adding the "Index: 0" to the table output. If tests pass in harbormaster then we are good to land.
It looks like there are a bunch of wasm-ld tests that need similar updates.
$ ../llvm-build/bin/llvm-lit lld/test/wasm/ ... ... Failed Tests (18): lld :: wasm/alias.s lld :: wasm/call-indirect.ll lld :: wasm/export-table.test lld :: wasm/growable-table.test lld :: wasm/import-table.test lld :: wasm/local-symbols.ll lld :: wasm/locals-duplicate.test lld :: wasm/pie.ll lld :: wasm/relocatable.ll lld :: wasm/shared-memory-no-atomics.yaml lld :: wasm/shared-memory.yaml lld :: wasm/shared.ll lld :: wasm/stack-pointer.ll lld :: wasm/undefined-weak-call.ll lld :: wasm/weak-alias-overide.ll lld :: wasm/weak-alias.ll lld :: wasm/weak-symbols.ll lld :: wasm/weak-undefined.ll
I seeing:
llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:111:11: warning: enumeration value 'FUNCREF' not handled in switch [-Wswitch] switch (VT) {
Oh, I was not getting that. It seems to come from https://github.com/llvm/llvm-project/commit/3bba91f64eef15956f589fa446c265a714cc7893 and I hadn't pulled that in yet. If I rebase my patch on this then yes, I can see the warning. i will fix it in this patch. No worries.
clang-tidy: error: no viable conversion from 'llvm::wasm::WasmLimits' to 'uint8_t' (aka 'unsigned char') [clang-diagnostic-error]
not useful
clang-tidy: warning: suggest braces around initialization of subobject [clang-diagnostic-missing-braces]
not useful
clang-tidy: warning: missing field 'Initial' initializer [clang-diagnostic-missing-field-initializers]
not useful