- User Since
- Nov 24 2017, 7:16 AM (7 w, 5 d)
Thanks for splitting. It would be an extremely long chain of patches if I submitted them in chunks this small - I hope the chunks I'm using are roughly the right size for sending to you to (I could go bigger or smaller).
Thanks for the reviews! I created a fair amount of work for you with these.
In fact, I wonder now as I re-do the calculations if I shouldn't in fact use 16KiB page size (reducing wastage to 10% for 129KiB allocations, at the expense of having a 16KiB mmap bitmap.)
- Simple rebase to fix conflict with __heap_base change
- Fixed test after rebase on top of __heap_base change
- Remove copy for already-sorted input
- Simple rebase to fix conflict
- Reverted change to remove imports from NAME section. They are now written out as before.
- Revert change to remove imports from NAME section; now names are written out for imported functions as before
- Renamed constant
- Responded to comments (formatting/tidying changes)
Updated: split up the big review into six smaller ones.
Updated: split up the big review into six smaller ones. This is the core change still, the change that motivated it all!
Updated: tiny tweak to calculateImports to fix up case of unresolved weak globals
Updated: Fix mistake not exporting weak flag
Mon, Jan 15
Nice, thank you! My Musl port makes use of this, I'll test it sometime and confirm it's working. I do need to get onto that and try and work out how to make it useful to the Emscripten folks or the compat waterfall.
Simple rebase to fix conflicts. The two problems above remain.
Simple rebase to fix conflicts.
Sat, Jan 13
Fri, Jan 12
More updates, fixed all but two tests.
Thank you for reviewing and landing! And also for the "setComdat/isComdat" fix in the Clang WasmObjectWriter, I missed a trick there.
- Some fixes after rebase
- Most tests now passing (a couple of bugs still to iron out)
- Responded to Sam's feedback
- Fixed all test failures
Fine by me then!
Simple rebase, to fix conflict with master after latest commits
I've just rewritten the name handling in D41955. What I did there was to associate the name with the Function (not the Symbol) since that's conceptually what the "names" section is (the debugger's name for the function body itself).
Thu, Jan 11
Thanks for the early feedback!
I'm abandoning this!
Updated to be on top of D40845, and removed non-quite-correct handling of globals/segments.
- Disentangled from D41390
- Changed to assume that all Functions/InputSegments are covered by Symbols. The "discardable" entities are the function and segment definitions, not the Symbols referring to them, so previously I was iterating over the Functions/Segments to make sure they were discarded. We know though that the MCWasmWriter will never output a COMDAT Function/Segment without an accompanying Symbol, so the change is safe.
Wed, Jan 10
Rebased as requested.
Rebased as requested. Tests pass.
Looks fine, it's just moving some code around (to a better place).
Tue, Jan 9
MC tests pass, but I've just noticed I missed one in CodeGen/WebAssembly/comdat.ll. I won't be able to fix it today, sorry..
NB. I haven't yet re-run the tests, this is an optimistic update - it takes a looong time to compile everything, and after a rebase pretty much everything needs to recompile. Just building the tests now.
Sat, Dec 23
Fri, Dec 22
Thanks, you're right that's nicer. I'll leave the bugzilla issue open for now until the Linking.Mr issue is closed as well.
Thu, Dec 21
Looks like it should work. One thing I wonder about is the size of the table? It looks like we're now writing out a table entry for each relocation, which is (a tiny amount of) bloat and makes function pointers not compare equal (which is irrelevant for the object file which can't run anyway). It was neater before to be writing out the table entry only once per symbol.
Ouch, hacky hacky!
I haven't looked into this in detail (I'd need to hand-inspect the wasm-dis output to be sure that the obj2yaml output is actually correct). But I'm sure you've done that! Fine by me.
Since the relocation is emitted in the function body, which is tested, it should be good enough for now.
Wed, Dec 20
Rebased on top of Sam's refactoring work...
- Rebased to be on top of Sam's D41419
- Added handling of global symbols, allowing InputSegments to be discarded as well if the InputSegment contains nothing else apart from the discarded Global
Oops, rebased to master than on top of my previous patches
- Responded to Sam's comments
- Fixed a minor issue with the function indices
- Fixed a bug in my addition to yaml2wasm
Sorry also for the lengthy responses!
Tue, Dec 19
Looks nice. My changes for pruning functions/segments rely on the relocations happening nice and late, so that we don't try and generate the index of a function that's being pruned. There changes all help with that, ensuring that we don't need the relocations to be "resolvable" for code that we're discarding.
I've now split some of this out into D41390.
Updated to make the diff a bit shorter/tidier.
Abandoning. There's not much that can be salvaged from here, it's all superseded.