- User Since
- Jan 7 2013, 9:35 AM (254 w, 3 d)
Thu, Nov 9
I'm fine with making this explicit for now. I'm not sure I'd yet go so far as to say that the goal is not to support comdats. We should definitely investigate how C++ and its debug info are supported in mach-o, but it may be that comdats are better than whatever they do. In general in the absence of good reasons (e.g. our 2-level namespace) I'd probably have a slight bias toward doing things the ELF way rather than the macho way; most people I've talked to who've worked a lot with macho don't speak too highly of it :)
Wed, Nov 8
Tue, Nov 7
Yes; in particular, musl's implementation of printf has some code where it promotes everything to long double, so if long double is f128, that causes several of these functions to get linked in to every binary, for no real benefit to the user.
Yeah. Or not; I think you can still use __float128 types in clang, so if you did that then it would use these methods too. And as long as long double is f64 then none of these functions would get pulled into every program that uses printf (which is the case now), so it wouldn't impose any cost on anyone that doesn't actually use f128. So in that case there's not really a reason not to have it.
Also btw I still think I want to back out the "long-double-is-f128" ABI from wasm. But either way it probably doesn't hurt to have these in compiler-rt.
BTW I'm still trying to get caught up, so if there's some review or something you have in flight that you want me to get to first, just let me know.
Fri, Oct 27
Oct 24 2017
If you wanted to come up with your own set of syscalls or lower-layer stuff, You'd basically be making up your own OS, which you could name. But libclangrt.builtins in particular doesn't really depend on the OS, it's mostly just architecture-specific or processor-specific stuff. So you could reuse the clang support for that.
D39218 uses the path we want want if the OS is 'unknown'. But shouldn't we make it so that any 'wasm-*-*' triple behaves the same way? i.e. someone who wants to put their own OS name in there but wants to use that same layout (or for that matter, maybe we want to adopt that in emscripten too).
I don't necessarily object to this accessor.
But for our generic non-emscripten toolchain support that we are adding right now, maybe we should make it have whatever defaults we are building now with any OS (not just explicitly "unknown")?
oh wait it's in the subject line... just ignore me :D
Can you add to the commit description that this fixes the build with -DLLVM_LINK_LLVM_DYLIB=ON?
Oct 23 2017
Yeah, this seems OK for now.
I think at some point we may want to revisit the overall goals of the representation that we discussed originally (i.e. not having to encode a symbol table outside of wasm globals, a wasm object file possibly being directly loadable, etc), once we have a prototype and the constraints are clearer. Probably then it will be more clear what to do in cases like this.
Oct 20 2017
LGTM, assuming I am correct in thinking that this will not affect the current wasm-elf assembly output (which does emit .loc directives).
Should symbols actually be allowed to have the same value? How do we handle aliases?
Oct 18 2017
Oct 10 2017
Oct 9 2017
Cool, and the code is even shorter this way too 👍
Oct 6 2017
Oct 5 2017
Removing the requirement works fine now because the passes are set up correctly. The original intention behind getRequiredProperties in this case was that if the pass configuration were to be changed and PEI were to be called before regalloc on a target which wasn't prepared for that, there would be an easy assertion to say what went wrong.
Oct 3 2017
- remove spurious diff
Sep 28 2017
Oh, right, implicit locals. Yeah I don't think it's really worth thinking about too much.
This strategy seems fine to me.
How would this CL be different if it also affected the ELF flavor though? I don't mind fixing s2wasm if it makes LLVM simper.
Sep 27 2017
Missed this yesterday, sorry.
Sep 26 2017
I think it probably makes sense to use the function/global index space for the compiler and linker, and user-oriented tools (e.g. objdump, nm). For things like obj2yaml or readobj, maybe a something more raw would be appropriate, but I'd guess that it's not a super-high priority, other than it's nice to have those for testing.
oh also it looks like you left a couple of words out of the first paragraph of the commit message.
That can be a separate change though.
Is this in tool-conventions yet? If so, should update.
Sep 25 2017
Thanks for the heads-up. WebAssembly's current varargs calling convention does pass the fixed args in one way (via declared function arguments) and the non-fixed args in another way (in memory), so in the cases where NumFixedArgs was wrong we would have been doing the wrong thing. However wasm also typechecks all calls against the declared function signature, so any wrong cases would fail in obvious ways (a signature mismatch error at validation time for direct calls, and at runtime for indirect calls). So just making the behavior correct should be the right thing to do. Also we have explicitly promised that we don't have a stable ABI yet, so that helps too :D
Sep 20 2017
Sep 19 2017
Sep 15 2017
(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"?
Sep 14 2017
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?
Sep 13 2017
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?
Sep 12 2017
Sep 7 2017
- remove spurious change
Sep 6 2017
Sep 5 2017
Sep 1 2017
Aug 31 2017
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
Aug 30 2017
- 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?