- User Since
- Jan 7 2013, 9:35 AM (403 w, 2 d)
Mon, Sep 28
LGTM too; I think I created wasm::WasmSignature in service of some other purpose (https://reviews.llvm.org/D52580) and never got around to finishing the job of removing this code...
Fri, Sep 25
Thu, Sep 17
rebase, address comment
Well it's been an interesting 6 months since I last looked at this patch!
Thanks for the thorough review, I think I've addressed all the feedback, please take a look.
Wed, Sep 16
Yeah, ELF splits ELFWriter out from ELFObjectWriter, and then instantiates it twice. It's all because ELFObjectWriter has to derive from MCObjectWriter which was clearly not designed with this in mind. I found the class split to be a bit awkward, but I don't really have strong feelings about it either way.
Tue, Sep 15
OK, I think this is ready to review for real. Can you take another look?
upload the correct diff
Get the right sections in the objects, add tests
sorry, Arcanist snafu. This should have been included in a different change
Mon, Sep 14
Fri, Sep 11
Thu, Sep 10
Tue, Sep 8
(And yes, you are right that using metadata for this isn't strictly correct since metadata can be dropped. And I think you are also correct that we haven't added multiple table support for MC or the instruction definitions either).
Thanks @ddcc for the summary. Given that, I do think it makes sense to fix the current implementation before worrying about multiple tables.
Hi, nice to hear from you, and thanks for the patch! I'm going to have to try to page-in all the vague knowledge I have about CFI, because I think it would be really good to get this working for real.
Aug 20 2020
Aug 11 2020
Aug 10 2020
Fix bad diff
Jul 30 2020
Can we pick this up again?
Jul 28 2020
Jul 7 2020
This is a nice simplification.
Jul 6 2020
Jun 30 2020
Jun 29 2020
Jun 25 2020
Compiler changes LGTM
Jun 23 2020
Jun 22 2020
oh i missed that it's tested in the initexprs.
So the code LGTM, were you going to add to usertest.ll in this CL?
Jun 19 2020
The dependent revision in phabricator has the context, but please also add it to the commit description for this change (either directly or via a link) so that it will show up in git as well.
Jun 18 2020
Yeah I think a 64-bit version of userstack.ll makes sense. (a fork or a second set of CHECKs, whatever you think would be cleaner). There's also stack-alignment.ll which covers dynamic stack adjustment.
Just taking a guess on a reviewer...
Jun 17 2020
Jun 15 2020
This is indeed what I had in mind on the other CL.
... Do you think it makes sense/is actually better?
Jun 12 2020
I think this is good now. If the Requires I mentioned above should actually go back in, it can be in a followup CL (presumably where we add MEMORY_SIZE_I64 and friends).
Maybe we could keep using "unstackify" as a verb and use "non-stackified" as an adjective. I like that better than "destackify" anyway.
(I'll use the term "de-stackify" as a verb to avoid confusion).
If fixUnwindMismatches de-stackifies the tee result, does that violate some kind of invariant? is it better to fix it up in fixUnwindMismatches?
Jun 11 2020
(discussed with @tlively offline). The root issue is that the br_table gets duplicated outside the wasm block that encloses the branch targets. That's what causes the irreducible control flow in this case. Because the target-independent passes that do the duplication can't reason about those wasm blocks (since we haven't done the analysis that places them until later in the pass pipeline) it probably just makes sense to use noduplicate, which is a stronger limitation than convergent.
Jun 4 2020
Jun 3 2020
Binaryen doesn't assume an unreachable is actually unreachable, the way C compilers do, does it? (and remove code before the unreachable)? I think it just knows that instructions that come after it are unreachable and removes those. I think it's ok once it's lowered to wasm unreachable; it really won't be able to return (and so Binaryen can do DCE), but that seems like a legit behavior for @llvm.debugtrap according to the langref.
Jun 2 2020
Jun 1 2020
May 29 2020
ah right, i had forgotten that.
no worries, I'm probably the right one to be reviewing this; I've thought a lot about linking, even if I didn't write any of this code :D
speaking of which, can you also add the _REL_ relocations to the linking.md doc in tool-conventions?
May 28 2020
oh also "WebbAssembly" in the CL title should be "WebAssembly"
May 26 2020
May 21 2020
If there's a difference between wouter's clang-format output and heejin's / the bots, it could be that the default clang-format on debian is really old. that's an issue I've seen before.
https://sourceware.org/binutils/docs/as/ is my go-to reference for assembly.
May 18 2020
May 15 2020
You know, it's pretty amazing that WebAssemblyISelLowering.cpp is as small as it is.
I guess the existing tests didnt catch this because the pop is still within the same subprogram. Did you see this just at the end of a function, or where?
May 14 2020
great, still LGTM
May 12 2020
May 11 2020
Actually, would it be possible to not ignore throw() but make it an alias for noexcept(true)? Apparently that is the standard behavior in C++17, so it might make more sense to just implement that now rather than just warning all the time and ignoring it. It would also cover most of the cases that exist, so more users wouldn't need to disable the warning.
May 6 2020
May 5 2020
So the plan is to support unresolved-symbols and then eventually deprecate and remove this?
May 4 2020
Wow, did our one external partner test case trigger all of these?
Apr 23 2020
We can definitely still talk about what you are trying to do, and it would probably be useful to have more folks involved. Opening an issue on https://github.com/WebAssembly/tool-conventions/ might be useful since it involves the conventions that LLVM and clang use. If it's specific to the web, then https://groups.google.com/forum/#!forum/emscripten-discuss could be another place (even if you don't plan on using emscripten's JS code).