- User Since
- Jan 7 2013, 9:35 AM (245 w, 6 d)
Wed, Sep 20
Tue, Sep 19
Fri, Sep 15
(btw if there's a link in the commit message to the review, it should close automatically after commit)
Right, I was actually referring to the VM's internal tracking of exception stacks. I'm assuming it will use its usual mechanism for JS exceptions, which does keep a stack trace that you can access from JS code. Rethrowing rather than a new throw might keep that one intact. It doesn't help with the C++ state, but it's still better than nothing.
Or you could just call it a revert of the previous patch, if you wanted.
So are we going to say that the mere existence of segment names tells the linker it should merge them as though they were separate data sections? (or, is there any other possible use?) Is there other potential per-MCSection metadata we'd want that would make this metadata end up more generic?
So, it just occurred to me; we don't have 2-phase unwinding, which is useful to get good stack traces for debugging (because if you have unhandled exceptions you can abort before the second phase, rather than after the stack has been unwound). If the VM has an exception object from the original throw, it should have a stack trace attached to it. If we rethrow rather than throwing anew each time, then it will be preserved. So could we try to use rethrow instead of throw where possible? If we think it even might be feasible, it might be worth leaving rethrow in for now.
Should the patch title be "create data *segments* based on MCSections"?
Thu, Sep 14
Oh also a nit: It's spelled 'separate' :D
Do wer have any tests with global vars that have an explicit section? We should have one, that tests that they get merged into a segment in the object file.
I think the reasoning was really just that code size reduction is even more important on wasm than other platforms, and because it's a new ABI there's no one depending on the default behavior that would break.
Shouldn't the LLVM-side tests be using the LLVM utils and not clang? Or did you mean lld tests, or waterfall tests or what?
Wed, Sep 13
I guess the reason for removing this is that we don't really have an analogue of different text sections in the wasm object format, but that dead function stripping can be done by any linker smart enough to link wasm at all. But this isn't necessarily true of data sections though, is it?
Tue, Sep 12
Thu, Sep 7
- remove spurious change
Wed, Sep 6
Tue, Sep 5
Fri, Sep 1
Thu, Aug 31
but "Fixes PRNNN" is fine to indicate that a bug is fixed, although now that I think of it, a real link (in either form) is probably even better. Either way there's no auto-linking. Not everyone even uses LLVM's bug tracker anyway.
ah we raced. LGTM :)
Cool, I guess this fixes PR34392, you should put that in the commit description too.
- address comments: remove atomic rules with constant offsets
Wed, Aug 30
- address review comments
- revert extra change
Aug 24 2017
Also looking back and my previous comment,
Aug 23 2017
OK nevermind, GlobalVariable doesn't make sense because you have to load it, and similarly for GEP. We could probably do inttoptr though, and maybe select, to get a function pointer and call that.
Are there any other constants/constexprs besides bitcast that we might encounter, that we should test? Looking at the constexprs, I don't see too much that looks likely. Maybe GEP? I guess you can indirectly call a GlobalVariable, which ought to look the same as a local function pointer, maybe we should have a test for that too (presumably dag isel handles that too).
Also that this patch really does is just bail from fast-isel when the callee is a constant (which is fine), and that has the effect of causing calls of bitcasts to be lowered as direct calls.
Aug 15 2017
Aug 14 2017
Jul 21 2017
Actually, maybe an even better idea:
getLibcallName() returns a StringRef and asserts rather than returning a failure code. getLIbcallNameIfAvailable() returns an Expected<StringRef> and can fail.
The former matches 90% of the existing uses. This would let us move away from char* for the external symbol APIs (assuming that's a good thing) but most of the existing uses wouldn't need to actually change, and those that check failure would still be simple.
So it looks like switching the API away from char*-based would be, maybe not super-trivial, but probably not too bad. It's interlinked essentially with other APIs relating to external symbols:
mostly SelectionDAG::getExternalSymbol(), and also MachineInstrBuilder::addExternalSymbol() but those could be converted too, and I don't think I'd mind doing it as general cleanup. I wonder if Expected<StringRef> is a little heavyweight though. It seems to have a really strong convention that the result should be checked first, and most use cases just don't care (arguably they could assert, but requiring that would add a lot of boilerplate asserts for probably not a lot of benefit). StringRef by itself (with empty string taking the place of the current use of nullptr) wouldn't be too bad, although having the empty string as a sort of sentinel value only used by a small fraction of the uses doesn't seem ideal either. I might still be inclined to prefer that over Expected though. Any thoughts?
Jul 19 2017
- remove redundant unsupported elements and simplify initialization
- clarify comments
Jul 18 2017
Yes; the wasm backend currently has a partial duplication of the libcall names that can go away (in fact, maybe i'll even roll that into this patch), and it also has signature information for each function. The signatures could go into the .def file as well, and the other users of the def file could just define a HANDLE_LIBCALL that ignores them.
Jul 17 2017
Jul 12 2017
Looking forward to reversing all of this soon :)
Jul 11 2017
OK, I got it; this is for the linker's use of the lib/object abstraction, that's what I was missing.
So just to be sure I understand, we need this because we only have segments for initialized data (and there is implicitly BSS in between), so the offset can't just be calculated from the sizes of the segments?
Jul 10 2017
So trivial.obj.wasm already has the updated linking section or this patch just depends on updating it?
Code looks good. Maybe also add a test to test/MC/WebAssembly/reloc-data.ll or one of those others too?
Jul 5 2017
One nit, otherwise LGTM. Don't forget to update Linking.md
LGTM with a nit. In the summary, did you mean that we were *not* setting their params and return types?
Jun 29 2017
This looks good. I guess we need to land the intrinsics on the LLVM side first though.
Jun 28 2017
Jun 27 2017