Skip to content

Poppy IR wast parsing and validation #3105

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
tlively merged 1 commit into from
Sep 9, 2020
Merged

Poppy IR wast parsing and validation #3105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/binaryen-c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3421,7 +3421,7 @@ BinaryenModuleRef BinaryenModuleParse(const char* text) {
try {
SExpressionParser parser(const_cast<char*>(text));
Element& root = *parser.root;
SExpressionWasmBuilder builder(*wasm, *root[0]);
SExpressionWasmBuilder builder(*wasm, *root[0], IRProfile::Normal);
} catch (ParseException& p) {
p.dump(std::cerr);
Fatal() << "error in parsing wasm text";
Expand Down
23 changes: 23 additions & 0 deletions src/ir/stack-utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

#include "stack-utils.h"
#include "ir/properties.h"

namespace wasm {

Expand All @@ -30,6 +31,28 @@ void removeNops(Block* block) {
block->list.resize(newIndex);
}

bool mayBeUnreachable(Expression* expr) {
if (Properties::isControlFlowStructure(expr)) {
return true;
}
Copy link
Member

@aheejin aheejin Sep 9, 2020

Choose a reason for hiding this comment

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

Can't this use StackSignature and precisely compute whether it is really unreachable or not?

Copy link
Member Author

@tlively tlively Sep 9, 2020

Choose a reason for hiding this comment

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

We can use StackSignature (or just look at expr->type) to figure out if it is unreachable, but what we want to do in this function is determine if it is allowed to be unreachable.

switch (expr->_id) {
case Expression::Id::BreakId:
return expr->cast<Break>()->condition == nullptr;
case Expression::Id::CallId:
return expr->cast<Call>()->isReturn;
case Expression::Id::CallIndirectId:
return expr->cast<CallIndirect>()->isReturn;
case Expression::Id::ReturnId:
case Expression::Id::SwitchId:
case Expression::Id::UnreachableId:
case Expression::Id::ThrowId:
case Expression::Id::RethrowId:
return true;
default:
return false;
}
}

} // namespace StackUtils

StackSignature::StackSignature(Expression* expr) {
Expand Down
14 changes: 12 additions & 2 deletions src/ir/stack-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@
// stack type of each instruction. Pops may not have `unreachable` type.
//
// 4. Only control flow structures and instructions that have polymorphic
// unreachable behavior in WebAssembly may have unreachable type.
// unreachable behavior in WebAssembly may have unreachable type. Blocks may
// be unreachable when they are not branch targets and when they have an
// unreachable child. Note that this means a block may be unreachable even
// if it would otherwise have a concrete type, unlike in Binaryen IR.
Copy link
Member

@aheejin aheejin Sep 9, 2020

Choose a reason for hiding this comment

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

Where do we do this? Poppy IR doesn't seem to have its own Block::finalize() or something and it shares the parser with Binaryen IR, aren't block types in Poppy IR assigned in the same way as that of Binaryen IR?

Copy link
Member Author

@tlively tlively Sep 9, 2020

Choose a reason for hiding this comment

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

Eventually we will need to add alternate logic to Block::finalize() to handle Poppy IR. The reason I haven't done this yet is because the parser only calls Block::finalize(Type), so none of the tests call Block::finalize(), so we would have no test coverage. The next PR will add the stackification pass, which will call refinalize, so I will implement this with test coverage then.

Copy link
Member

@aheejin aheejin Sep 9, 2020

Choose a reason for hiding this comment

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

For example,

(block (result i32)
  (unreachable)
)

According to your explanation, this block's type will be i32 in Binaryen IR and unreachable in Poppy IR. This block has a result type so Block::finalize(type) will be called. But the current Block::finalize(type) respects the type given as the argument and does not check instructions inside for unreachables, no?

What I meant was, as you said, given that we only use Block::finalize(type) for now, how we determine the unreachability of a block will be the same for Binaryen IR and Poppy IR, so this comments block does not seem to match the current implementation. I think I might be missing something.

Copy link
Member Author

@tlively tlively Sep 9, 2020

Choose a reason for hiding this comment

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

In both Binaryen IR and Poppy IR, the block in your example can have either i32 or unreachable as its type. In Binaryen IR, the parser will give it i32 type and subsequent optimizations will change it to have unreachable type. In Poppy IR, the parser will similarly give it i32 type, but we don't yet have any code that will change the type to unreachable (even though that would be allowed).

The difference between Poppy IR and Binaryen IR is in a block like this:

(block
  (unreachable)
  (i32.const 0)
)

In Binaryen IR, it would be illegal for that block to have unreachable type, but in Poppy IR it could still have either unreachable or i32 type.

Copy link
Member

@aheejin aheejin Sep 9, 2020

Choose a reason for hiding this comment

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

I see, thanks. Is there any particular reason for this distinction?

Copy link
Member Author

@tlively tlively Sep 9, 2020

Choose a reason for hiding this comment

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

Yes, it has to do with type inference in Poppy IR, which we don't do in Binaryen IR. Consider the following function:

(func (result i32)
  (block $body
    (unreachable)
    (i32.const 0)
  )
)

Binaryen IR unconditionally gives unreachable blocks none type when writing them out, so if $body were allowed to be unreachable in Binaryen IR, there resulting binary would be invalid. In contrast, Poppy IR does type inference before emitting binaries, which correctly gives $body an i32 type.

//
// For example, the following Binaryen IR Function:
//
Expand All @@ -58,7 +61,10 @@
// )
//
// Notice that the sequence of instructions in the block is now identical to the
// sequence of instructions in raw WebAssembly.
// sequence of instructions in raw WebAssembly. Also note that Poppy IR's
// validation rules are largely additional on top of the normal Binaryen IR
// validation rules, with the only exceptions being block body validation and
// block unreahchability rules.
//

#ifndef wasm_ir_stack_h
Expand All @@ -75,6 +81,10 @@ namespace StackUtils {
// Iterate through `block` and remove nops.
void removeNops(Block* block);

// Whether `expr` may be unreachable in Poppy IR. True for control flow
// structures and polymorphic unreachable instructions.
bool mayBeUnreachable(Expression* expr);
Copy link
Member

@kripken kripken Sep 9, 2020

Choose a reason for hiding this comment

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

I still think my comment about this is relevant: tlively#1 (comment)

The comment here doesn't clearly say which of the two cases it is IMO. If it is "not exited normally" or such, that might be good to clarify?

Copy link
Member

@kripken kripken Sep 9, 2020

Choose a reason for hiding this comment

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

I mean, is "unreachable" here referring to a type in the type system, or to a particular property, etc.


} // namespace StackUtils

// Stack signatures are like regular function signatures, but they are used to
Expand Down
3 changes: 2 additions & 1 deletion src/passes/RemoveNonJSOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ struct RemoveNonJSOpsPass : public WalkerPass<PostWalker<RemoveNonJSOpsPass>> {
std::string input(IntrinsicsModuleWast);
SExpressionParser parser(const_cast<char*>(input.c_str()));
Element& root = *parser.root;
SExpressionWasmBuilder builder(intrinsicsModule, *root[0]);
SExpressionWasmBuilder builder(
intrinsicsModule, *root[0], IRProfile::Normal);

std::set<Name> neededFunctions;

Expand Down
9 changes: 8 additions & 1 deletion src/tools/tool-options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ struct ToolOptions : public Options {
PassOptions passOptions;

bool quiet = false;
IRProfile profile = IRProfile::Normal;

ToolOptions(const std::string& command, const std::string& description)
: Options(command, description) {
Expand Down Expand Up @@ -67,7 +68,13 @@ struct ToolOptions : public Options {
"-q",
"Emit less verbose output and hide trivial warnings.",
Arguments::Zero,
[this](Options*, const std::string&) { quiet = true; });
[this](Options*, const std::string&) { quiet = true; })
.add(
"--experimental-poppy",
"",
"Parse wast files as Poppy IR for testing purposes.",
Arguments::Zero,
[this](Options*, const std::string&) { profile = IRProfile::Poppy; });
(*this)
.addFeature(FeatureSet::SignExt, "sign extension operations")
.addFeature(FeatureSet::Atomics, "atomic operations")
Expand Down
2 changes: 1 addition & 1 deletion src/tools/wasm-as.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ int main(int argc, const char* argv[]) {
if (options.debug) {
std::cerr << "w-parsing..." << std::endl;
}
SExpressionWasmBuilder builder(wasm, *root[0]);
SExpressionWasmBuilder builder(wasm, *root[0], options.profile);
} catch (ParseException& p) {
p.dump(std::cerr);
Fatal() << "error in parsing input";
Expand Down
1 change: 1 addition & 0 deletions src/tools/wasm-opt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ int main(int argc, const char* argv[]) {
// asked to remove it.
reader.setDWARF(options.passOptions.debugInfo &&
!willRemoveDebugInfo(options.passes));
reader.setProfile(options.profile);
try {
reader.read(options.extra["infile"], wasm, inputSourceMapFilename);
} catch (ParseException& p) {
Expand Down
4 changes: 2 additions & 2 deletions src/tools/wasm-shell.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ static void run_asserts(Name moduleName,
std::unique_ptr<SExpressionWasmBuilder> builder;
try {
builder = std::unique_ptr<SExpressionWasmBuilder>(
new SExpressionWasmBuilder(wasm, *curr[1]));
new SExpressionWasmBuilder(wasm, *curr[1], IRProfile::Normal));
} catch (const ParseException&) {
invalid = true;
}
Expand Down Expand Up @@ -306,7 +306,7 @@ int main(int argc, const char* argv[]) {
auto module = wasm::make_unique<Module>();
Name moduleName;
auto builder = wasm::make_unique<SExpressionWasmBuilder>(
*module, *root[i], &moduleName);
*module, *root[i], IRProfile::Normal, &moduleName);
builders[moduleName].swap(builder);
modules[moduleName].swap(module);
i++;
Expand Down
5 changes: 3 additions & 2 deletions src/tools/wasm2js.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ void AssertionEmitter::emit() {
asmModule = Name(moduleNameS.str().c_str());
Module wasm;
options.applyFeatures(wasm);
SExpressionWasmBuilder builder(wasm, e);
SExpressionWasmBuilder builder(wasm, e, options.profile);
emitWasm(wasm, out, flags, options.passOptions, funcName);
continue;
}
Expand Down Expand Up @@ -972,7 +972,8 @@ int main(int argc, const char* argv[]) {
if (options.debug) {
std::cerr << "w-parsing..." << std::endl;
}
sexprBuilder = make_unique<SExpressionWasmBuilder>(wasm, *(*root)[0]);
sexprBuilder =
make_unique<SExpressionWasmBuilder>(wasm, *(*root)[0], options.profile);
}
} catch (ParseException& p) {
p.dump(std::cerr);
Expand Down
4 changes: 4 additions & 0 deletions src/wasm-io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class ModuleReader {
// the binary, so that we can update DWARF sections later when writing.
void setDWARF(bool DWARF_) { DWARF = DWARF_; }

void setProfile(IRProfile profile_) { profile = profile_; }

// read text
void readText(std::string filename, Module& wasm);
// read binary
Expand All @@ -49,6 +51,8 @@ class ModuleReader {
private:
bool DWARF = false;

IRProfile profile = IRProfile::Normal;

void readStdin(Module& wasm, std::string sourceMapFilename);

void readBinaryData(std::vector<char>& input,
Expand Down
2 changes: 2 additions & 0 deletions src/wasm-s-parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ class SExpressionParser {
class SExpressionWasmBuilder {
Module& wasm;
MixedArena& allocator;
IRProfile profile;
std::vector<Signature> signatures;
std::unordered_map<std::string, size_t> signatureIndices;
std::vector<Name> functionNames;
Expand All @@ -127,6 +128,7 @@ class SExpressionWasmBuilder {
// Assumes control of and modifies the input.
SExpressionWasmBuilder(Module& wasm,
Element& module,
IRProfile profile,
Name* moduleName = nullptr);

private:
Expand Down
3 changes: 3 additions & 0 deletions src/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ struct Address {
}
};

enum class IRProfile { Normal, Poppy };

// Operators

enum UnaryOp {
Expand Down Expand Up @@ -1260,6 +1262,7 @@ class Function : public Importable {
public:
Name name;
Signature sig; // parameters and return value
IRProfile profile = IRProfile::Normal;
std::vector<Type> vars; // non-param locals

// The body of the function
Expand Down
8 changes: 4 additions & 4 deletions src/wasm/wasm-io.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,16 @@ namespace wasm {

#define DEBUG_TYPE "writer"

static void readTextData(std::string& input, Module& wasm) {
static void readTextData(std::string& input, Module& wasm, IRProfile profile) {
SExpressionParser parser(const_cast<char*>(input.c_str()));
Element& root = *parser.root;
SExpressionWasmBuilder builder(wasm, *root[0]);
SExpressionWasmBuilder builder(wasm, *root[0], profile);
}

void ModuleReader::readText(std::string filename, Module& wasm) {
BYN_TRACE("reading text from " << filename << "\n");
auto input(read_file<std::string>(filename, Flags::Text));
readTextData(input, wasm);
readTextData(input, wasm, profile);
}

void ModuleReader::readBinaryData(std::vector<char>& input,
Expand Down Expand Up @@ -113,7 +113,7 @@ void ModuleReader::readStdin(Module& wasm, std::string sourceMapFilename) {
s.write(input.data(), input.size());
s << '\0';
std::string input_str = s.str();
readTextData(input_str, wasm);
readTextData(input_str, wasm, profile);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/wasm/wasm-s-parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,8 +314,9 @@ Element* SExpressionParser::parseString() {

SExpressionWasmBuilder::SExpressionWasmBuilder(Module& wasm,
Element& module,
IRProfile profile,
Name* moduleName)
: wasm(wasm), allocator(wasm.allocator) {
: wasm(wasm), allocator(wasm.allocator), profile(profile) {
if (module.size() == 0) {
throw ParseException("empty toplevel, expected module");
}
Expand Down Expand Up @@ -795,6 +796,7 @@ void SExpressionWasmBuilder::parseFunction(Element& s, bool preParseImport) {
// make a new function
currFunction = std::unique_ptr<Function>(Builder(wasm).makeFunction(
name, std::move(params), sig.results, std::move(vars)));
currFunction->profile = profile;

// parse body
Block* autoBlock = nullptr; // may need to add a block for the very top level
Expand Down
Loading