[NFC] Refactor symbol table creation in binary writer #1535
Conversation
src/binary-writer.cc
Outdated
| auto iter = symtab_.find(name); | ||
| if (iter != symtab_.end()) { | ||
| Index sym_index = iter->second; | ||
| assert(symbols_[sym_index].type() == T::type); |
There was a problem hiding this comment.
I'm not sure whether this assert is safe; seems like it could be invalid based on malformed data in wasm module. @sbc100 do you know?
There was a problem hiding this comment.
Good point; let's print a warning in that case instead of failing.
In any case, the current logic doesn't actually follow the toolchain-conventions invariants; symbols that are neither imported nor exported by a module have no name uniqueness requirements AFAIU. I will fix the logic in a followup.
wingo
force-pushed
the
refactor-symbol-table-writing
branch
from
bc34d78
to
f185838
Compare
This commit refactors the representations of symbols in the binary writer. It will allow us to fix WebAssembly#1534 in a followup.
wingo
force-pushed
the
refactor-symbol-table-writing
branch
from
f185838
to
55f2a91
Compare
|
Updated patch addresses review comments, except regarding tests: because this patch has no functional change (it doesn't fixed the linked issues yet), no test needs to be modified. I'll extend the |
|
(And apologies wrt force-pushes; I gather the convention is to push commits to fix nits, then you rebase/squash when landing -- will do that next time.) |
I don't think we have any rules around this. I basically always use a rebase flow which means I often/always have to force push to github PRs (even if i don't squash, which I also occasionally do too). |
| bool explicit_name() const { return flags() & WABT_SYMBOL_FLAG_EXPLICIT_NAME; } | ||
| bool no_strip() const { return flags() & WABT_SYMBOL_FLAG_NO_STRIP; } | ||
|
|
||
| bool IsFunction() const { return type() == Function::type; } |
There was a problem hiding this comment.
Why not just use SymbolType::Function here? What purpose does the Function::type serve?
There was a problem hiding this comment.
The purpose of the ::type definitions is to allow InternSymbol to know what kind of symbol to make. I admit that I wasn't sure which name to use here in IsFunction and friends; if you prefer that I change it, I am happy to do so.
There was a problem hiding this comment.
I guess it will make more sense if/when we add data symbol but for now it looks like you could avoid a lot of complexity and avoid InternSymbol being a template at all by just passing the SymbolType to InternSymbol.
It seems like having a separate class nested inside of Symbol, for each different type of symbol payload adds a fair amount of complexity, but perhaps that is justified and will make more sense once the followups land?
lgtm aside from this minor concern.
There was a problem hiding this comment.
I see what you are saying, and if every symbol just had an Index payload it would be entirely clear-cut. I guess since I saw the SymbolType::Data symbols having additional fields, I found it confusing for Symbol to have unused fields for e.g. SymbolType::Function symbols, which would propagate around as arguments to e.g. InternSymbol and related functions. But that is a theoretical concern given that wabt doesn't know anything about data symbols, and my patches don't change that.
The followup #1537 doesn't do anything amazing here; this is just a question of data representation. If you like I can change to a less structured class Symbol { string name; uint8_t flags; SymbolType type; Index index; Offset offset; Address size; } and assert that offset/size accessors only happen for data symbols (of which we will have none), or even exclude data symbols from the representation. WDYT?
There was a problem hiding this comment.
I think I'm good with this approach. What do you think binji?
There was a problem hiding this comment.
I'm happy either way, tbh. I like having more explicit types (i.e. more structured) since it seems like it will be easier to expand if/when we add new symbol values. OTOH, YAGNI and all :)
| @@ -312,34 +401,49 @@ Index BinaryWriter::GetEventVarDepth(const Var* var) { | |||
| return var->index(); | |||
| } | |||
|
|
|||
| template <typename T> | |||
| Index BinaryWriter::InternSymbol(const std::string& name, uint8_t flags, | |||
There was a problem hiding this comment.
Could this have a better name? Perhaps addToSymtab or getSymbolIndex?
| bool explicit_name() const { return flags() & WABT_SYMBOL_FLAG_EXPLICIT_NAME; } | ||
| bool no_strip() const { return flags() & WABT_SYMBOL_FLAG_NO_STRIP; } | ||
|
|
||
| bool IsFunction() const { return type() == Function::type; } |
There was a problem hiding this comment.
I guess it will make more sense if/when we add data symbol but for now it looks like you could avoid a lot of complexity and avoid InternSymbol being a template at all by just passing the SymbolType to InternSymbol.
It seems like having a separate class nested inside of Symbol, for each different type of symbol payload adds a fair amount of complexity, but perhaps that is justified and will make more sense once the followups land?
lgtm aside from this minor concern.
wingo
changed the title
Fine with me, I kinda thought you already could as long as there was a reviewer. Lemme check the settings. |
This commit refactors the representations of symbols in the binary writer. It will allow us to fix #1534 in a followup.